Re: [PATCH v2 3/9] atomic: Introduce atomic_{inc,dec,dec_and_test}_overflow()

[Mark Rutland <mark.rutland@arm.com>]

On Fri, Dec 10, 2021 at 05:16:21PM +0100, Peter Zijlstra wrote:
> In order to facilitate architecture support for refcount_t, introduce
> a number of new atomic primitives that have a uaccess style exception
> for overflow.
> 
> Notably:
> 
>   atomic_inc_overflow(v, Label):
> 
> 	increment and goto Label when the old value of v is zero or
> 	negative.
> 
>   atomic_dec_overflow(v, Label):
> 
> 	decrement and goto Label when the new value of v is zero or
> 	negative
> 
>   atomic_dec_and_test_overflow(v, Label):
> 
> 	decrement and return true when the result is zero and goto
> 	Label when the new value of v is negative

Maybe it's worth adding these as comments in the fallback, which we have for a
few existing functions, e.g.

| /**
|  * arch_${atomic}_add_negative - add and test if negative
|  * @i: integer value to add 
|  * @v: pointer of type ${atomic}_t
|  *
|  * Atomically adds @i to @v and returns true
|  * if the result is negative, or false when
|  * result is greater than or equal to zero.
|  */
| static __always_inline bool
| arch_${atomic}_add_negative(${int} i, ${atomic}_t *v) 
| {
|         return arch_${atomic}_add_return(i, v) < 0;
| }

Not a big deal either way.

> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/atomic/atomic-arch-fallback.h    |   64 ++++++++++++++++++++++++
>  include/linux/atomic/atomic-instrumented.h     |   65 ++++++++++++++++++++++++-
>  include/linux/atomic/atomic-long.h             |   32 +++++++++++-
>  scripts/atomic/atomics.tbl                     |    3 +
>  scripts/atomic/fallbacks/dec_and_test_overflow |   12 ++++
>  scripts/atomic/fallbacks/dec_overflow          |    8 +++
>  scripts/atomic/fallbacks/inc_overflow          |    8 +++
>  7 files changed, 189 insertions(+), 3 deletions(-)
> 
> --- a/include/linux/atomic/atomic-arch-fallback.h
> +++ b/include/linux/atomic/atomic-arch-fallback.h
> @@ -1250,6 +1250,37 @@ arch_atomic_dec_if_positive(atomic_t *v)
>  #define arch_atomic_dec_if_positive arch_atomic_dec_if_positive
>  #endif
>  
> +#ifndef arch_atomic_inc_overflow
> +#define arch_atomic_inc_overflow(_v, _label)				\
> +do {									\
> +	int __old = arch_atomic_fetch_inc(_v);			\
> +	if (unlikely(__old <= 0))					\
> +		goto _label;						\
> +} while (0)
> +#endif
> +
> +#ifndef arch_atomic_dec_overflow
> +#define arch_atomic_dec_overflow(_v, _label)				\
> +do {									\
> +	int __new = arch_atomic_dec_return(_v);			\
> +	if (unlikely(__new <= 0))					\
> +		goto _label;						\
> +} while (0)
> +#endif
> +
> +#ifndef arch_atomic_dec_and_test_overflow
> +#define arch_atomic_dec_and_test_overflow(_v, _label)		\
> +({									\
> +	bool __ret = false;						\
> +	int __new = arch_atomic_dec_return(_v);			\
> +	if (unlikely(__new < 0))					\
> +		goto _label;						\
> +	if (unlikely(__new == 0))					\
> +		__ret = true;						\
> +	__ret;								\
> +})
> +#endif

I had wanted to move at least part of this to a function to ensure
single-evaluation and avoid accidental symbol aliasing, but (as we discussed
over IRC) I couldn't find any good way to do so, and given this is sufficiently
specialise I think we should be ok with this as-is. It's certainly no worse
than the existing stuff for xchg/cmpxchg.

With that in mind, these (and the other variants, and the underlying fallback
templates) all look good to me.

