lkl: let atomic ops outsourced #273

thehajime · 2016-11-21T08:53:04Z

This will allow us to reimplpement atomic ops with lock/touch/unlock way
if underlying hosts don't support a particular atomic ops.

Signed-off-by: Hajime Tazaki [email protected]

This change is

tavip · 2016-11-22T06:35:48Z

arch/lkl/kernel/threads.c

@@ -123,7 +123,7 @@ struct task_struct *__switch_to(struct task_struct *prev,
 	}

 	if (_prev->dead) {
-		__sync_fetch_and_sub(&threads_counter, 1);
+		lkl__sync_fetch_and_sub((int *)&threads_counter, 1);


Its probably better to just convert threads_counter to long instead of casting here.

tavip · 2016-11-22T06:35:54Z

arch/lkl/kernel/atomic.c

+	atomic_lock = lkl_ops->sem_alloc(1);
+	return 0;
+}
+early_initcall(lkl_atomic_ops_init);


I think we are missing the lock free.

thanks. added cleanups.

tavip · 2016-11-22T06:36:04Z

arch/lkl/kernel/atomic.c

@@ -0,0 +1,86 @@
+#include <linux/kernel.h>


Should we move this to tools/lkl/lib?

agree. I actually wanted to do it in toolks/lkl/lib but was reluctant to add new lkl_host_ops entries for various atomic ops. do you think it's nicer to do int host_ops or have better idea to do it in tools/lkl ?

I don't see any nicer way to do it and we will need them anyway for SMP support. We may need a different set for SMP, but we can change that later.

This will allow us to reimplpement atomic ops with lock/touch/unlock way if underlying hosts don't support a particular atomic ops. Signed-off-by: Hajime Tazaki <[email protected]>

thehajime · 2016-11-22T12:58:46Z

one thing that I wasn't sure is how to emulate __sync_synchronize() with semaphore in ARM environment. do you see any good idea ?

tavip · 2016-11-22T13:06:19Z

about __sync_synchronize() : mutex_lock / mutex_unlock should do it.

I forgot, but what platform is this that gcc doesn't support this builtins?

thehajime · 2016-11-22T13:19:48Z

about __sync_synchronize() : mutex_lock / mutex_unlock should do it.

don't understand what you described.. do you mean like this ?

void lkl__sync_synchronize(void)
{
	lkl_ops->sem_down(atomic_lock);
	lkl_ops->sem_up(atomic_lock);
}

I forgot, but what platform is this that gcc doesn't support this builtins?

% arm-none-eabi-gcc --version
arm-none-eabi-gcc (Fedora 5.2.0-4.fc24) 5.2.0

tavip · 2016-11-22T13:26:23Z

don't understand what you described.. do you mean like this ?
void lkl__sync_synchronize(void)
{
lkl_ops->sem_down(atomic_lock);
lkl_ops->sem_up(atomic_lock);
}

Yes, or the mutex ops, if we have it for that host.

I forgot, but what platform is this that gcc doesn't support this builtins?
% arm-none-eabi-gcc --version
arm-none-eabi-gcc (Fedora 5.2.0-4.fc24) 5.2.0

Hmm, I think you can get the buitins if you switch to using -mcpu=cortex-m4. Do you really need to support anything less then m4?

tavip · 2016-11-22T13:39:05Z

Hmm, I think you can get the buitins if you switch to using -mcpu=cortex-m4. Do you really need to support anything less then m4?

Actually it looks that -march=armv7 is better, since it covers both MCUs and CPUs

thehajime · 2016-11-25T12:45:52Z

Hmm, I think you can get the buitins if you switch to using -mcpu=cortex-m4. Do you really need to support anything less then m4?
Actually it looks that -march=armv7 is better, since it covers both MCUs and CPUs

I'm currently using arm926ej-s cpu (-mcpu arm926ej-s) for the target. This is just a randomly picked (not written by myself). for the qemu test, it might be built/tested with -march=armv7 with available atomic builtins, but for the bare-metal use case, the arm926 target might be also useful.

the atomic ops wrapper is useful for such situations if it is really needed.

mxi1 · 2017-05-16T02:50:12Z

arch/lkl/kernel/threads.c

@@ -5,7 +5,7 @@
 #include <asm/cpu.h>
 #include <asm/sched.h>

-static volatile int threads_counter;


Strange that no "threads_counter" is defined in my version ( based on a063e16 - Merge pull request #340 from libos-nuse/fix-checkpath-tak2).

I think this PR is a bit outdated: need to update/rebase in advance to apply the latest lkl.

I have added the code changes to the latest release, so we can see if the patch works for the ARM 32bit platforms. Will let you know if it works or not.

@thehajime The patch doesn't work for me right now.
After changing to lkl__sync_ functions, the __sync_synchronize used in lib/virtio.c becomes undefined once again. Then, I tried to change it to lkl__sync_synchronize and compile, what I got is that there're two lkl__sync_synchronize symbols in liblkl-hijack.so:

00061454 l F .text 00000044 lkl__sync_synchronize 00000000 *UND* 00000000 lkl__sync_synchronize

The undefined symbol comes from lib/lkl-in.o, the defined one comes from lib/lkl.o. When running AR to generate liblkl.a based on lkl-in.o and lkl.o, the two symbols are joint together.

What should I do to get rid of it? I am not professional on GCC toolchain....

mxi1 · 2017-05-16T02:51:01Z

arch/lkl/kernel/threads.c

@@ -220,7 +220,7 @@ void threads_init(void)

 void threads_cnt_dec(void)


No such one function is defined and used in the latest code base (a063e16).

tavip reviewed Nov 22, 2016

View reviewed changes

lkl: let atomic ops outsourced

31e75dd

This will allow us to reimplpement atomic ops with lock/touch/unlock way if underlying hosts don't support a particular atomic ops. Signed-off-by: Hajime Tazaki <[email protected]>

thehajime force-pushed the fix-atomic-outsource branch from 0865053 to 31e75dd Compare November 22, 2016 12:58

thehajime mentioned this pull request May 15, 2017

For arm 32bit android build, the "__sync_synchronize" symbol is UNDEFINED in liblkl-hijack.so #348

Closed

mxi1 reviewed May 16, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lkl: let atomic ops outsourced #273

lkl: let atomic ops outsourced #273

thehajime commented Nov 21, 2016 •

edited

Loading

tavip Nov 22, 2016

thehajime Nov 22, 2016

tavip Nov 22, 2016

thehajime Nov 22, 2016

tavip Nov 22, 2016

thehajime Nov 22, 2016

tavip Nov 22, 2016

thehajime commented Nov 22, 2016

tavip commented Nov 22, 2016

thehajime commented Nov 22, 2016

tavip commented Nov 22, 2016

tavip commented Nov 22, 2016

thehajime commented Nov 25, 2016

mxi1 May 16, 2017

thehajime May 16, 2017

mxi1 May 16, 2017

mxi1 May 17, 2017

mxi1 May 16, 2017

		@@ -220,7 +220,7 @@ void threads_init(void)

		void threads_cnt_dec(void)

lkl: let atomic ops outsourced #273

Are you sure you want to change the base?

lkl: let atomic ops outsourced #273

Conversation

thehajime commented Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thehajime commented Nov 22, 2016

tavip commented Nov 22, 2016

thehajime commented Nov 22, 2016

tavip commented Nov 22, 2016

tavip commented Nov 22, 2016

thehajime commented Nov 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thehajime commented Nov 21, 2016 •

edited

Loading