diff mbox

[RFC] powerpc: Fix missing barriers in atomic_exchange_and_add_{acq,rel}

Message ID 5474DE4F.8050401@linux.vnet.ibm.com
State Committed
Delegated to: Adhemerval Zanella Netto
Headers

Commit Message

Adhemerval Zanella Netto Nov. 25, 2014, 7:53 p.m. UTC 
  On 25-11-2014 16:09, Adhemerval Zanella wrote:
> On 25-11-2014 13:39, Torvald Riegel wrote:
>> On Tue, 2014-11-25 at 13:07 -0200, Adhemerval Zanella wrote:
>>> Hi Torvald,
>>>
>>> On 21-10-2014 17:54, Torvald Riegel wrote:
>>>> 				      \
>>>>    })
>>>> +#define atomic_exchange_and_add_acq(mem, value) \
>>>> +  ({									      \
>>>> +    __typeof (*(mem)) __result2;					      \
>>>> +    __result2 = atomic_exchange_and_add (mem, value);			      \
>>>> +    atomic_read_barrier ();						      \
>>>> +    __result2;		
>>> Although it is not wrong by using a 'atomic_read_barrier' (lwsync), it adds a more 
>>> expensive synchronization than required (isync).  I would prefer if we use the
>>> already defined __arch_compare_and_exchange_val_[32|64]_[acq|rel] operations on powerpc.
>> That's fine with me.  Do you want to go adapt and commit the patch
>> (given that you can test this easily I guess), or should I?
>>
> I will do it, thanks.
>
This is what I intend to commit. Comments?

--
	* csu/tst-atomic.c (do_test): Add atomic_exchange_and_add_{acq,rel}
	tests.
	* sysdeps/powerpc/bits/atomic.h
	(__arch_atomic_exchange_and_add_32_acq): Add definition.
	(__arch_atomic_exchange_and_add_32_rel): Likewise.
	(atomic_exchange_and_add_acq): Likewise.
	(atomic_exchange_and_add_rel): Likewise.
	* sysdeps/powerpc/powerpc32/bits/atomic.h
	(__arch_atomic_exchange_and_add_64_acq): Add definition.
	(__arch_atomic_exchange_and_add_64_rel): Likewise.
	* sysdeps/powerpc/powerpc64/bits/atomic.h
	(__arch_atomic_exchange_and_add_64_acq): Add definition.
	(__arch_atomic_exchange_and_add_64_rel): Likewise.
--

Comments

Torvald Riegel Nov. 26, 2014, 9 a.m. UTC | #1 
On Tue, 2014-11-25 at 17:53 -0200, Adhemerval Zanella wrote:
> On 25-11-2014 16:09, Adhemerval Zanella wrote:
> > On 25-11-2014 13:39, Torvald Riegel wrote:
> >> On Tue, 2014-11-25 at 13:07 -0200, Adhemerval Zanella wrote:
> >>> Hi Torvald,
> >>>
> >>> On 21-10-2014 17:54, Torvald Riegel wrote:
> >>>> 				      \
> >>>>    })
> >>>> +#define atomic_exchange_and_add_acq(mem, value) \
> >>>> +  ({									      \
> >>>> +    __typeof (*(mem)) __result2;					      \
> >>>> +    __result2 = atomic_exchange_and_add (mem, value);			      \
> >>>> +    atomic_read_barrier ();						      \
> >>>> +    __result2;		
> >>> Although it is not wrong by using a 'atomic_read_barrier' (lwsync), it adds a more 
> >>> expensive synchronization than required (isync).  I would prefer if we use the
> >>> already defined __arch_compare_and_exchange_val_[32|64]_[acq|rel] operations on powerpc.
> >> That's fine with me.  Do you want to go adapt and commit the patch
> >> (given that you can test this easily I guess), or should I?
> >>
> > I will do it, thanks.
> >
> This is what I intend to commit. Comments?
> 
> --
> 	* csu/tst-atomic.c (do_test): Add atomic_exchange_and_add_{acq,rel}
> 	tests.

Thanks for adding the tests.

> 	* sysdeps/powerpc/bits/atomic.h
> 	(__arch_atomic_exchange_and_add_32_acq): Add definition.
> 	(__arch_atomic_exchange_and_add_32_rel): Likewise.
> 	(atomic_exchange_and_add_acq): Likewise.
> 	(atomic_exchange_and_add_rel): Likewise.
> 	* sysdeps/powerpc/powerpc32/bits/atomic.h
> 	(__arch_atomic_exchange_and_add_64_acq): Add definition.
> 	(__arch_atomic_exchange_and_add_64_rel): Likewise.
> 	* sysdeps/powerpc/powerpc64/bits/atomic.h
> 	(__arch_atomic_exchange_and_add_64_acq): Add definition.
> 	(__arch_atomic_exchange_and_add_64_rel): Likewise.

Looks good to me.
diff mbox

Patch

diff --git a/csu/tst-atomic.c b/csu/tst-atomic.c
index c6e786d..5ab651e 100644
--- a/csu/tst-atomic.c
+++ b/csu/tst-atomic.c
@@ -113,6 +113,22 @@  do_test (void)
       ret = 1;
     }
 
+  mem = 2;
+  if (atomic_exchange_and_add_acq (&mem, 11) != 2
+      || mem != 13)
+    {
+      puts ("atomic_exchange_and_add test failed");
+      ret = 1;
+    }
+
+  mem = 2;
+  if (atomic_exchange_and_add_rel (&mem, 11) != 2
+      || mem != 13)
+    {
+      puts ("atomic_exchange_and_add test failed");
+      ret = 1;
+    }
+
   mem = -21;
   atomic_add (&mem, 22);
   if (mem != 1)
diff --git a/sysdeps/powerpc/bits/atomic.h b/sysdeps/powerpc/bits/atomic.h
index f312676..b05b0f7 100644
--- a/sysdeps/powerpc/bits/atomic.h
+++ b/sysdeps/powerpc/bits/atomic.h
@@ -152,6 +152,34 @@  typedef uintmax_t uatomic_max_t;
     __val;								      \
   })
 
