Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230526205204.861311518@infradead.org> <CAHk-=wg2RHZKTN29Gr7MhgYfaNtzz58wry9jCNP75LAmQ9t8-A@mail.gmail.com>
 <20230530092342.GA149947@hirez.programming.kicks-ass.net> <20230606094251.GA907347@hirez.programming.kicks-ass.net>
 <CAHk-=wi-RyoUhbChiVaJZoZXheAwnJ7OO=Gxe85BkPAd93TwDA@mail.gmail.com> <20230606134005.GE905437@hirez.programming.kicks-ass.net>
In-Reply-To: <20230606134005.GE905437@hirez.programming.kicks-ass.net>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 6 Jun 2023 07:50:47 -0700
Message-ID: <CAHk-=wgQ5m+SnWTYGHu0JgYXTk2dkGF+msX=ARfYoo3t1_fX9g@mail.gmail.com>
Subject: Re: [PATCH v2 0/2] Lock and Pointer guards
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     keescook@chromium.org, gregkh@linuxfoundation.org,
        pbonzini@redhat.com, linux-kernel@vger.kernel.org,
        ojeda@kernel.org, ndesaulniers@google.com, mingo@redhat.com,
        will@kernel.org, longman@redhat.com, boqun.feng@gmail.com,
        juri.lelli@redhat.com, vincent.guittot@linaro.org,
        dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com,
        mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com,
        paulmck@kernel.org, frederic@kernel.org, quic_neeraju@quicinc.com,
        joel@joelfernandes.org, josh@joshtriplett.org,
        mathieu.desnoyers@efficios.com, jiangshanlai@gmail.com,
        rcu@vger.kernel.org, tj@kernel.org, tglx@linutronix.de,
        linux-toolchains@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Jun 6, 2023 at 6:40=E2=80=AFAM Peter Zijlstra <peterz@infradead.org=
> wrote:
>
> I know the name from C++ where it is std::lock_guard<> (well, back when
> I still did C++ it wasn't std, but whatever), and Rust seems to have
> std::sync::MutexGuard<>.
>
> But if that's the sole objection left, lets have a bike-shed party and
> pick a colour :-)
>
> 'shield' 'sentry' 'sentinel' 'keeper' 'custodian' 'warden' ?

I feel like you seem entirely too fixated on locking.

That's not even _remotely_ the most interesting case, I think.

For the common locking patterns , maybe "guard" makes sense as part of
the name, but it damn well shouldn't be about any "ptr_guard" or
whatever.

I think it should be some trivial

        mutex_scope(mymytex) {
                ..
        }

and yes, in that case scoping makes tons of sense and might even be
required, because you do want to visually see the scope of the
locking.

I'm not seeing the point of "guard" in that name either, but I don't
think the above is the complex case for naming, for use, or for
syntax.

In contrast, I think reference counting and allocations are the much
more interesting one, and the case where you are also more likely to
have multiple consecutive ones, and where it's likely less about "this
region is protected" and much more about the RAII patterns and
resources that you just want cleanup for.

So that's why I had that "auto_release" name - to make it clear that
it's about the *cleanup*, not about some "guarded region". See my
argument?

The locking cases can probably be handled with just a couple of macros
(mutex, rcu, maybe spinlock?) and I would violently argue that the
locking cases will have to have unconditional cleanup (ie if you
*ever* have the situation where some path is supposed to be able to
return with the lock held and no cleanup, then you should simply not
use the "mutex_scope()" kind of helper AT ALL).

So locking is, I feel, almost uninteresting. There are only a few
cases, and only trivial patterns for it, because anything non-trivial
had better be done very explicitly, and very much visibly by hand.

But look at the cases where we currently use "goto exit" kind of code,
or have duplicated cleanups because we do "cleanup(x); return -ERRNO"
kinds of patterns.

Some of them are locking, yes. But a lot of them are more generic "do
this before returning" kinds of things: freeing (possibly complex)
data structures, uncharging statistcs, or even things like writing
results back to user space.

And, unlike locking, those often (but not always) end up having the
"don't do this in the single success case" situation, so that you need
to have some way to show "ok, don't release it after all". Yes, that
ends up in practice likely being setting that special pointer to NULL,
but I think we need to have a much better syntax for it to show that
"now we're *not* doing the auto-release".

So it's that more complex case that I

 (a) don't think "guard" makes any sense for at all

 (b) think it's where the bigger payoffs are

 (c) think that generally are not "nested" (you release the resources
you've allocated, but you don't "nest" the allocations - they are just
serial.

 (d) think that the syntax could be pretty nasty, because the cleanup
is not just a trivial fixed "unlock" function

Just as an example of something that has a bit of everything, look at
kernel/cpu.c and the _cpu_down() function in particular, which has
that "goto out" to do all the cleanup.

That looks like a perfect case for having some nice "associate the
cleanup with the initialization" RAII patterns, but while it does have
one lock (cpus_write_lock/unlock()), even that one is abstracted away
so that it may be a no-op, and might be a percpu rwlock, and so having
to create a special macro for that would be more work than is worth
it.

Because what I *don't* want to happen is that people create some
one-time-use "helper" macro infrastructure to use this all. Again,
that cpus_write_lock/unlock is a good example of this.

End result: to avoid having crazy indentation, and crazy one-time
helper inline functions or whatever, I think you'd really want some
fairly generic model for

  "I am doing X, and I want to to Y as a cleanup when you exit the function=
"

where both X and Y can be *fairly* complicated things. They might be
as simple (and reasonably common) as "fd =3D fdget(f)" and "fdput(fd)",
but what about the one-offs like that cpus_write_lock/unlock pattern?

That one only happens in two places (_cpu_down() and _cpu_up()), and
maybe the answer is "we can't reasonably do it for that, because the
complexity is not worth it".

But maybe we *can*.

For example, this is likely the only realistic *good* case for the
horrible "nested function" thing that gcc supports. In my "look, we
could clean up a 'struct fd' example from last week, I made it do
something like this:

    /* Trivial "getfd()" wrapper */
    static inline void release_fd(struct fd *fd)
    { fdput(*fd); }

    #define auto_getfd(name, n) \
        auto_release_name(struct fd, name, fdget(n), release_fd)

but it would possibly be much more generic, and much more useful, if
that "release_fd()" function was generated by the macro as a local
nested inline function.

End result: you could use any arbitrary local cleanup code.

So you could have something like

  #define RAII(type, var, init, exit) \
        __RAII(type, var, init, exit, __UNIQUE_ID(fn)

  #define __RAII(type, var, init, exit, exitname) \
        void exitname(type *p) { exit } \
        type var __attribute__((__cleanup__(exitname))) =3D (init)

and do all of the above with

    RAII(struct fd, fd, fdget(f), fdput(fd));

because that macro would literally expand to create a (uniquely named)
nested function that then contains that "fdput(fd)".

I dunno. The syntax looks pretty bad, and I'm not even convinced clang
supports nested functions, so the above may not be an option.

But wouldn't it be nice to just be able to declare that arbitrary
cleanup in-place?

Then the lock cases would really just be trivial helper wrappers. And
you can use "guard" there if you want, but I feel it makes absolutely
*no* sense in the generic case. There is absolutely nothng that is
being "guarded" by having an automatic cleanup of some random
variable.

              Linus