Message-ID: <CAPjX3Fed9D1wdjvLyhXq5MB2aKOtUPsxx7gaEM_81q32hgnKtQ@mail.gmail.com>
Date: Thu, 13 Nov 2025 13:30:38 +0100
From: Daniel Vacek <neelx@...e.com>
To: dsterba@...e.cz
Cc: Gladyshev Ilya <foxido@...ido.dev>, David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 4/8] btrfs: simplify function protections with guards

On Thu, 13 Nov 2025 at 12:25, David Sterba <dsterba@...e.cz> wrote:
>
> On Thu, Nov 13, 2025 at 01:06:42PM +0300, Gladyshev Ilya wrote:
> > On 11/13/25 11:43, David Sterba wrote:
> > > On Wed, Nov 12, 2025 at 09:49:40PM +0300, Gladyshev Ilya wrote:
> > >> Replaces cases like
> > >>
> > >> void foo() {
> > >>      spin_lock(&lock);
> > >>      ... some code ...
> > >>      spin_unlock(&lock)
> > >> }
> > >>
> > >> with
> > >>
> > >> void foo() {
> > >>      guard(spinlock)(&lock);
> > >>      ... some code ...
> > >> }
> > >>
> > >> While it doesn't has any measurable impact,
> > >
> > > There is impact, as Daniel mentioned elsewhere, this also adds the
> > > variable on stack. It's measuable on the stack meter, one variable is
> > > "nothing" but combined wherever the guards are used can add up. We don't
> > > mind adding variables where needed, occasional cleanups and stack
> > > reduction is done. Here it's a systematic stack use increase, not a
> > > reason to reject the guards but still something I cannot just brush off
> > I thought it would be optimized out by the compiler in the end, I will
> > probably recheck this
>
> There are cases where compiler will optimize it out, IIRC when the lock
> is embedded in a structure, and not when the pointer is dereferenced.

Yes. If you have a pointer to a structure with a lock embedded in it,
the compiler knows the local variable is stable (as in non-volatile or
const, the pointer does not change) and is happy to use it and
optimize out the explicit copy. It knows both values are the same and
so it is safe to do so. At least starting with -O2 with gcc with my
tests.

But if you have a pointer to a pointer to some structure, the compiler
(unlike us) cannot make the same assumptions. Strictly speaking, since
the address to the structure containing the lock is not in a local
variable but somewhere in the memory, it can change during the
execution of the function. We as developers know the pointers are
reasonably stable as the code is usually designed in such a way. And
if it's not, we know that as well and modify the locking accordingly.
So usually we're happy to `spin_unlock(&foo->bar->lock)`, but for the
compiler this may be a different lock then the one initially locked
before. From the language point of view, the address of `bar` could
have changed in the meantime. And so the compiler is forced to use the
local copy of the pointer to the lock saved when acquiring the lock
and cannot optimize it out.

So this really depends case by case. Using the guard in place of
patterns like `spin_unlock(&foo->lock)` can be optimized out while
guard for patterns like `spin_unlock(&foo->bar->lock)` need to use an
extra stack space for the local variable storing a copy of the lock
address.

You would really have to use `struct bar *bar = foo->bar;
guard(spinlock)(&bar->lock); ...`. But then you are explicitly using
the stack yourself. So it's equivalent.

> > >> it makes clear that whole
> > >> function body is protected under lock and removes future errors with
> > >> additional cleanup paths.
> > >
> > > The pattern above is the one I find problematic the most, namely in
> > > longer functions or evolved code. Using your example as starting point
> > > a followup change adds code outside of the locked section:
> > >
> > > void foo() {
> > >      spin_lock(&lock);
> > >      ... some code ...
> > >      spin_unlock(&lock)
> > >
> > >      new code;
> > > }
> > >
> > > with
> > >
> > > void foo() {
> > >      guard(spinlock)(&lock);
> > >      ... some code ...
> > >
> > >      new code;
> > > }
> > >
> > > This will lead to longer critical sections or even incorrect code
> > > regarding locking when new code must not run under lock.
> > >
> > > The fix is to convert it to scoped locking, with indentation and code
> > > churn to unrelated code to the new one.
> > >
> > > Suggestions like refactoring to make minimal helpers and functions is
> > > another unecessary churn and breaks code reading flow.
> >
> > What if something like C++ unique_lock existed? So code like this would
> > be possible:
> >
> > void foo() {
> >    GUARD = unique_lock(&spin);
> >
> >    if (a)
> >      // No unlocking required -> it will be done automatically
> >      return;
> >
> >    unlock(GUARD);
> >
> >    ... unlocked code ...
> >
> >    // Guard undestands that it's unlocked and does nothing
> >    return;
> > }
> >
> > It has similar semantics to scoped_guard [however it has weaker
> > protection -- goto from locked section can bypass `unlock` and hold lock
> > until return], but it doesn't introduce diff noise correlated with
> > indentations.
> >
> > Another approach is allowing scoped_guards to have different indentation
> > codestyle to avoid indentation of internal block [like goto labels for
> > example].
> >
> > However both of this approaches has their own downsides and are pure
> > proposals
>
> Thanks, it's good to have concrete examples to discuss. It feels like
> we'd switch away from C and force patterns that are maybe common in
> other languages like C++ that do things under the hood and it's fine
> there as it's the mental programming model one has. This feels alien to
> kernel C programming, namely for locking where the "hide things" is IMO
> worse than "make things explicit".
>
> There are cases where switching to guards would be reasonable because
> the functions are short and simple but then it does not utilize the
> semantics of automatic cleanup. In more complex functions it would mean
> to make more structural changes to the code at the cost of churn and
> merge conflicts of backported patches.

Powered by blists - more mailing lists