On Fri, Jan 31, 2025 at 9:39 PM Petr Mladek <pmladek@xxxxxxxx> wrote:
>
> On Fri 2025-01-31 21:22:13, zhang warden wrote:
> >
> >
> > > On Jan 22, 2025, at 20:50, Petr Mladek <pmladek@xxxxxxxx> wrote:
> > >
> > > With this patch, any operation which takes the tasklist_lock might
> > > break klp_try_complete_transition(). I am afraid that this might
> > > block the transition for a long time on huge systems with some
> > > specific loads.
> > >
> > > And the problem is caused by a printk() added just for debugging.
> > > I wonder if you even use a slow serial port.
> > >
> > > You might try to use printk_deferred() instead. Also you might need
> > > to disable interrupts around the read_lock()/read_unlock() to
> > > make sure that the console handling will be deferred after
> > > the tasklist_lock gets released.
> > >
> > > Anyway, I am against this patch.
> > >
> > > Best Regards,
> > > Petr
> >
> > Hi, Petr.
> >
> > I am unfamiliar with the function `rwlock_is_contended`, but it seems this function will not block and just only check the status of the rw_lock.
> >
> > If I understand it right, the problem would raise from the `break` which will stop the process of `for_each_process_thread`, right?
>
> You got it right. I am afraid that it might create a livelock
> situation for the livepatch transition. I mean that the check
> might almost always break on systems with thousands of processes
> and frequently created/exited processes. It always has
> to start from the beginning.
It doesn’t start from the beginning, as tasks that have already
switched over will be skipped.
Since the task->patch_state is set before the task is added to the
task list and the child’s patch_state is inherited from the parent, I
believe we can remove the tasklist_lock and use RCU instead, as
follows:
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 30187b1d8275..1d022f983bbc 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -399,11 +399,11 @@ void klp_try_complete_transition(void)
* Usually this will transition most (or all) of the tasks on a system
* unless the patch includes changes to a very common function.
*/
- read_lock(&tasklist_lock);
+ read_rcu_lock();
for_each_process_thread(g, task)
if (!klp_try_switch_task(task))
complete = false;
- read_unlock(&tasklist_lock);
+ read_rcu_unlock();
/*
* Ditto for the idle "swapper" tasks.
--
Regards
Yafang
