switch to idle_host_task when idle

[laijs]

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]

Can one of the admins verify this patch?

[tavip]

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]

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]

pr was rebased, and the NO_HZ_IDLE was preserved.

[tavip]

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

[liuyuan10]

Not needed

[laijs]

the compiler will warn on it if it is removed.

[liuyuan10]

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

[laijs]

updated

[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

[laijs]

@liuyuan10 added a commit to fix it.

[tavip]

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]

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]

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.

[laijs]

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]

@lkl-jenkins: test this please

[liuyuan10]

LGTM. thanks for cleaning it up.

[thehajime]

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]

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.