On 2021/10/12 下午8:29, Steven Rostedt wrote: > On Tue, 12 Oct 2021 14:24:43 +0200 (CEST) > Miroslav Benes <mbenes@xxxxxxx> wrote: > >>> +++ b/kernel/livepatch/patch.c >>> @@ -52,11 +52,6 @@ static void notrace klp_ftrace_handler(unsigned long ip, >>> bit = ftrace_test_recursion_trylock(ip, parent_ip); >>> if (WARN_ON_ONCE(bit < 0)) >>> return; >>> - /* >>> - * A variant of synchronize_rcu() is used to allow patching functions >>> - * where RCU is not watching, see klp_synchronize_transition(). >>> - */ >>> - preempt_disable_notrace(); >>> >>> func = list_first_or_null_rcu(&ops->func_stack, struct klp_func, >>> stack_node); >>> @@ -120,7 +115,6 @@ static void notrace klp_ftrace_handler(unsigned long ip, >>> klp_arch_set_pc(fregs, (unsigned long)func->new_func); >>> >>> unlock: >>> - preempt_enable_notrace(); >>> ftrace_test_recursion_unlock(bit); >>> } >> >> I don't like this change much. We have preempt_disable there not because >> of ftrace_test_recursion, but because of RCU. ftrace_test_recursion was >> added later. Yes, it would work with the change, but it would also hide >> things which should not be hidden in my opinion. > > Agreed, but I believe the change is fine, but requires a nice comment to > explain what you said above. > > Thus, before the "ftrace_test_recursion_trylock()" we need: > > /* > * The ftrace_test_recursion_trylock() will disable preemption, > * which is required for the variant of synchronize_rcu() that is > * used to allow patching functions where RCU is not watching. > * See klp_synchronize_transition() for more details. > */ Will be in v2 too :-) Regards, Michael Wang > > -- Steve >
