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

[James Hogan]

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