Re: [PATCH] livepatch: Fix idle cpu's tasks transition
From: Petr Mladek <pmladek@suse.com>
Date: 2021-09-10 09:17:59
Also in:
lkml
On Thu 2021-09-09 11:16:01, Vasily Gorbik wrote:
quoted hunk ↗ jump to hunk
On an idle system with large amount of cpus it might happen that klp_update_patch_state() is not reached in do_idle() for a long periods of time. With debug messages enabled log is filled with: [ 499.442643] livepatch: klp_try_switch_task: swapper/63:0 is running without any signs of progress. Ending up with "failed to complete transition". On s390 LPAR with 128 cpus not a single transition is able to complete and livepatch kselftests fail. Tests on idling x86 kvm instance with 128 cpus demonstrate similar symptoms with and without CONFIG_NO_HZ. To deal with that, since runqueue is already locked in klp_try_switch_task() identify idling cpus and trigger rescheduling potentially waking them up and making sure idle tasks break out of do_idle() inner loop and reach klp_update_patch_state(). This helps to speed up transition time while avoiding unnecessary extra system load. Signed-off-by: Vasily Gorbik <gor@linux.ibm.com> --- Previous discussion and RFC PATCH: lkml.kernel.org/r/patch.git-b76842ceb035.your-ad-here.call-01625661932-ext-1304@work.hours kernel/livepatch/transition.c | 2 ++ 1 file changed, 2 insertions(+)diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index 3a4beb9395c4..c5832b2dd081 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c@@ -308,6 +308,8 @@ static bool klp_try_switch_task(struct task_struct *task)
Please, update also the comment above klp_try_switch_task(). I would write something like: /* * Try to safely switch a task to the target patch state. If it's currently * running, or it's sleeping on a to-be-patched or to-be-unpatched function, or * if the stack is unreliable, return false. * * Idle tasks are switched in the main loop when running. */
rq = task_rq_lock(task, &flags);
if (task_running(rq, task) && task != current) {This would deserve a comment, for example: /* * Idle task might stay running for a long time. Switch them * in the main loop. */
+ if (is_idle_task(task)) + resched_curr(rq); snprintf(err_buf, STACK_ERR_BUF_SIZE, "%s: %s:%d is running\n", __func__, task->comm, task->pid);
Otherwise, it looks good to me. With the two comments: Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr