From patchwork Tue Nov 25 19:53:51 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 3911 X-Patchwork-Delegate: azanella@linux.vnet.ibm.com Received: (qmail 15052 invoked by alias); 25 Nov 2014 19:54:02 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 15037 invoked by uid 89); 25 Nov 2014 19:54:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: e24smtp04.br.ibm.com Message-ID: <5474DE4F.8050401@linux.vnet.ibm.com> Date: Tue, 25 Nov 2014 17:53:51 -0200 From: Adhemerval Zanella User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: libc-alpha@sourceware.org Subject: Re: [RFC] [PATCH] powerpc: Fix missing barriers in atomic_exchange_and_add_{acq, rel} References: <1413921274.8483.65.camel@triegel.csb> <54749B3D.5010806@linux.vnet.ibm.com> <1416929990.1771.211.camel@triegel.csb> <5474C5D5.5080407@linux.vnet.ibm.com> In-Reply-To: <5474C5D5.5080407@linux.vnet.ibm.com> X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 14112519-0029-0000-0000-000002212052 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; \