MIME-Version: 1.0
References: <20211222075406.57191-1-qiang1.zhang@intel.com>
 <CANpmjNOmaSxqTjUd8y=A+sK0jGg6c00t8uvmOey+QrJsDamsmQ@mail.gmail.com> <PH0PR11MB58802F642473BA6213446AB7DA7F9@PH0PR11MB5880.namprd11.prod.outlook.com>
In-Reply-To: <PH0PR11MB58802F642473BA6213446AB7DA7F9@PH0PR11MB5880.namprd11.prod.outlook.com>
From:   Marco Elver <elver@google.com>
Date:   Fri, 24 Dec 2021 12:02:21 +0100
Message-ID: <CANpmjNPU9Yw3RtsSrn0ZfdKQ7XUEp-Ecu9Wcx4HGhH3D+6-CrQ@mail.gmail.com>
Subject: Re: [PATCH] rcu: record kasan stack before enter local_irq_save()/restore()
 critical area
To:     "Zhang, Qiang1" <qiang1.zhang@intel.com>
Cc:     "paulmck@kernel.org" <paulmck@kernel.org>,
        "ryabinin.a.a@gmail.com" <ryabinin.a.a@gmail.com>,
        "urezki@gmail.com" <urezki@gmail.com>,
        "Miao, Jun" <jun.miao@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 24 Dec 2021 at 04:23, Zhang, Qiang1 <qiang1.zhang@intel.com> wrote:
>
>
> 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 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.

But there's no locking here, it's disabling interrupts. Yes, a lock is
taken inside kasan_record_aux_stack_noalloc(), but that's not one you
can do much about.

I don't mind this patch, but I think there might be some confusion. A
better explanation (in commit message or otherwise) would help make
sure we're not talking about different things.