Re: [RFC][PATCH 14/31] locking,metag: Implement atomic_fetch_{add,sub,and,or,xor}()

From: James Hogan
Date: Fri Apr 29 2016 - 20:20:44 EST


Hi Peter,

On Fri, Apr 22, 2016 at 11:04:27AM +0200, Peter Zijlstra wrote:
> Implement FETCH-OP atomic primitives, these are very similar to the
> existing OP-RETURN primitives we already have, except they return the
> value of the atomic variable _before_ modification.
>
> This is especially useful for irreversible operations -- such as
> bitops (because it becomes impossible to reconstruct the state prior
> to modification).
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
> arch/metag/include/asm/atomic.h | 2 +
> arch/metag/include/asm/atomic_lnkget.h | 36 +++++++++++++++++++++++++++++----
> arch/metag/include/asm/atomic_lock1.h | 33 ++++++++++++++++++++++++++----
> 3 files changed, 63 insertions(+), 8 deletions(-)
>
> --- a/arch/metag/include/asm/atomic.h
> +++ b/arch/metag/include/asm/atomic.h
> @@ -17,6 +17,8 @@
> #include <asm/atomic_lnkget.h>
> #endif
>
> +#define atomic_fetch_or atomic_fetch_or
> +
> #define atomic_add_negative(a, v) (atomic_add_return((a), (v)) < 0)
>
> #define atomic_dec_return(v) atomic_sub_return(1, (v))
> --- a/arch/metag/include/asm/atomic_lnkget.h
> +++ b/arch/metag/include/asm/atomic_lnkget.h
> @@ -69,16 +69,44 @@ static inline int atomic_##op##_return(i
> return result; \
> }
>
> -#define ATOMIC_OPS(op) ATOMIC_OP(op) ATOMIC_OP_RETURN(op)
> +#define ATOMIC_FETCH_OP(op) \
> +static inline int atomic_fetch_##op(int i, atomic_t *v) \
> +{ \
> + int result, temp; \
> + \
> + smp_mb(); \
> + \
> + asm volatile ( \
> + "1: LNKGETD %1, [%2]\n" \
> + " " #op " %0, %1, %3\n" \

i was hoping never to have to think about meta asm constraints again :-P

and/or/xor are only available in the data units, as determined by %1 in
this case, so the constraint for result shouldn't have "a" in it.

diff --git a/arch/metag/include/asm/atomic_lnkget.h b/arch/metag/include/asm/atomic_lnkget.h
index 50ad05050947..def2c642f053 100644
--- a/arch/metag/include/asm/atomic_lnkget.h
+++ b/arch/metag/include/asm/atomic_lnkget.h
@@ -84,7 +84,7 @@ static inline int atomic_fetch_##op(int i, atomic_t *v) \
" ANDT %0, %0, #HI(0x3f000000)\n" \
" CMPT %0, #HI(0x02000000)\n" \
" BNZ 1b\n" \
- : "=&d" (temp), "=&da" (result) \
+ : "=&d" (temp), "=&d" (result) \
: "da" (&v->counter), "bd" (i) \
: "cc"); \

That also ensures the "bd" constraint for %3 (meaning "an op2 register
where op1 [%1 in this case] is a data unit register and the instruction
supports O2R") is consistent.

So with that change this patch looks good to me:
Acked-by: James Hogan <james.hogan@xxxxxxxxxx>


Note that for the ATOMIC_OP_RETURN() case (add/sub only) either address
or data units can be used (hence the "da" for %1), but then the "bd"
constraint on %3 is wrong as op1 [%1] may not be in data unit (sorry I
didn't spot that at the time). I'll queue a fix, something like below
probably ("br" means "An Op2 register and the instruction supports O2R",
i.e. op1/%1 doesn't have to be a data unit register):

diff --git a/arch/metag/include/asm/atomic_lnkget.h b/arch/metag/include/asm/atomic_lnkget.h
index 50ad05050947..def2c642f053 100644
--- a/arch/metag/include/asm/atomic_lnkget.h
+++ b/arch/metag/include/asm/atomic_lnkget.h
@@ -61,7 +61,7 @@ static inline int atomic_##op##_return(int i, atomic_t *v) \
" CMPT %0, #HI(0x02000000)\n" \
" BNZ 1b\n" \
: "=&d" (temp), "=&da" (result) \
- : "da" (&v->counter), "bd" (i) \
+ : "da" (&v->counter), "br" (i) \
: "cc"); \
\
smp_mb(); \
\ \

Thanks
James

> + " LNKSETD [%2], %0\n" \
> + " DEFR %0, TXSTAT\n" \
> + " ANDT %0, %0, #HI(0x3f000000)\n" \
> + " CMPT %0, #HI(0x02000000)\n" \
> + " BNZ 1b\n" \
> + : "=&d" (temp), "=&da" (result) \
> + : "da" (&v->counter), "bd" (i) \
> + : "cc"); \
> + \
> + smp_mb(); \
> + \
> + return result; \
> +}
> +
> +#define ATOMIC_OPS(op) ATOMIC_OP(op) ATOMIC_OP_RETURN(op) ATOMIC_FETCH_OP(op)
>
> ATOMIC_OPS(add)
> ATOMIC_OPS(sub)
>
> -ATOMIC_OP(and)
> -ATOMIC_OP(or)
> -ATOMIC_OP(xor)
> +#undef ATOMIC_OPS
> +#define ATOMIC_OPS(op) ATOMIC_OP(op) ATOMIC_FETCH_OP(op)
> +
> +ATOMIC_OPS(and)
> +ATOMIC_OPS(or)
> +ATOMIC_OPS(xor)
>
> #undef ATOMIC_OPS
> +#undef ATOMIC_FETCH_OP
> #undef ATOMIC_OP_RETURN
> #undef ATOMIC_OP
>
> --- a/arch/metag/include/asm/atomic_lock1.h
> +++ b/arch/metag/include/asm/atomic_lock1.h
> @@ -64,15 +64,40 @@ static inline int atomic_##op##_return(i
> return result; \
> }
>
> -#define ATOMIC_OPS(op, c_op) ATOMIC_OP(op, c_op) ATOMIC_OP_RETURN(op, c_op)
> +#define ATOMIC_FETCH_OP(op, c_op) \
> +static inline int atomic_fetch_##op(int i, atomic_t *v) \
> +{ \
> + unsigned long result; \
> + unsigned long flags; \
> + \
> + __global_lock1(flags); \
> + result = v->counter; \
> + fence(); \
> + v->counter c_op i; \
> + __global_unlock1(flags); \
> + \
> + return result; \
> +}
> +
> +#define ATOMIC_OPS(op, c_op) \
> + ATOMIC_OP(op, c_op) \
> + ATOMIC_OP_RETURN(op, c_op) \
> + ATOMIC_FETCH_OP(op, c_op)
>
> ATOMIC_OPS(add, +=)
> ATOMIC_OPS(sub, -=)
> -ATOMIC_OP(and, &=)
> -ATOMIC_OP(or, |=)
> -ATOMIC_OP(xor, ^=)
>
> #undef ATOMIC_OPS
> +#define ATOMIC_OPS(op, c_op) \
> + ATOMIC_OP(op, c_op) \
> + ATOMIC_FETCH_OP(op, c_op)
> +
> +ATOMIC_OPS(and, &=)
> +ATOMIC_OPS(or, |=)
> +ATOMIC_OPS(xor, ^=)
> +
> +#undef ATOMIC_OPS
> +#undef ATOMIC_FETCH_OP
> #undef ATOMIC_OP_RETURN
> #undef ATOMIC_OP
>
>
>

Attachment: signature.asc
Description: Digital signature