This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [RFC] [PATCH] powerpc: Fix missing barriers in atomic_exchange_and_add_{acq,rel}
- From: Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>
- To: libc-alpha at sourceware dot org
- Date: Tue, 25 Nov 2014 17:53:51 -0200
- Subject: Re: [RFC] [PATCH] powerpc: Fix missing barriers in atomic_exchange_and_add_{acq,rel}
- Authentication-results: sourceware.org; auth=none
- References: <1413921274 dot 8483 dot 65 dot camel at triegel dot csb> <54749B3D dot 5010806 at linux dot vnet dot ibm dot com> <1416929990 dot 1771 dot 211 dot camel at triegel dot csb> <5474C5D5 dot 5080407 at linux dot vnet dot ibm dot com>
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.
--
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; \