MIME-Version: 1.0
References: <20211222075406.57191-1-qiang1.zhang@intel.com>
In-Reply-To: <20211222075406.57191-1-qiang1.zhang@intel.com>
From:   Marco Elver <elver@google.com>
Date:   Thu, 23 Dec 2021 16:08:31 +0100
Message-ID: <CANpmjNOmaSxqTjUd8y=A+sK0jGg6c00t8uvmOey+QrJsDamsmQ@mail.gmail.com>
Subject: Re: [PATCH] rcu: record kasan stack before enter local_irq_save()/restore()
 critical area
To:     Zqiang <qiang1.zhang@intel.com>
Cc:     paulmck@kernel.org, ryabinin.a.a@gmail.com, urezki@gmail.com,
        jun.miao@intel.com, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 22 Dec 2021 at 08:54, Zqiang <qiang1.zhang@intel.com> 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,
-- Marco

> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> ---
>  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
>