[PATCHv2,05/16] atomics: prepare for atomic64_fetch_add_unless()

Message ID 20180529154346.3168-6-mark.rutland@arm.com
State Superseded
Headers show
Series
  • atomics: API cleanups
Related show

Commit Message

Mark Rutland May 29, 2018, 3:43 p.m.
Currently architecture must implement atomic_fetch_add_unless(), with
common code providing atomic_add_unless(). Architectures must also
implement atmic64_add_unless() directly, with no corresponding
atomic64_fetch_add_unless().

This divergenece is unfortunate, and means that the APIs for atomic_t,
atomic64_t, and atomic_long_t differ.

In preparation for unifying things, with architectures providing
atomic64_fetch_add_unless, this patch adds a generic
atomic64_add_unless() which will use atomic64_fetch_add_unless(). The
instrumented atomics are updated to take this case into account.

There should be no functional change as a result of this patch.

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

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: Vineet Gupta <vgupta@synopsys.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Palmer Dabbelt <palmer@sifive.com>
Cc: Albert Ou <albert@sifive.com>
---
 include/asm-generic/atomic-instrumented.h |  9 +++++++++
 include/linux/atomic.h                    | 16 ++++++++++++++++
 2 files changed, 25 insertions(+)

-- 
2.11.0

Comments

Peter Zijlstra June 5, 2018, 9:26 a.m. | #1
On Tue, May 29, 2018 at 04:43:35PM +0100, Mark Rutland wrote:
>  /**

> + * atomic64_add_unless - add unless the number is already a given value

> + * @v: pointer of type atomic_t

> + * @a: the amount to add to v...

> + * @u: ...unless v is equal to u.

> + *

> + * Atomically adds @a to @v, so long as @v was not already @u.

> + * Returns non-zero if @v was not @u, and zero otherwise.


I always get confused by that wording; would something like: "Returns
true if the addition was done" not be more clear?

> + */

> +#ifdef atomic64_fetch_add_unless

> +static inline int atomic64_add_unless(atomic64_t *v, long long a, long long u)


Do we want to make that a "bool' return?

> +{

> +	return atomic64_fetch_add_unless(v, a, u) != u;

> +}

> +#endif
Mark Rutland June 5, 2018, 9:53 a.m. | #2
On Tue, Jun 05, 2018 at 11:26:37AM +0200, Peter Zijlstra wrote:
> On Tue, May 29, 2018 at 04:43:35PM +0100, Mark Rutland wrote:

> >  /**

> > + * atomic64_add_unless - add unless the number is already a given value

> > + * @v: pointer of type atomic_t

> > + * @a: the amount to add to v...

> > + * @u: ...unless v is equal to u.

> > + *

> > + * Atomically adds @a to @v, so long as @v was not already @u.

> > + * Returns non-zero if @v was not @u, and zero otherwise.

> 

> I always get confused by that wording; would something like: "Returns

> true if the addition was done" not be more clear?


Sounds clearer to me; I just stole the wording from the existing
atomic_add_unless().

I guess you'll want similar for the conditional inc/dec ops, e.g.

/**
 * atomic_inc_not_zero - increment unless the number is zero
 * @v: pointer of type atomic_t
 *
 * Atomically increments @v by 1, so long as @v is non-zero.
 * Returns non-zero if @v was non-zero, and zero otherwise.
 */

> > + */

> > +#ifdef atomic64_fetch_add_unless

> > +static inline int atomic64_add_unless(atomic64_t *v, long long a, long long u)

> 

> Do we want to make that a "bool' return?


I think so -- that's what the instrumented wrappers (and x86) do today
anyhow, and what I ended up using for the generated headers.

I'll spin a prep patch cleaning up the existing fallbacks in
<linux/atomic.h>, along with the comment fixup above, then rework the
additions likewise.

Thanks,
Mark.
Michael Ellerman June 5, 2018, 10:54 a.m. | #3
Mark Rutland <mark.rutland@arm.com> writes:
> On Tue, Jun 05, 2018 at 11:26:37AM +0200, Peter Zijlstra wrote:

>> On Tue, May 29, 2018 at 04:43:35PM +0100, Mark Rutland wrote:

>> >  /**

>> > + * atomic64_add_unless - add unless the number is already a given value

>> > + * @v: pointer of type atomic_t

>> > + * @a: the amount to add to v...

>> > + * @u: ...unless v is equal to u.

>> > + *

>> > + * Atomically adds @a to @v, so long as @v was not already @u.

>> > + * Returns non-zero if @v was not @u, and zero otherwise.

>> 

>> I always get confused by that wording; would something like: "Returns

>> true if the addition was done" not be more clear?

>

> Sounds clearer to me; I just stole the wording from the existing

> atomic_add_unless().

>

> I guess you'll want similar for the conditional inc/dec ops, e.g.

>

> /**

>  * atomic_inc_not_zero - increment unless the number is zero

>  * @v: pointer of type atomic_t

>  *

>  * Atomically increments @v by 1, so long as @v is non-zero.

>  * Returns non-zero if @v was non-zero, and zero otherwise.

>  */


If we're bike-shedding .. :)

I think "so long as" is overly wordy and not helpful. It'd be clearer
just as:

  * Atomically increments @v by 1, if @v is non-zero.

cheers
Mark Rutland June 5, 2018, 11:08 a.m. | #4
On Tue, Jun 05, 2018 at 08:54:03PM +1000, Michael Ellerman wrote:
> Mark Rutland <mark.rutland@arm.com> writes:

