Date:   Fri, 16 Jun 2023 09:57:16 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     "Paul E. McKenney" <paulmck@...nel.org>
Cc:     linux-kernel@...r.kernel.org, akiyks@...il.com,
        boqun.feng@...il.com, corbet@....net, keescook@...omium.org,
        linux@...linux.org.uk, linux-doc@...r.kernel.org,
        mchehab@...nel.org, peterz@...radead.org, rdunlap@...radead.org,
        sstabellini@...nel.org, will@...nel.org
Subject: Re: [PATCH v2 25/27] locking/atomic: scripts: generate kerneldoc
 comments

On Thu, Jun 15, 2023 at 07:07:13AM -0700, Paul E. McKenney wrote:
> On Mon, Jun 05, 2023 at 08:01:22AM +0100, Mark Rutland wrote:
> > Currently the atomics are documented in Documentation/atomic_t.txt, and
> > have no kerneldoc comments. There are a sufficient number of gotchas
> > (e.g. semantics, noinstr-safety) that it would be nice to have comments
> > to call these out, and it would be nice to have kerneldoc comments such
> > that these can be collated.
> > 
> > While it's possible to derive the semantics from the code, this can be
> > painful given the amount of indirection we currently have (e.g. fallback
> > paths), and it's easy to be mislead by naming, e.g.
> > 
> > * The unconditional void-returning ops *only* have relaxed variants
> >   without a _relaxed suffix, and can easily be mistaken for being fully
> >   ordered.
> > 
> >   It would be nice to give these a _relaxed() suffix, but this would
> >   result in significant churn throughout the kernel.
> > 
> > * Our naming of conditional and unconditional+test ops is rather
> >   inconsistent, and it can be difficult to derive the name of an
> >   operation, or to identify where an op is conditional or
> >   unconditional+test.
> > 
> >   Some ops are clearly conditional:
> >   - dec_if_positive
> >   - add_unless
> >   - dec_unless_positive
> >   - inc_unless_negative
> > 
> >   Some ops are clearly unconditional+test:
> >   - sub_and_test
> >   - dec_and_test
> >   - inc_and_test
> > 
> >   However, what exactly those test is not obvious. A _test_zero suffix
> >   might be clearer.
> > 
> >   Others could be read ambiguously:
> >   - inc_not_zero	// conditional
> >   - add_negative	// unconditional+test
> > 
> >   It would probably be worth renaming these, e.g. to inc_unless_zero and
> >   add_test_negative.
> > 
> > As a step towards making this more consistent and easier to understand,
> > this patch adds kerneldoc comments for all generated *atomic*_*()
> > functions. These are generated from templates, with some common text
> > shared, making it easy to extend these in future if necessary.
> > 
> > I've tried to make these as consistent and clear as possible, and I've
> > deliberately ensured:
> > 
> > * All ops have their ordering explicitly mentioned in the short and long
> >   description.
> > 
> > * All test ops have "test" in their short description.
> > 
> > * All ops are described as an expression using their usual C operator.
> >   For example:
> > 
> >   andnot: "Atomically updates @v to (@v & ~@i)"
> >   inc:    "Atomically updates @v to (@v + 1)"
> > 
> >   Which may be clearer to non-naative English speakers, and allows all
> >   the operations to be described in the same style.
> > 
> > * All conditional ops have their condition described as an expression
> >   using the usual C operators. For example:
> > 
> >   add_unless: "If (@v != @u), atomically updates @v to (@v + @i)"
> >   cmpxchg:    "If (@v == @old), atomically updates @v to @new"
> > 
> >   Which may be clearer to non-naative English speakers, and allows all
> >   the operations to be described in the same style.
> > 
> > * All bitwise ops (and,andnot,or,xor) explicitly mention that they are
> >   bitwise in their short description, so that they are not mistaken for
> >   performing their logical equivalents.
> > 
> > * The noinstr safety of each op is explicitly described, with a
> >   description of whether or not to use the raw_ form of the op.
> > 
> > There should be no functional change as a result of this patch.
> > 
> > Reported-by: Paul E. McKenney <paulmck@...nel.org>
> > Signed-off-by: Mark Rutland <mark.rutland@....com>
> > Reviewed-by: Kees Cook <keescook@...omium.org>
> > Cc: Boqun Feng <boqun.feng@...il.com>
> > Cc: Jonathan Corbet <corbet@....net>
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: Will Deacon <will@...nel.org>
> 
> With the dec_if_positive fix:
> 
> Reviewed-by: Paul E. McKenney <paulmck@...nel.org>

Thanks! This is already queued in the tip tree's locking/core branch:

  https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=locking/core

... so I was assuming that the dec_if_positive patch would be picked up atop
that.

Regardless, thanks for checking I hadn't missed anything else here! :)

Mark.

Powered by blists - more mailing lists