switch to idle_host_task when idle #336

laijs · 2017-02-22T07:35:38Z

Add a idle_host_task, and the cpu will wake up the
idle_host_task when enter idle.

The idle_host_task simulates resuming userspace
when scheduled. Thus the irqs and syscalls can
enter the kernel directly.

It also simplifies the idle code a lot, and
removes the error-prone hacks.

This change is

lkl-jenkins · 2017-02-22T07:35:40Z

Can one of the admins verify this patch?

tavip · 2017-02-22T09:31:57Z

This removes the call to cpu_idle_loop? We must run the cpu_idle_loop as it does some important jobs like time keeping setups.

Also, removing NO_HZ is not good, as it will force generating interrupts when not needed, increasing the overhead.

laijs · 2017-02-22T10:07:31Z

rest_init() -> cpu_startup_entry() -> cpu_idle_loop()
So the code doesn't remove the call to cpu_idle_loop().
It just changes arch_cpu_idle().

important jobs like time keeping setups.

there is no important job if these is no interrupt nor syscalls.
if interrupt or syscalls happens, any resulted important job will be scheduled to complete.

Also, removing NO_HZ is not good, as it will force generating interrupts when not needed, increasing the overhead.

I think the lkl-linux cpu is running on "host-task" in the "userspace" in most time, so you can't stop "generating interrupts". (correct me if I'm wrong.)

We might need to add some change to NO_HZ_FULL and make it works for CPU0, thus we can use NO_HZ_FULL for "host-task".

laijs · 2017-02-23T01:31:35Z

pr was rebased, and the NO_HZ_IDLE was preserved.

tavip · 2017-02-23T09:54:58Z

@laijs I will need more time to review this, sorry. I will get back to you during the week-end.

liuyuan10 · 2017-02-24T05:08:59Z

arch/lkl/kernel/syscalls.c

