On Fri, Feb 28, 2025 at 10:38 AM Yafang Shao <[email protected]> wrote: > > On Fri, Feb 28, 2025 at 12:22 AM Petr Mladek <[email protected]> wrote: > > > > On Thu 2025-02-27 10:47:33, Yafang Shao wrote: > > > The tasklist_lock in the KLP transition might result high latency under > > > specific workload. We can replace it with RCU. > > > > > > After a new task is forked, its kernel stack is always set to empty[0]. > > > Therefore, we can init these new tasks to KLP_TRANSITION_IDLE state > > > after they are forked. If these tasks are forked during the KLP > > > transition but before klp_check_and_switch_task(), they can be safely > > > skipped during klp_check_and_switch_task(). Additionally, if > > > klp_ftrace_handler() is triggered right after forking, the task can > > > determine which function to use based on the klp_target_state. > > > > > > With the above change, we can safely convert the tasklist_lock to RCU. > > > > > > Link: > > > https://lore.kernel.org/all/20250213173253.ovivhuq2c5rmvkhj@jpoimboe/ [0] > > > Link: > > > https://lore.kernel.org/all/20250214181206.xkvxohoc4ft26uhf@jpoimboe/ [1] > > > Suggested-by: Josh Poimboeuf <[email protected]> > > > Signed-off-by: Yafang Shao <[email protected]> > > > --- > > > --- a/kernel/livepatch/patch.c > > > +++ b/kernel/livepatch/patch.c > > > @@ -95,7 +95,13 @@ static void notrace klp_ftrace_handler(unsigned long > > > ip, > > > > > > patch_state = current->patch_state; > > > > > > - WARN_ON_ONCE(patch_state == KLP_TRANSITION_IDLE); > > > + /* If the patch_state is KLP_TRANSITION_IDLE, it means the > > > task > > > + * was forked after klp_init_transition(). In this case, the > > > + * newly forked task can determine which function to use > > > based > > > + * on the klp_target_state. > > > + */ > > > + if (patch_state == KLP_TRANSITION_IDLE) > > > + patch_state = klp_target_state; > > > > > > > I'm a bit confused by your example. Let me break it down to ensure I > understand correctly: > > > CPU0 CPU1 > > > > task_0 (forked process): > > > > funcA > > klp_ftrace_handler() > > if (patch_state == KLP_TRANSITION_IDLE) > > patch_state = klp_target_state > > # set to KLP_TRANSITION_PATCHED > > > > # redirected to klp_funcA() > > It seems that the KLP is still in the process of transitioning, right? > > > > > > > echo 0 >/sys/kernel/livepatch/patch1/enabled > > > > klp_reverse_transition() > > > > klp_target_state = !klp_target_state; > > # set to KLP_TRANSITION_UNPATCHED > > If you attempt to initiate a new transition while the current one is > still ongoing, the __klp_disable_patch function will return -EBUSY, > correct? > > __klp_disable_patch > if (klp_transition_patch) > return -EBUSY; > > On the other hand, if klp_transition_patch is NULL, it indicates that > the first KLP transition has completed successfully.
Please disregard my previous comment. I missed the fact that it is called from klp_reverse_transition(). I will review your example again and give it more thought. -- Regards Yafang
