Hi Mark,
Thank you for the nice documentation improvements!
Please see inline comments for minor nits.
On Mon, 22 May 2023 13:24:27 +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)"
The kernel-doc script converts "~@i" into reST source of "~**i**",
where the emphasis of i is not recognized by Sphinx.
For the "@" to work as expected, please say "~(@i)" or "~ @i".
My preference is the former.
> inc: "Atomically updates @v to (@v + 1)"
>
> Which may be clearer to non-naative English speakers, and allows all
non-native
> 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
Ditto.
> 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@xxxxxxxxxx>
> Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Boqun Feng <boqun.feng@xxxxxxxxx>
FWIW,
Reviewed-by: Akira Yokosawa <akiyks@xxxxxxxxx>
Thanks, Akira
> ---
> include/linux/atomic/atomic-arch-fallback.h | 1848 +++++++++++-
> include/linux/atomic/atomic-instrumented.h | 2771 +++++++++++++++++-
> include/linux/atomic/atomic-long.h | 925 +++++-
> scripts/atomic/atomic-tbl.sh | 112 +-
> scripts/atomic/gen-atomic-fallback.sh | 2 +
> scripts/atomic/gen-atomic-instrumented.sh | 2 +
> scripts/atomic/gen-atomic-long.sh | 2 +
> scripts/atomic/kerneldoc/add | 13 +
> scripts/atomic/kerneldoc/add_negative | 13 +
> scripts/atomic/kerneldoc/add_unless | 18 +
> scripts/atomic/kerneldoc/and | 13 +
> scripts/atomic/kerneldoc/andnot | 13 +
> scripts/atomic/kerneldoc/cmpxchg | 14 +
> scripts/atomic/kerneldoc/dec | 12 +
> scripts/atomic/kerneldoc/dec_and_test | 12 +
> scripts/atomic/kerneldoc/dec_if_positive | 12 +
> scripts/atomic/kerneldoc/dec_unless_positive | 12 +
> scripts/atomic/kerneldoc/inc | 12 +
> scripts/atomic/kerneldoc/inc_and_test | 12 +
> scripts/atomic/kerneldoc/inc_not_zero | 12 +
> scripts/atomic/kerneldoc/inc_unless_negative | 12 +
> scripts/atomic/kerneldoc/or | 13 +
> scripts/atomic/kerneldoc/read | 12 +
> scripts/atomic/kerneldoc/set | 13 +
> scripts/atomic/kerneldoc/sub | 13 +
> scripts/atomic/kerneldoc/sub_and_test | 13 +
> scripts/atomic/kerneldoc/try_cmpxchg | 15 +
> scripts/atomic/kerneldoc/xchg | 13 +
> scripts/atomic/kerneldoc/xor | 13 +
> 29 files changed, 5940 insertions(+), 7 deletions(-)
> create mode 100644 scripts/atomic/kerneldoc/add
> create mode 100644 scripts/atomic/kerneldoc/add_negative
> create mode 100644 scripts/atomic/kerneldoc/add_unless
> create mode 100644 scripts/atomic/kerneldoc/and
> create mode 100644 scripts/atomic/kerneldoc/andnot
> create mode 100644 scripts/atomic/kerneldoc/cmpxchg
> create mode 100644 scripts/atomic/kerneldoc/dec
> create mode 100644 scripts/atomic/kerneldoc/dec_and_test
> create mode 100644 scripts/atomic/kerneldoc/dec_if_positive
> create mode 100644 scripts/atomic/kerneldoc/dec_unless_positive
> create mode 100644 scripts/atomic/kerneldoc/inc
> create mode 100644 scripts/atomic/kerneldoc/inc_and_test
> create mode 100644 scripts/atomic/kerneldoc/inc_not_zero
> create mode 100644 scripts/atomic/kerneldoc/inc_unless_negative
> create mode 100644 scripts/atomic/kerneldoc/or
> create mode 100644 scripts/atomic/kerneldoc/read
> create mode 100644 scripts/atomic/kerneldoc/set
> create mode 100644 scripts/atomic/kerneldoc/sub
> create mode 100644 scripts/atomic/kerneldoc/sub_and_test
> create mode 100644 scripts/atomic/kerneldoc/try_cmpxchg
> create mode 100644 scripts/atomic/kerneldoc/xchg
> create mode 100644 scripts/atomic/kerneldoc/xor
>
[...]
