RE: [PATCH] rcu: record kasan stack before enter local_irq_save()/restore() critical area

[Zhang, Qiang1]

On Wed, 22 Dec 2021 at 08:54, Zqiang <qiang1.zhang@xxxxxxxxx> wrote:
> The kasan_record_aux_stack_noalloc() only record stack, it doesn't 
> need to be called in local_irq_save()/restore() critical area, and the 
> global spinlock (depot_lock) will be acquired in this function, When 
> enable kasan stack, locking contention may increase the time in the critical area.
>
>I think the change itself is harmless, because
>kasan_record_aux_stack_noalloc() doesn't care if interrupts are enabled or not when called, but the justification isn't clear to me.
>
>What "locking contention" are you speaking about? You're moving a
>local_irq_save() which disables interrupts. Yes, it might be nice to reduce the time interrupts are disabled, but in this case the benefit (if any) isn't clear at all, also because this only benefits non-production KASAN kernels.
>
>Can you provide better justification? Did you encounter a specific problem, maybe together with data?
>

Thanks for reply, Yes, this only benefits non-production KASAN kernel.  In KASAN kernel, 
there may be a lot of call stack recorded, in addition to locking competition, the find_stack()  will
also take a long time.


Thanks,
Zqiang

>Thanks,
>-- Marco

> Signed-off-by: Zqiang <qiang1.zhang@xxxxxxxxx>
> ---
>  kernel/rcu/tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 
> 347dae1876a6..5198e44cb124 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3030,8 +3030,8 @@ __call_rcu(struct rcu_head *head, rcu_callback_t func)
>         }
>         head->func = func;
>         head->next = NULL;
> -       local_irq_save(flags);
>         kasan_record_aux_stack_noalloc(head);
> +       local_irq_save(flags);
>         rdp = this_cpu_ptr(&rcu_data);
>
>         /* Add the callback to our list. */
> --
> 2.25.1
>