+#define __arch_atomic_exchange_and_add_32_acq(mem, value) \
+  ({									      \
+    __typeof (*mem) __val, __tmp;					      \
+    __asm __volatile ("1:	lwarx	%0,0,%3" MUTEX_HINT_ACQ "\n"	      \
+		      "		add	%1,%0,%4\n"			      \
+		      "		stwcx.	%1,0,%3\n"			      \
+		      "		bne-	1b\n"				      \
+		      __ARCH_ACQ_INSTR					      \
+		      : "=&b" (__val), "=&r" (__tmp), "=m" (*mem)	      \
+		      : "b" (mem), "r" (value), "m" (*mem)		      \
+		      : "cr0", "memory");				      \
+    __val;								      \
+  })
+
+#define __arch_atomic_exchange_and_add_32_rel(mem, value) \
+  ({									      \
+    __typeof (*mem) __val, __tmp;					      \
+    __asm __volatile (__ARCH_REL_INSTR "\n"				      \
+		      "1:	lwarx	%0,0,%3" MUTEX_HINT_REL "\n"	      \
+		      "		add	%1,%0,%4\n"			      \
+		      "		stwcx.	%1,0,%3\n"			      \
+		      "		bne-	1b"				      \
+		      : "=&b" (__val), "=&r" (__tmp), "=m" (*mem)	      \
+		      : "b" (mem), "r" (value), "m" (*mem)		      \
+		      : "cr0", "memory");				      \
+    __val;								      \
+  })
+
 #define __arch_atomic_increment_val_32(mem) \
   ({									      \
     __typeof (*(mem)) __val;						      \
@@ -252,6 +280,28 @@  typedef uintmax_t uatomic_max_t;
        abort ();							      \
     __result;								      \
   })