With or without the comments as above:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> +
>  #ifdef CONFIG_GENERIC_ATOMIC64
>  #include <asm-generic/atomic64.h>
>  #endif
> @@ -2357,5 +2388,36 @@ arch_atomic64_dec_if_positive(atomic64_t
>  #define arch_atomic64_dec_if_positive arch_atomic64_dec_if_positive
>  #endif
>  
> +#ifndef arch_atomic64_inc_overflow
> +#define arch_atomic64_inc_overflow(_v, _label)				\
> +do {									\
> +	s64 __old = arch_atomic64_fetch_inc(_v);			\
> +	if (unlikely(__old <= 0))					\
> +		goto _label;						\
> +} while (0)
> +#endif
> +
> +#ifndef arch_atomic64_dec_overflow
> +#define arch_atomic64_dec_overflow(_v, _label)				\
> +do {									\
> +	s64 __new = arch_atomic64_dec_return(_v);			\
> +	if (unlikely(__new <= 0))					\
> +		goto _label;						\
> +} while (0)
> +#endif
> +
> +#ifndef arch_atomic64_dec_and_test_overflow
> +#define arch_atomic64_dec_and_test_overflow(_v, _label)		\
> +({									\
> +	bool __ret = false;						\
> +	s64 __new = arch_atomic64_dec_return(_v);			\
> +	if (unlikely(__new < 0))					\
> +		goto _label;						\
> +	if (unlikely(__new == 0))					\
> +		__ret = true;						\
> +	__ret;								\
> +})
> +#endif
> +
>  #endif /* _LINUX_ATOMIC_FALLBACK_H */
> -// cca554917d7ea73d5e3e7397dd70c484cad9b2c4
> +// e4c677b23b3fd5e8dc4bce9d6c055103666cfc4a
> --- a/include/linux/atomic/atomic-instrumented.h
> +++ b/include/linux/atomic/atomic-instrumented.h
> @@ -599,6 +599,27 @@ atomic_dec_if_positive(atomic_t *v)
>  	return arch_atomic_dec_if_positive(v);
>  }
>  
> +#define atomic_inc_overflow(v, L) \
> +({ \
> +	typeof(v) __ai_v = (v); \
> +	instrument_atomic_read_write(__ai_v, sizeof(*__ai_v)); \
> +	arch_atomic_inc_overflow(__ai_v, L); \
> +})
> +
> +#define atomic_dec_overflow(v, L) \
> +({ \
> +	typeof(v) __ai_v = (v); \
> +	instrument_atomic_read_write(__ai_v, sizeof(*__ai_v)); \
> +	arch_atomic_dec_overflow(__ai_v, L); \
> +})
> +
> +#define atomic_dec_and_test_overflow(v, L) \
> +({ \
> +	typeof(v) __ai_v = (v); \
> +	instrument_atomic_read_write(__ai_v, sizeof(*__ai_v)); \
> +	arch_atomic_dec_and_test_overflow(__ai_v, L); \
> +})
> +
>  static __always_inline s64
>  atomic64_read(const atomic64_t *v)
>  {
> @@ -1177,6 +1198,27 @@ atomic64_dec_if_positive(atomic64_t *v)
>  	return arch_atomic64_dec_if_positive(v);
>  }
>  
> +#define atomic64_inc_overflow(v, L) \
> +({ \
> +	typeof(v) __ai_v = (v); \
> +	instrument_atomic_read_write(__ai_v, sizeof(*__ai_v)); \
> +	arch_atomic64_inc_overflow(__ai_v, L); \
> +})
> +
> +#define atomic64_dec_overflow(v, L) \
> +({ \
> +	typeof(v) __ai_v = (v); \
> +	instrument_atomic_read_write(__ai_v, sizeof(*__ai_v)); \
> +	arch_atomic64_dec_overflow(__ai_v, L); \
> +})
> +
> +#define atomic64_dec_and_test_overflow(v, L) \
> +({ \
> +	typeof(v) __ai_v = (v); \
> +	instrument_atomic_read_write(__ai_v, sizeof(*__ai_v)); \
> +	arch_atomic64_dec_and_test_overflow(__ai_v, L); \
> +})
> +
>  static __always_inline long
>  atomic_long_read(const atomic_long_t *v)
>  {
> @@ -1755,6 +1797,27 @@ atomic_long_dec_if_positive(atomic_long_
>  	return arch_atomic_long_dec_if_positive(v);
>  }
>  
> +#define atomic_long_inc_overflow(v, L) \
> +({ \
> +	typeof(v) __ai_v = (v); \
> +	instrument_atomic_read_write(__ai_v, sizeof(*__ai_v)); \
> +	arch_atomic_long_inc_overflow(__ai_v, L); \
> +})
> +
> +#define atomic_long_dec_overflow(v, L) \
> +({ \
> +	typeof(v) __ai_v = (v); \
> +	instrument_atomic_read_write(__ai_v, sizeof(*__ai_v)); \
> +	arch_atomic_long_dec_overflow(__ai_v, L); \
> +})
> +
> +#define atomic_long_dec_and_test_overflow(v, L) \
> +({ \
> +	typeof(v) __ai_v = (v); \
> +	instrument_atomic_read_write(__ai_v, sizeof(*__ai_v)); \
> +	arch_atomic_long_dec_and_test_overflow(__ai_v, L); \
> +})
> +
>  #define xchg(ptr, ...) \
>  ({ \
>  	typeof(ptr) __ai_ptr = (ptr); \
> @@ -1912,4 +1975,4 @@ atomic_long_dec_if_positive(atomic_long_
>  
>  
>  #endif /* _LINUX_ATOMIC_INSTRUMENTED_H */
> -// 66cdf9a0e0a995cba29c61baf018f7ef35974ae5
> +// 702806891ef1d01d76767c55088264ab6a1ef77d
> --- a/include/linux/atomic/atomic-long.h
> +++ b/include/linux/atomic/atomic-long.h
> @@ -515,6 +515,21 @@ arch_atomic_long_dec_if_positive(atomic_
>  	return arch_atomic64_dec_if_positive(v);
>  }
>  
> +#define arch_atomic_long_inc_overflow(v, L) \
> +({ \
> +	arch_atomic64_inc_overflow((v), L) \
> +})
> +
> +#define arch_atomic_long_dec_overflow(v, L) \
> +({ \
> +	arch_atomic64_dec_overflow((v), L) \
> +})
> +
> +#define arch_atomic_long_dec_and_test_overflow(v, L) \
> +({ \
> +	arch_atomic64_dec_and_test_overflow((v), L) \
> +})
> +
>  #else /* CONFIG_64BIT */
>  
>  static __always_inline long
> @@ -1009,6 +1024,21 @@ arch_atomic_long_dec_if_positive(atomic_
>  	return arch_atomic_dec_if_positive(v);
>  }
>  
> +#define arch_atomic_long_inc_overflow(v, L) \
> +({ \
> +	arch_atomic_inc_overflow((v), L) \
> +})
> +
> +#define arch_atomic_long_dec_overflow(v, L) \
> +({ \
> +	arch_atomic_dec_overflow((v), L) \
> +})
> +
> +#define arch_atomic_long_dec_and_test_overflow(v, L) \
> +({ \
> +	arch_atomic_dec_and_test_overflow((v), L) \
> +})
> +
>  #endif /* CONFIG_64BIT */
>  #endif /* _LINUX_ATOMIC_LONG_H */
> -// e8f0e08ff072b74d180eabe2ad001282b38c2c88
> +// 487bc4fea91f23f2a4b42af7d5b49ef9172ae792
> --- a/scripts/atomic/atomics.tbl
> +++ b/scripts/atomic/atomics.tbl
> @@ -44,3 +44,6 @@ inc_not_zero		b	v
>  inc_unless_negative	b	v
>  dec_unless_positive	b	v
>  dec_if_positive		i	v
> +inc_overflow			n	v	L
> +dec_overflow			n	v	L
> +dec_and_test_overflow	m	v	L
> --- /dev/null
> +++ b/scripts/atomic/fallbacks/dec_and_test_overflow
> @@ -0,0 +1,12 @@
> +cat << EOF
> +#define arch_${atomic}_dec_and_test_overflow(_v, _label)		\\
> +({									\\
> +	bool __ret = false;						\\
> +	${int} __new = arch_${atomic}_dec_return(_v);			\\
> +	if (unlikely(__new < 0))					\\
> +		goto _label;						\\
> +	if (unlikely(__new == 0))					\\
> +		__ret = true;						\\
> +	__ret;								\\
> +})
> +EOF
> --- /dev/null
> +++ b/scripts/atomic/fallbacks/dec_overflow
> @@ -0,0 +1,8 @@
> +cat << EOF
> +#define arch_${atomic}_dec_overflow(_v, _label)				\\
> +do {									\\
> +	${int} __new = arch_${atomic}_dec_return(_v);			\\
> +	if (unlikely(__new <= 0))					\\
> +		goto _label;						\\
> +} while (0)
> +EOF
> --- /dev/null
> +++ b/scripts/atomic/fallbacks/inc_overflow
> @@ -0,0 +1,8 @@
> +cat << EOF
> +#define arch_${atomic}_inc_overflow(_v, _label)				\\
> +do {									\\
> +	${int} __old = arch_${atomic}_fetch_inc(_v);			\\
> +	if (unlikely(__old <= 0))					\\
> +		goto _label;						\\
> +} while (0)
> +EOF
> 
>