From ad8110706f381170c9f9975f1cb06010fd3ca381 Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Mon, 5 Jun 2023 08:01:22 +0100 Subject: locking/atomic: scripts: generate kerneldoc comments 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 Signed-off-by: Mark Rutland Signed-off-by: Peter Zijlstra (Intel) Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20230605070124.3741859-26-mark.rutland@arm.com --- scripts/atomic/kerneldoc/dec_if_positive | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 scripts/atomic/kerneldoc/dec_if_positive (limited to 'scripts/atomic/kerneldoc/dec_if_positive') diff --git a/scripts/atomic/kerneldoc/dec_if_positive b/scripts/atomic/kerneldoc/dec_if_positive new file mode 100644 index 000000000000..7c742866fb6b --- /dev/null +++ b/scripts/atomic/kerneldoc/dec_if_positive @@ -0,0 +1,12 @@ +cat < 0), atomically updates @v to (@v - 1) with ${desc_order} ordering. + * + * ${desc_noinstr} + * + * Return: @true if @v was updated, @false otherwise. + */ +EOF -- cgit v1.2.3 From b33eb50a92b0a298fa8a6ac350e741c3ec100f6d Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Thu, 15 Jun 2023 14:27:34 +0100 Subject: locking/atomic: scripts: fix ${atomic}_dec_if_positive() kerneldoc The ${atomic}_dec_if_positive() ops are unlike all the other conditional atomic ops. Rather than returning a boolean success value, these return the value that the atomic variable would be updated to, even when no update is performed. We missed this when adding kerneldoc comments, and the documentation for ${atomic}_dec_if_positive() erroneously states: | Return: @true if @v was updated, @false otherwise. Ideally we'd clean this up by aligning ${atomic}_dec_if_positive() with the usual atomic op conventions: with ${atomic}_fetch_dec_if_positive() for those who care about the value of the varaible, and ${atomic}_dec_if_positive() returning a boolean success value. In the mean time, align the documentation with the current reality. Fixes: ad8110706f381170 ("locking/atomic: scripts: generate kerneldoc comments") Signed-off-by: Mark Rutland Signed-off-by: Peter Zijlstra (Intel) Reviewed-by: Paul E. McKenney Link: https://lore.kernel.org/r/20230615132734.1119765-1-mark.rutland@arm.com --- include/linux/atomic/atomic-arch-fallback.h | 6 +++--- include/linux/atomic/atomic-instrumented.h | 8 ++++---- include/linux/atomic/atomic-long.h | 4 ++-- scripts/atomic/kerneldoc/dec_if_positive | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) (limited to 'scripts/atomic/kerneldoc/dec_if_positive') diff --git a/include/linux/atomic/atomic-arch-fallback.h b/include/linux/atomic/atomic-arch-fallback.h index 8cded57dd7a6..18f5744dfb5d 100644 --- a/include/linux/atomic/atomic-arch-fallback.h +++ b/include/linux/atomic/atomic-arch-fallback.h @@ -2520,7 +2520,7 @@ raw_atomic_dec_unless_positive(atomic_t *v) * * Safe to use in noinstr code; prefer atomic_dec_if_positive() elsewhere. * - * Return: @true if @v was updated, @false otherwise. + * Return: The old value of (@v - 1), regardless of whether @v was updated. */ static __always_inline int raw_atomic_dec_if_positive(atomic_t *v) @@ -4636,7 +4636,7 @@ raw_atomic64_dec_unless_positive(atomic64_t *v) * * Safe to use in noinstr code; prefer atomic64_dec_if_positive() elsewhere. * - * Return: @true if @v was updated, @false otherwise. + * Return: The old value of (@v - 1), regardless of whether @v was updated. */ static __always_inline s64 raw_atomic64_dec_if_positive(atomic64_t *v) @@ -4657,4 +4657,4 @@ raw_atomic64_dec_if_positive(atomic64_t *v) } #endif /* _LINUX_ATOMIC_FALLBACK_H */ -// 3916f02c038baa3f5190d275f68b9211667fcc9d +// 202b45c7db600ce36198eb1f1fc2c2d5268ace2d diff --git a/include/linux/atomic/atomic-instrumented.h b/include/linux/atomic/atomic-instrumented.h index ebfc795f921b..d401b406ef7c 100644 --- a/include/linux/atomic/atomic-instrumented.h +++ b/include/linux/atomic/atomic-instrumented.h @@ -1570,7 +1570,7 @@ atomic_dec_unless_positive(atomic_t *v) * * Unsafe to use in noinstr code; use raw_atomic_dec_if_positive() there. * - * Return: @true if @v was updated, @false otherwise. + * Return: The old value of (@v - 1), regardless of whether @v was updated. */ static __always_inline int atomic_dec_if_positive(atomic_t *v) @@ -3134,7 +3134,7 @@ atomic64_dec_unless_positive(atomic64_t *v) * * Unsafe to use in noinstr code; use raw_atomic64_dec_if_positive() there. * - * Return: @true if @v was updated, @false otherwise. + * Return: The old value of (@v - 1), regardless of whether @v was updated. */ static __always_inline s64 atomic64_dec_if_positive(atomic64_t *v) @@ -4698,7 +4698,7 @@ atomic_long_dec_unless_positive(atomic_long_t *v) * * Unsafe to use in noinstr code; use raw_atomic_long_dec_if_positive() there. * - * Return: @true if @v was updated, @false otherwise. + * Return: The old value of (@v - 1), regardless of whether @v was updated. */ static __always_inline long atomic_long_dec_if_positive(atomic_long_t *v) @@ -5000,4 +5000,4 @@ atomic_long_dec_if_positive(atomic_long_t *v) #endif /* _LINUX_ATOMIC_INSTRUMENTED_H */ -// 06cec02e676a484857aee38b0071a1d846ec9457 +// 1568f875fef72097413caab8339120c065a39aa4 diff --git a/include/linux/atomic/atomic-long.h b/include/linux/atomic/atomic-long.h index f6df2adadf99..c82947170ddc 100644 --- a/include/linux/atomic/atomic-long.h +++ b/include/linux/atomic/atomic-long.h @@ -1782,7 +1782,7 @@ raw_atomic_long_dec_unless_positive(atomic_long_t *v) * * Safe to use in noinstr code; prefer atomic_long_dec_if_positive() elsewhere. * - * Return: @true if @v was updated, @false otherwise. + * Return: The old value of (@v - 1), regardless of whether @v was updated. */ static __always_inline long raw_atomic_long_dec_if_positive(atomic_long_t *v) @@ -1795,4 +1795,4 @@ raw_atomic_long_dec_if_positive(atomic_long_t *v) } #endif /* _LINUX_ATOMIC_LONG_H */ -// 029d2e3a493086671e874a4c2e0e42084be42403 +// 4ef23f98c73cff96d239896175fd26b10b88899e diff --git a/scripts/atomic/kerneldoc/dec_if_positive b/scripts/atomic/kerneldoc/dec_if_positive index 7c742866fb6b..04f1aed3cf83 100644 --- a/scripts/atomic/kerneldoc/dec_if_positive +++ b/scripts/atomic/kerneldoc/dec_if_positive @@ -7,6 +7,6 @@ cat <