+		lkl_ops->sem_down(ti->sched_sem);
+		if (idle_host_task == NULL) {
+			lkl_ops->thread_exit();
+			return 0;


the compiler will warn on it if it is removed.

liuyuan10 · 2017-02-24T05:16:10Z

arch/lkl/kernel/syscalls.c

 	return 0;
 }

 void syscalls_cleanup(void)
 {
+	struct thread_info *ti = task_thread_info(idle_host_task);


I think there is chance that this idle_host_task never runs. Safer to check if it's null first?

Add a idle_host_task, and the cpu will wake up the idle_host_task when enter idle. The idle_host_task simulates resuming userspace when scheduled. Thus the irqs and syscalls can enter the kernel directly. It also simplifies the idle code a lot, and removes the error-prone hacks. Signed-off-by: Lai Jiangshan <[email protected]>

liuyuan10

Can you test netperf TCP_RR on this patch?
I'm seeing it slows it down about 2x.
My guess is that the new idle_host_task has a higher priority so it takes longer for the application thread waiting on socket read to preempt it.

cd tools/lkl
sh tests/run_netperf.sh 192.168.13.1 5 0 TCP_RR

Signed-off-by: Lai Jiangshan <[email protected]>

laijs · 2017-02-25T14:08:12Z

@liuyuan10 added a commit to fix it.

tavip · 2017-02-27T08:16:46Z

This does not look right to me. The cpu_idle_loop() has effectively been replaced with idle_host_task_loop() but they are not equivalent. In particular it is missing at least tick_nohz_idle_enter() / tick_nohz_idle_exit(), quiet_vmstat() and rcu_idle_enter()/rcu_idle_exit() which is needed even in our use-case.

Even if we fix these missing calls, I don't like messing with cpu_idle_loop() as it may change in the future and that will make it difficult to maintain it.

laijs · 2017-02-27T15:45:48Z

This does not look right to me. The cpu_idle_loop() has effectively been replaced with idle_host_task_loop() but they are not equivalent. In particular it is missing at least tick_nohz_idle_enter() / tick_nohz_idle_exit(), quiet_vmstat() and rcu_idle_enter()/rcu_idle_exit() which is needed even in our use-case.

Please remember, in most cases, the lkl-linux is in the "userspace" context, rather than idle mode.
idle_host_task_loop() makes it always "userspace" context when the kernel leaves the cpu.

tavip · 2017-03-04T11:18:13Z

Please remember, in most cases, the lkl-linux is in the "userspace" context, rather than idle mode.
idle_host_task_loop() makes it always "userspace" context when the kernel leaves the cpu.

OK, it now makes sense and it looks good to me. Can you also revert 9072b9a ("lkl: expose cpu_idle_loop from idle.c") as it looks like we don't need it?

Sorry it took so long to review this.

cpu_idle_loop() is unneeded to be exposed since the lkl will switch to the idle_host_task when idle, and the hack for cpu_idle_loop() is unneeded. Signed-off-by: Lai Jiangshan <[email protected]>

laijs · 2017-03-05T00:47:39Z

reverted 9072b9a and pushed. Thanks for review!

tavip

Looks good to me. @liuyuan10 can you also take a look and let us know if this works on your setup?

tavip · 2017-03-05T08:35:53Z

@lkl-jenkins: test this please

liuyuan10 · 2017-03-05T18:07:23Z

LGTM. thanks for cleaning it up.

thehajime · 2017-03-06T01:37:20Z

just a side note after a trial with my private branch (PR #255).

I faced a trouble which no irqs are triggered after this update, and it turned out that an incomplete TLS implementation (there is tls_alloc() but always returns NULL) doesn't work well with this patch.

if I updated the initialization of idle_host_task_loop like below, everything works fine.

I think we don't need to change this but just for your information.

diff --git a/arch/lkl/kernel/syscalls.c b/arch/lkl/kernel/syscalls.c
index fbbbcce..d6f70a3 100644
--- a/arch/lkl/kernel/syscalls.c
+++ b/arch/lkl/kernel/syscalls.c
@@ -166,17 +166,15 @@ int syscalls_init(void)
        set_thread_flag(TIF_HOST_THREAD);
        host0 = current;
 
+       if (kernel_thread(idle_host_task_loop, NULL, CLONE_FLAGS) < 0) {
+               return -1;
+       }
        if (lkl_ops->tls_alloc) {
                task_key = lkl_ops->tls_alloc(del_host_task);
                if (!task_key)
                        return -1;
        }
 
-       if (kernel_thread(idle_host_task_loop, NULL, CLONE_FLAGS) < 0) {
-               if (lkl_ops->tls_free)
-                       lkl_ops->tls_free(task_key);
-               return -1;
-       }
 
        return 0;
 }

laijs · 2017-03-06T02:32:57Z

I faced a trouble which no irqs are triggered after this update,

Because idle_host_task is NULL on this incomplete TLS implementation, I thought the lkl-kernel is considered fails on setup in this case. We need to change the initialization as your code or make the caller of syscall_init() respect the return value, or both.

laijs mentioned this pull request Feb 22, 2017

Linux v4.10 merge #335

Merged

laijs force-pushed the lkl/idle_host_task branch from 915cfbb to ffa8185 Compare February 23, 2017 01:28

liuyuan10 reviewed Feb 24, 2017

View reviewed changes

laijs force-pushed the lkl/idle_host_task branch from ffa8185 to 88f8dc2 Compare February 24, 2017 08:00

liuyuan10 reviewed Feb 24, 2017

View reviewed changes

don't leave cpu if there is any running task

2545621

Signed-off-by: Lai Jiangshan <[email protected]>

revert 9072b9a ("lkl: expose cpu_idle_loop from idle.c")

a7d380d

cpu_idle_loop() is unneeded to be exposed since the lkl will switch to the idle_host_task when idle, and the hack for cpu_idle_loop() is unneeded. Signed-off-by: Lai Jiangshan <[email protected]>

tavip approved these changes Mar 5, 2017

View reviewed changes

liuyuan10 merged commit be0ea3f into lkl:master Mar 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

switch to idle_host_task when idle #336

switch to idle_host_task when idle #336

laijs commented Feb 22, 2017 •

edited

Loading

lkl-jenkins commented Feb 22, 2017

tavip commented Feb 22, 2017

laijs commented Feb 22, 2017

laijs commented Feb 23, 2017

tavip commented Feb 23, 2017

liuyuan10 Feb 24, 2017

laijs Feb 24, 2017

liuyuan10 Feb 24, 2017

laijs Feb 24, 2017

liuyuan10 left a comment

laijs commented Feb 25, 2017

tavip commented Feb 27, 2017

laijs commented Feb 27, 2017 •

edited

Loading

tavip commented Mar 4, 2017

laijs commented Mar 5, 2017 •

edited

Loading

tavip left a comment

tavip commented Mar 5, 2017

liuyuan10 commented Mar 5, 2017

thehajime commented Mar 6, 2017

laijs commented Mar 6, 2017

switch to idle_host_task when idle #336

switch to idle_host_task when idle #336

Conversation

laijs commented Feb 22, 2017 • edited Loading

lkl-jenkins commented Feb 22, 2017

tavip commented Feb 22, 2017

laijs commented Feb 22, 2017

laijs commented Feb 23, 2017

tavip commented Feb 23, 2017

liuyuan10 Feb 24, 2017

Choose a reason for hiding this comment

laijs Feb 24, 2017

Choose a reason for hiding this comment

liuyuan10 Feb 24, 2017

Choose a reason for hiding this comment

laijs Feb 24, 2017

Choose a reason for hiding this comment

liuyuan10 left a comment

Choose a reason for hiding this comment

laijs commented Feb 25, 2017

tavip commented Feb 27, 2017

laijs commented Feb 27, 2017 • edited Loading

tavip commented Mar 4, 2017

laijs commented Mar 5, 2017 • edited Loading

tavip left a comment

Choose a reason for hiding this comment

tavip commented Mar 5, 2017

liuyuan10 commented Mar 5, 2017

thehajime commented Mar 6, 2017

laijs commented Mar 6, 2017

laijs commented Feb 22, 2017 •

edited

Loading

laijs commented Feb 27, 2017 •

edited

Loading

laijs commented Mar 5, 2017 •

edited

Loading