> > On Tue, Jun 05, 2018 at 11:26:37AM +0200, Peter Zijlstra wrote:

> >> On Tue, May 29, 2018 at 04:43:35PM +0100, Mark Rutland wrote:

> >> >  /**

> >> > + * atomic64_add_unless - add unless the number is already a given value

> >> > + * @v: pointer of type atomic_t

> >> > + * @a: the amount to add to v...

> >> > + * @u: ...unless v is equal to u.

> >> > + *

> >> > + * Atomically adds @a to @v, so long as @v was not already @u.

> >> > + * Returns non-zero if @v was not @u, and zero otherwise.

> >> 

> >> I always get confused by that wording; would something like: "Returns

> >> true if the addition was done" not be more clear?

> >

> > Sounds clearer to me; I just stole the wording from the existing

> > atomic_add_unless().

> >

> > I guess you'll want similar for the conditional inc/dec ops, e.g.

> >

> > /**

> >  * atomic_inc_not_zero - increment unless the number is zero

> >  * @v: pointer of type atomic_t

> >  *

> >  * Atomically increments @v by 1, so long as @v is non-zero.

> >  * Returns non-zero if @v was non-zero, and zero otherwise.

> >  */

> 

> If we're bike-shedding .. :)

> 

> I think "so long as" is overly wordy and not helpful. It'd be clearer

> just as:

> 

>   * Atomically increments @v by 1, if @v is non-zero.


I agree; done.

Mark.
Michael Ellerman June 6, 2018, 8:57 a.m. | #5
Mark Rutland <mark.rutland@arm.com> writes:

> On Tue, Jun 05, 2018 at 08:54:03PM +1000, Michael Ellerman wrote:

>> Mark Rutland <mark.rutland@arm.com> writes:

>> > On Tue, Jun 05, 2018 at 11:26:37AM +0200, Peter Zijlstra wrote:

>> >> On Tue, May 29, 2018 at 04:43:35PM +0100, Mark Rutland wrote:

>> >> >  /**

>> >> > + * atomic64_add_unless - add unless the number is already a given value

>> >> > + * @v: pointer of type atomic_t

>> >> > + * @a: the amount to add to v...

>> >> > + * @u: ...unless v is equal to u.

>> >> > + *

>> >> > + * Atomically adds @a to @v, so long as @v was not already @u.

>> >> > + * Returns non-zero if @v was not @u, and zero otherwise.

>> >> 

>> >> I always get confused by that wording; would something like: "Returns

>> >> true if the addition was done" not be more clear?

>> >

>> > Sounds clearer to me; I just stole the wording from the existing

>> > atomic_add_unless().

>> >

>> > I guess you'll want similar for the conditional inc/dec ops, e.g.

>> >

>> > /**

>> >  * atomic_inc_not_zero - increment unless the number is zero

>> >  * @v: pointer of type atomic_t

>> >  *

>> >  * Atomically increments @v by 1, so long as @v is non-zero.

>> >  * Returns non-zero if @v was non-zero, and zero otherwise.

>> >  */

>> 

>> If we're bike-shedding .. :)

>> 

>> I think "so long as" is overly wordy and not helpful. It'd be clearer

>> just as:

>> 

>>   * Atomically increments @v by 1, if @v is non-zero.

>

> I agree; done.


Thanks.

cheers

Patch

diff --git a/include/asm-generic/atomic-instrumented.h b/include/asm-generic/atomic-instrumented.h
index 6e0818c182e2..e22d7e5f4ce7 100644
--- a/include/asm-generic/atomic-instrumented.h
+++ b/include/asm-generic/atomic-instrumented.h
@@ -93,11 +93,20 @@  static __always_inline int atomic_fetch_add_unless(atomic_t *v, int a, int u)
 }
 #endif
 
+#ifdef arch_atomic64_fetch_add_unless
+#define atomic64_fetch_add_unless atomic64_fetch_add_unless
+static __always_inline int atomic64_fetch_add_unless(atomic64_t *v, s64 a, s64 u)
+{
+	kasan_check_write(v, sizeof(*v));
+	return arch_atomic64_fetch_add_unless(v, a, u);
+}
+#else
 static __always_inline bool atomic64_add_unless(atomic64_t *v, s64 a, s64 u)
 {
 	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_add_unless(v, a, u);
 }
+#endif
 
 static __always_inline void atomic_inc(atomic_t *v)
 {
diff --git a/include/linux/atomic.h b/include/linux/atomic.h
index 1105c0b37f27..8d93209052e1 100644
--- a/include/linux/atomic.h
+++ b/include/linux/atomic.h
@@ -1072,6 +1072,22 @@  static inline int atomic_dec_if_positive(atomic_t *v)
 #endif /* atomic64_try_cmpxchg */
 
 /**
+ * atomic64_add_unless - add unless the number is already a given value
+ * @v: pointer of type atomic_t
+ * @a: the amount to add to v...
+ * @u: ...unless v is equal to u.
+ *
+ * Atomically adds @a to @v, so long as @v was not already @u.
+ * Returns non-zero if @v was not @u, and zero otherwise.
+ */
+#ifdef atomic64_fetch_add_unless
+static inline int atomic64_add_unless(atomic64_t *v, long long a, long long u)
+{
+	return atomic64_fetch_add_unless(v, a, u) != u;
+}
+#endif
+
+/**
  * atomic64_inc_not_zero - increment unless the number is zero
  * @v: pointer of type atomic64_t
  *