+#define atomic_exchange_and_add_acq(mem, value) \
+  ({									      \
+    __typeof (*(mem)) __result;						      \
+    if (sizeof (*mem) == 4)						      \
+      __result = __arch_atomic_exchange_and_add_32_acq (mem, value);	      \
+    else if (sizeof (*mem) == 8)					      \
+      __result = __arch_atomic_exchange_and_add_64_acq (mem, value);	      \
+    else 								      \
+       abort ();							      \
+    __result;								      \
+  })
+#define atomic_exchange_and_add_rel(mem, value) \
+  ({									      \
+    __typeof (*(mem)) __result;						      \
+    if (sizeof (*mem) == 4)						      \
+      __result = __arch_atomic_exchange_and_add_32_rel (mem, value);	      \
+    else if (sizeof (*mem) == 8)					      \
+      __result = __arch_atomic_exchange_and_add_64_rel (mem, value);	      \
+    else 								      \
+       abort ();							      \
+    __result;								      \
+  })
 
 #define atomic_increment_val(mem) \
   ({									      \
diff --git a/sysdeps/powerpc/powerpc32/bits/atomic.h b/sysdeps/powerpc/powerpc32/bits/atomic.h
index 117b5a0..e2a1bf4 100644
--- a/sysdeps/powerpc/powerpc32/bits/atomic.h
+++ b/sysdeps/powerpc/powerpc32/bits/atomic.h
@@ -98,6 +98,12 @@ 
 #define __arch_atomic_exchange_and_add_64(mem, value) \
     ({ abort (); (*mem) = (value); })
 
+#define __arch_atomic_exchange_and_add_64_acq(mem, value) \
+    ({ abort (); (*mem) = (value); })
+
+#define __arch_atomic_exchange_and_add_64_rel(mem, value) \
+    ({ abort (); (*mem) = (value); })
+
 #define __arch_atomic_increment_val_64(mem) \
     ({ abort (); (*mem)++; })
 
diff --git a/sysdeps/powerpc/powerpc64/bits/atomic.h b/sysdeps/powerpc/powerpc64/bits/atomic.h
index 83b5dfe..46117b0 100644
--- a/sysdeps/powerpc/powerpc64/bits/atomic.h
+++ b/sysdeps/powerpc/powerpc64/bits/atomic.h
@@ -186,6 +186,34 @@ 
       __val;								      \
     })
 
+#define __arch_atomic_exchange_and_add_64_acq(mem, value) \
+    ({									      \
+      __typeof (*mem) __val, __tmp;					      \
+      __asm __volatile ("1:	ldarx	%0,0,%3" MUTEX_HINT_ACQ "\n"	      \
+			"	add	%1,%0,%4\n"			      \
+			"	stdcx.	%1,0,%3\n"			      \
+			"	bne-	1b\n"				      \
+			__ARCH_ACQ_INSTR				      \
+			: "=&b" (__val), "=&r" (__tmp), "=m" (*mem)	      \
+			: "b" (mem), "r" (value), "m" (*mem)		      \
+			: "cr0", "memory");				      \
+      __val;								      \
+    })
+
+#define __arch_atomic_exchange_and_add_64_rel(mem, value) \
+    ({									      \
+      __typeof (*mem) __val, __tmp;					      \
+      __asm __volatile (__ARCH_REL_INSTR "\n"				      \
+			"1:	ldarx	%0,0,%3" MUTEX_HINT_REL "\n"	      \
+			"	add	%1,%0,%4\n"			      \
+			"	stdcx.	%1,0,%3\n"			      \
+			"	bne-	1b"				      \
+			: "=&b" (__val), "=&r" (__tmp), "=m" (*mem)	      \
+			: "b" (mem), "r" (value), "m" (*mem)		      \
+			: "cr0", "memory");				      \
+      __val;								      \
+    })
+
 #define __arch_atomic_increment_val_64(mem) \
     ({									      \
       __typeof (*(mem)) __val;						      \