public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.
@ 2016-06-12  9:47 Lei Xu
  2016-06-13 10:37 ` Torvald Riegel
  2016-06-13 14:19 ` Tulio Magno Quites Machado Filho
  0 siblings, 2 replies; 10+ messages in thread
From: Lei Xu @ 2016-06-12  9:47 UTC (permalink / raw)
  To: carlos, sjmunroe, libc-alpha; +Cc: scott.wood, Lei Xu

In current code, atomic_compare_and_exchange_*_rel is defined as
atomic_compare_and_exchange_*_acq, however this is wrong on power arch,
especially __arch_*_rel has been defined on power arch.
This has caused segmentation fault issue
when doing pthread_mutex_lock/unlock operations on PowerPC E6500.

Considering generic code should never be assuming that an acquire barrier
can be used in place of a release barrier(If it's OK for a particular arch
let the arch define them as being the same), fix this issue as follows:

For atomic_compare_and_exchange_val_rel:
If arch defines __arch_compare_and_exchange_val_32_rel,
defines to __arch_compare_and_exchange_val_*_rel;
otherwise defines to atomic_compare_and_exchange_val_acq.
For atomic_compare_and_exchange_bool_rel:
If arch defines __arch_compare_and_exchange_bool_32_rel,
defines to __arch_compare_and_exchange_bool_*_rel,
otherwise defines to atomic_compare_and_exchange_val_rel.
For catomic_compare_and_exchange_bool_rel:
If defines atomic_compare_and_exchange_bool_rel,
defines to atomic_compare_and_exchange_bool_rel,
otherwise defines to catomic_compare_and_exchange_val_rel.

Signed-off-by: Lei Xu <lei.xu@nxp.com>
---
 include/atomic.h |   38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/include/atomic.h b/include/atomic.h
index 5e8bfff..effb5af 100644
--- a/include/atomic.h
+++ b/include/atomic.h
@@ -106,6 +106,18 @@
 #endif
 
 
+#ifndef atomic_compare_and_exchange_val_rel
+# ifdef __arch_compare_and_exchange_val_32_rel
+#  define atomic_compare_and_exchange_val_rel(mem, newval, oldval) \
+	__atomic_val_bysize(__arch_compare_and_exchange_val, rel,	      \
+		       mem, newval, oldval)
+# else
+# define atomic_compare_and_exchange_val_rel(mem, newval, oldval)	      \
+	atomic_compare_and_exchange_val_acq(mem, newval, oldval)
+# endif
+#endif
+
+
 #ifndef catomic_compare_and_exchange_val_rel
 # ifndef atomic_compare_and_exchange_val_rel
 #  define catomic_compare_and_exchange_val_rel(mem, newval, oldval)	      \
@@ -117,12 +129,6 @@
 #endif
 
 
-#ifndef atomic_compare_and_exchange_val_rel
-# define atomic_compare_and_exchange_val_rel(mem, newval, oldval)	      \
-  atomic_compare_and_exchange_val_acq (mem, newval, oldval)
-#endif
-
-
 /* Atomically store NEWVAL in *MEM if *MEM is equal to OLDVAL.
    Return zero if *MEM was changed or non-zero if no exchange happened.  */
 #ifndef atomic_compare_and_exchange_bool_acq
@@ -159,10 +165,22 @@
 #endif
 
 
+#ifndef atomic_compare_and_exchange_bool_rel
+# ifdef __arch_compare_and_exchange_bool_32_rel
+#  define atomic_compare_and_exchange_bool_rel(mem, newval, oldval) \
+	__atomic_bool_bysize(__arch_compare_and_exchange_bool, rel,	      \
+				mem, newval, oldval)
+# else
+# define atomic_compare_and_exchange_bool_rel(mem, newval, oldval) \
+	atomic_compare_and_exchange_val_rel(mem, newval, oldval)
+# endif
+#endif
+
+
 #ifndef catomic_compare_and_exchange_bool_rel
 # ifndef atomic_compare_and_exchange_bool_rel
 #  define catomic_compare_and_exchange_bool_rel(mem, newval, oldval)	      \
-  catomic_compare_and_exchange_bool_acq (mem, newval, oldval)
+	catomic_compare_and_exchange_val_rel(mem, newval, oldval)
 # else
 #  define catomic_compare_and_exchange_bool_rel(mem, newval, oldval)	      \
   atomic_compare_and_exchange_bool_rel (mem, newval, oldval)
@@ -170,12 +188,6 @@
 #endif
 
 
-#ifndef atomic_compare_and_exchange_bool_rel
-# define atomic_compare_and_exchange_bool_rel(mem, newval, oldval) \
-  atomic_compare_and_exchange_bool_acq (mem, newval, oldval)
-#endif
-
-
 /* Store NEWVALUE in *MEM and return the old value.  */
 #ifndef atomic_exchange_acq
 # define atomic_exchange_acq(mem, newvalue) \
-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.
  2016-06-12  9:47 [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions Lei Xu
@ 2016-06-13 10:37 ` Torvald Riegel
  2016-06-14  4:27   ` Lei Xu
  2016-06-13 14:19 ` Tulio Magno Quites Machado Filho
  1 sibling, 1 reply; 10+ messages in thread
From: Torvald Riegel @ 2016-06-13 10:37 UTC (permalink / raw)
  To: Lei Xu; +Cc: carlos, sjmunroe, libc-alpha, scott.wood

On Sun, 2016-06-12 at 17:30 +0800, Lei Xu wrote:
> In current code, atomic_compare_and_exchange_*_rel is defined as
> atomic_compare_and_exchange_*_acq, however this is wrong on power arch,
> especially __arch_*_rel has been defined on power arch.
> This has caused segmentation fault issue
> when doing pthread_mutex_lock/unlock operations on PowerPC E6500.

I agree that on some archs, acquire and release memory order are not the
same.
However, ./sysdeps/powerpc/atomic-machine.h does define
atomic_compare_and_exchange_val_rel.  Is
atomic_compare_and_exchange_bool_rel what is missing in the powerpc
definitions?

> Considering generic code should never be assuming that an acquire barrier
> can be used in place of a release barrier(If it's OK for a particular arch
> let the arch define them as being the same), fix this issue as follows:

The generic file include/atomic.h does define *fallbacks* for atomic
operations, assuming that arch-specific files will provide other
definitions if the default fallbacks are not correct on the respective
arch.  This scheme may not be better than not defining fallbacks that
are not correct on all archs, but it is the one we have right now.

Therefore, I think the easiest way is to fix the powerpc side of this.
Note that the atomic operations you are changing here will be phased out
eventually, because we're moving to the newer C11-like atomic operations
(see the last part of include/atomic.h).

Your patch introduces a powerpc-specific change into the generic
include/atomic.h (I don't think all archs define
__arch_compare_and_exchange_val_32_rel, for example), which is not OK.
If you would like to change how include/atomic.h selects defaults, the
patch should change this for all operations, and in a way that is
correct for all archs we have and that does not decrease performance for
any of the archs.

A third option to improve this would be to remove
atomic_compare_and_exchange_bool_rel and replace it with
atomic_compare_exchange_weak_release (ie, the matching C11 atomic
operation).  There are just three callsites that would have to change.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.
  2016-06-12  9:47 [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions Lei Xu
  2016-06-13 10:37 ` Torvald Riegel
@ 2016-06-13 14:19 ` Tulio Magno Quites Machado Filho
  2016-06-13 14:33   ` Steven Munroe
  2016-06-14  5:58   ` Lei Xu
  1 sibling, 2 replies; 10+ messages in thread
From: Tulio Magno Quites Machado Filho @ 2016-06-13 14:19 UTC (permalink / raw)
  To: Lei Xu, libc-alpha; +Cc: scott.wood, carlos, sjmunroe

Hi Lei,

Lei Xu <lei.xu@nxp.com> writes:

> In current code, atomic_compare_and_exchange_*_rel is defined as
> atomic_compare_and_exchange_*_acq, however this is wrong on power arch,
> especially __arch_*_rel has been defined on power arch.
> This has caused segmentation fault issue
> when doing pthread_mutex_lock/unlock operations on PowerPC E6500.

I didn't expect the generic code to affect a PowerPC E6500 build.
It should be using both of the files:
sysdeps/powerpc/atomic-machine.h
sysdeps/powerpc/powerpc32/atomic-machine.h

These files define atomic_compare_and_exchange_val_rel(), which should
prevent from defining the atomic_compare_and_exchange_val_rel() as
atomic_compare_and_exchange_val_acq() in the generic file.

So, there is a chance your build didn't use these files...
How are you configuring your glibc build?

-- 
Tulio Magno

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.
  2016-06-13 14:19 ` Tulio Magno Quites Machado Filho
@ 2016-06-13 14:33   ` Steven Munroe
  2016-06-14  6:00     ` Lei Xu
  2016-06-14  5:58   ` Lei Xu
  1 sibling, 1 reply; 10+ messages in thread
From: Steven Munroe @ 2016-06-13 14:33 UTC (permalink / raw)
  To: Tulio Magno Quites Machado Filho, Lei Xu
  Cc: libc-alpha, scott.wood, carlos, sjmunroe

On Mon, 2016-06-13 at 11:19 -0300, Tulio Magno Quites Machado Filho
wrote:
> Hi Lei,
> 
> Lei Xu <lei.xu@nxp.com> writes:
> 
> > In current code, atomic_compare_and_exchange_*_rel is defined as
> > atomic_compare_and_exchange_*_acq, however this is wrong on power arch,
> > especially __arch_*_rel has been defined on power arch.
> > This has caused segmentation fault issue
> > when doing pthread_mutex_lock/unlock operations on PowerPC E6500.
> 
> I didn't expect the generic code to affect a PowerPC E6500 build.
> It should be using both of the files:
> sysdeps/powerpc/atomic-machine.h
> sysdeps/powerpc/powerpc32/atomic-machine.h
> 
E6500 is a 64-bit part which unfortunately follows the Embedded profile
and so can not blindly use the power* server profile definitions and
overrides.

I suspect he needs a sysdeps/powerpc/powerpc64/e6500/ with overrides and
Implies to pick up common powerpc definitions

> These files define atomic_compare_and_exchange_val_rel(), which should
> prevent from defining the atomic_compare_and_exchange_val_rel() as
> atomic_compare_and_exchange_val_acq() in the generic file.
> 
> So, there is a chance your build didn't use these files...
> How are you configuring your glibc build?
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.
  2016-06-13 10:37 ` Torvald Riegel
@ 2016-06-14  4:27   ` Lei Xu
  2016-06-14 10:16     ` Torvald Riegel
  0 siblings, 1 reply; 10+ messages in thread
From: Lei Xu @ 2016-06-14  4:27 UTC (permalink / raw)
  To: Torvald Riegel; +Cc: carlos, sjmunroe, libc-alpha, Scott Wood

Hello, Torvald

Thank you for your review and suggestions.
Add some comments below~

-----Original Message-----
From: Torvald Riegel [mailto:triegel@redhat.com] 
Sent: Monday, June 13, 2016 6:37 PM
To: Lei Xu
Cc: carlos@systemhalted.org; sjmunroe@us.ibm.com; libc-alpha@sourceware.org; Scott Wood
Subject: Re: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.

On Sun, 2016-06-12 at 17:30 +0800, Lei Xu wrote:
> In current code, atomic_compare_and_exchange_*_rel is defined as 
> atomic_compare_and_exchange_*_acq, however this is wrong on power 
> arch, especially __arch_*_rel has been defined on power arch.
> This has caused segmentation fault issue when doing 
> pthread_mutex_lock/unlock operations on PowerPC E6500.

I agree that on some archs, acquire and release memory order are not the same.
However, ./sysdeps/powerpc/atomic-machine.h does define atomic_compare_and_exchange_val_rel.  Is atomic_compare_and_exchange_bool_rel what is missing in the powerpc definitions?
[XuLei] Yes, you are right. The atomic_compare_and_exchange_bool_rel is missed in powerpc definitions.

> Considering generic code should never be assuming that an acquire 
> barrier can be used in place of a release barrier(If it's OK for a 
> particular arch let the arch define them as being the same), fix this issue as follows:

The generic file include/atomic.h does define *fallbacks* for atomic operations, assuming that arch-specific files will provide other definitions if the default fallbacks are not correct on the respective arch.  This scheme may not be better than not defining fallbacks that are not correct on all archs, but it is the one we have right now.

Therefore, I think the easiest way is to fix the powerpc side of this.
Note that the atomic operations you are changing here will be phased out eventually, because we're moving to the newer C11-like atomic operations (see the last part of include/atomic.h).

Your patch introduces a powerpc-specific change into the generic include/atomic.h (I don't think all archs define __arch_compare_and_exchange_val_32_rel, for example), which is not OK.
If you would like to change how include/atomic.h selects defaults, the patch should change this for all operations, and in a way that is correct for all archs we have and that does not decrease performance for any of the archs.

[XuLei] Yes, I agree with you. At first I plan to fix this to add atomic_compare_and_exchange_bool_rel in powerpc definitions.
Next I considered whether the default definitions in include/atomic.h should be changed:
The atomic_compare_and_exchange_val_acq was defined according to __arch_c_compare_and_exchange_val_32_acq,
Why not atomic_compare_and_exchange_val_rel? If there is no __arch_compare_and_exchange_val_32_rel, it can also use atomic_compare_and_exchange_val_acq.

Meantime we can define atomic_compare_and_exchange_bool_rel to use __arch_compare_and_exchange_bool_32_rel firstly, then atomic_compare_and_exchange_val_rel;
This is similarly with atomic_compare_and_exchange_bool_acq definitions.

These can be applied to generic but not powerpc specific and follows the same rules:
For acq, Firstly try to use __arch_*acq, then use atomic_*val_*acq;
For rel, firstly to use __arch_*rel, then use atomic_*val_*rel, then atomic_*val*acq;

But I did not test this on all of the archs, and just compile it successfully on x86 platform.

So the key point may be that whether it need to change the default generic files, if NOT, I will modify the files in powerpc definitions.
Thanks.
-------------------------[xulei]

A third option to improve this would be to remove atomic_compare_and_exchange_bool_rel and replace it with atomic_compare_exchange_weak_release (ie, the matching C11 atomic operation).  There are just three callsites that would have to change.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.
  2016-06-13 14:19 ` Tulio Magno Quites Machado Filho
  2016-06-13 14:33   ` Steven Munroe
@ 2016-06-14  5:58   ` Lei Xu
  1 sibling, 0 replies; 10+ messages in thread
From: Lei Xu @ 2016-06-14  5:58 UTC (permalink / raw)
  To: Tulio Magno Quites Machado Filho, libc-alpha; +Cc: Scott Wood, carlos, sjmunroe

Hello, Tulio

Thank you and please see my comments below~

-----Original Message-----
From: Tulio Magno Quites Machado Filho [mailto:tuliom@linux.vnet.ibm.com] 
Sent: Monday, June 13, 2016 10:19 PM
To: Lei Xu; libc-alpha@sourceware.org
Cc: Scott Wood; carlos@systemhalted.org; sjmunroe@us.ibm.com
Subject: Re: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.

Hi Lei,

Lei Xu <lei.xu@nxp.com> writes:

> In current code, atomic_compare_and_exchange_*_rel is defined as 
> atomic_compare_and_exchange_*_acq, however this is wrong on power 
> arch, especially __arch_*_rel has been defined on power arch.
> This has caused segmentation fault issue when doing 
> pthread_mutex_lock/unlock operations on PowerPC E6500.

I didn't expect the generic code to affect a PowerPC E6500 build.
It should be using both of the files:
sysdeps/powerpc/atomic-machine.h
sysdeps/powerpc/powerpc32/atomic-machine.h

These files define atomic_compare_and_exchange_val_rel(), which should prevent from defining the atomic_compare_and_exchange_val_rel() as
atomic_compare_and_exchange_val_acq() in the generic file.

[XuLei] It defines atomic_compare_and_exchange_val_rel in sysdeps/powerpc/bits/atomic.h, but not define atomic_compare_and_exchange_bool_rel.
I will paste another solution in next email and would like to get your comments, thanks.

So, there is a chance your build didn't use these files...
How are you configuring your glibc build?

--
Tulio Magno

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.
  2016-06-13 14:33   ` Steven Munroe
@ 2016-06-14  6:00     ` Lei Xu
  2016-06-14 14:35       ` Steven Munroe
  0 siblings, 1 reply; 10+ messages in thread
From: Lei Xu @ 2016-06-14  6:00 UTC (permalink / raw)
  To: munroesj, Tulio Magno Quites Machado Filho
  Cc: libc-alpha, Scott Wood, carlos, sjmunroe

Hello, Steven, Tulio

Thank you and please see my comments below~


-----Original Message-----
From: Steven Munroe [mailto:munroesj@linux.vnet.ibm.com] 
Sent: Monday, June 13, 2016 10:33 PM
To: Tulio Magno Quites Machado Filho; Lei Xu
Cc: libc-alpha@sourceware.org; Scott Wood; carlos@systemhalted.org; sjmunroe@us.ibm.com
Subject: Re: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.

On Mon, 2016-06-13 at 11:19 -0300, Tulio Magno Quites Machado Filho
wrote:
> Hi Lei,
> 
> Lei Xu <lei.xu@nxp.com> writes:
> 
> > In current code, atomic_compare_and_exchange_*_rel is defined as 
> > atomic_compare_and_exchange_*_acq, however this is wrong on power 
> > arch, especially __arch_*_rel has been defined on power arch.
> > This has caused segmentation fault issue when doing 
> > pthread_mutex_lock/unlock operations on PowerPC E6500.
> 
> I didn't expect the generic code to affect a PowerPC E6500 build.
> It should be using both of the files:
> sysdeps/powerpc/atomic-machine.h
> sysdeps/powerpc/powerpc32/atomic-machine.h
> 
E6500 is a 64-bit part which unfortunately follows the Embedded profile and so can not blindly use the power* server profile definitions and overrides.


I suspect he needs a sysdeps/powerpc/powerpc64/e6500/ with overrides and Implies to pick up common powerpc definitions
[XuLei] 
[XuLei] If we would not like to modify the generic files, another solution is to add the definitions directly in "sysdeps/powerpc/bits/atomic.h" file:
I am very happy to have all your comments, thanks.

[PATCH] Define "atomic_compare_and_exchange_bool_rel" for powerpc

Define the "atomic_compare_and_exchange_bool_rel" for powerpc,
otherwise it will use "atomic_compare_and_exchange_bool_acq" by default.

---
 sysdeps/powerpc/bits/atomic.h |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sysdeps/powerpc/bits/atomic.h b/sysdeps/powerpc/bits/atomic.h
index d71f64e..d2b91ba 100644
--- a/sysdeps/powerpc/bits/atomic.h
+++ b/sysdeps/powerpc/bits/atomic.h
@@ -80,6 +80,12 @@ typedef uintmax_t uatomic_max_t;
 #define atomic_full_barrier()	__asm ("sync" ::: "memory")
 #define atomic_write_barrier()	__asm ("eieio" ::: "memory")
 
+#ifdef __arch_compare_and_exchange_bool_32_rel
+#define atomic_compare_and_exchange_bool_rel(mem, newval, oldval)	      \
+  __atomic_bool_bysize (__arch_compare_and_exchange_bool,rel,		      \
+		        mem, newval, oldval)
+#endif
+
 #define __arch_compare_and_exchange_val_32_acq(mem, newval, oldval)	      \
   ({									      \
       __typeof (*(mem)) __tmp;						      \

--------------------------------------[Xulei]

> These files define atomic_compare_and_exchange_val_rel(), which should 
> prevent from defining the atomic_compare_and_exchange_val_rel() as
> atomic_compare_and_exchange_val_acq() in the generic file.


> 
> So, there is a chance your build didn't use these files...
> How are you configuring your glibc build?
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.
  2016-06-14  4:27   ` Lei Xu
@ 2016-06-14 10:16     ` Torvald Riegel
  0 siblings, 0 replies; 10+ messages in thread
From: Torvald Riegel @ 2016-06-14 10:16 UTC (permalink / raw)
  To: Lei Xu; +Cc: carlos, sjmunroe, libc-alpha, Scott Wood

On Tue, 2016-06-14 at 04:27 +0000, Lei Xu wrote:
> [XuLei] Yes, I agree with you. At first I plan to fix this to add atomic_compare_and_exchange_bool_rel in powerpc definitions.
> Next I considered whether the default definitions in include/atomic.h should be changed:
> The atomic_compare_and_exchange_val_acq was defined according to __arch_c_compare_and_exchange_val_32_acq,
> Why not atomic_compare_and_exchange_val_rel? If there is no __arch_compare_and_exchange_val_32_rel, it can also use atomic_compare_and_exchange_val_acq.
> 
> Meantime we can define atomic_compare_and_exchange_bool_rel to use __arch_compare_and_exchange_bool_32_rel firstly, then atomic_compare_and_exchange_val_rel;
> This is similarly with atomic_compare_and_exchange_bool_acq definitions.
> 
> These can be applied to generic but not powerpc specific and follows the same rules:
> For acq, Firstly try to use __arch_*acq, then use atomic_*val_*acq;
> For rel, firstly to use __arch_*rel, then use atomic_*val_*rel, then atomic_*val*acq;
> 
> But I did not test this on all of the archs, and just compile it successfully on x86 platform.
> 
> So the key point may be that whether it need to change the default generic files, if NOT, I will modify the files in powerpc definitions.
> Thanks.
> -------------------------[xulei]
> 
> A third option to improve this would be to remove atomic_compare_and_exchange_bool_rel and replace it with atomic_compare_exchange_weak_release (ie, the matching C11 atomic operation).  There are just three callsites that would have to change.

Thinking more about this, I think the easiest way is actually to remove
atomic_compare_and_exchange_bool_rel completely and replace the current
3 calls to it with C11 atomics.  I'll send a patch that does this
shortly.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.
  2016-06-14  6:00     ` Lei Xu
@ 2016-06-14 14:35       ` Steven Munroe
  2016-06-22 10:08         ` Lei Xu
  0 siblings, 1 reply; 10+ messages in thread
From: Steven Munroe @ 2016-06-14 14:35 UTC (permalink / raw)
  To: Lei Xu
  Cc: Tulio Magno Quites Machado Filho, libc-alpha, Scott Wood, carlos,
	sjmunroe

On Tue, 2016-06-14 at 06:00 +0000, Lei Xu wrote:
> Hello, Steven, Tulio
> 
> Thank you and please see my comments below~
> 
> 
> -----Original Message-----
> From: Steven Munroe [mailto:munroesj@linux.vnet.ibm.com] 
> Sent: Monday, June 13, 2016 10:33 PM
> To: Tulio Magno Quites Machado Filho; Lei Xu
> Cc: libc-alpha@sourceware.org; Scott Wood; carlos@systemhalted.org; sjmunroe@us.ibm.com
> Subject: Re: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.
> 
> On Mon, 2016-06-13 at 11:19 -0300, Tulio Magno Quites Machado Filho
> wrote:
> > Hi Lei,
> > 
> > Lei Xu <lei.xu@nxp.com> writes:
> > 
> > > In current code, atomic_compare_and_exchange_*_rel is defined as 
> > > atomic_compare_and_exchange_*_acq, however this is wrong on power 
> > > arch, especially __arch_*_rel has been defined on power arch.
> > > This has caused segmentation fault issue when doing 
> > > pthread_mutex_lock/unlock operations on PowerPC E6500.
> > 
> > I didn't expect the generic code to affect a PowerPC E6500 build.
> > It should be using both of the files:
> > sysdeps/powerpc/atomic-machine.h
> > sysdeps/powerpc/powerpc32/atomic-machine.h
> > 
> E6500 is a 64-bit part which unfortunately follows the Embedded profile and so can not blindly use the power* server profile definitions and overrides.
> 
> 
> I suspect he needs a sysdeps/powerpc/powerpc64/e6500/ with overrides and Implies to pick up common powerpc definitions
> [XuLei] 
> [XuLei] If we would not like to modify the generic files, another solution is to add the definitions directly in "sysdeps/powerpc/bits/atomic.h" file:
> I am very happy to have all your comments, thanks.
> 
Only if the changes are complaint with the PowerISA Server profile. If
it contains any Embedded profile specific instructions it must be
isolated to a Embedded specific path and override header file. ie
sysdeps/powerpc/powerpc64/e6500/

The E6500 part claims to be PowerISAâ„¢ Version 2.06 complaint, but does
not implement the full Server profile (for example decimal and vsx are
omitted) and does include a number of Embedded specific Book II
extensions not implemented for any server processor.

This is why we have --with-cpu configure option and the supporting
directory structure under ./sysdeps/powerpc/


> [PATCH] Define "atomic_compare_and_exchange_bool_rel" for powerpc
> 
> Define the "atomic_compare_and_exchange_bool_rel" for powerpc,
> otherwise it will use "atomic_compare_and_exchange_bool_acq" by default.
> 
> ---
>  sysdeps/powerpc/bits/atomic.h |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/sysdeps/powerpc/bits/atomic.h b/sysdeps/powerpc/bits/atomic.h
> index d71f64e..d2b91ba 100644
> --- a/sysdeps/powerpc/bits/atomic.h
> +++ b/sysdeps/powerpc/bits/atomic.h
> @@ -80,6 +80,12 @@ typedef uintmax_t uatomic_max_t;
>  #define atomic_full_barrier()	__asm ("sync" ::: "memory")
>  #define atomic_write_barrier()	__asm ("eieio" ::: "memory")
>  
> +#ifdef __arch_compare_and_exchange_bool_32_rel
> +#define atomic_compare_and_exchange_bool_rel(mem, newval, oldval)	      \
> +  __atomic_bool_bysize (__arch_compare_and_exchange_bool,rel,		      \
> +		        mem, newval, oldval)
> +#endif
> +
>  #define __arch_compare_and_exchange_val_32_acq(mem, newval, oldval)	      \
>    ({									      \
>        __typeof (*(mem)) __tmp;						      \
> 
> --------------------------------------[Xulei]
> 
> > These files define atomic_compare_and_exchange_val_rel(), which should 
> > prevent from defining the atomic_compare_and_exchange_val_rel() as
> > atomic_compare_and_exchange_val_acq() in the generic file.
> 
> 
> > 
> > So, there is a chance your build didn't use these files...
> > How are you configuring your glibc build?
> > 
> 
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.
  2016-06-14 14:35       ` Steven Munroe
@ 2016-06-22 10:08         ` Lei Xu
  0 siblings, 0 replies; 10+ messages in thread
From: Lei Xu @ 2016-06-22 10:08 UTC (permalink / raw)
  To: munroesj
  Cc: Tulio Magno Quites Machado Filho, libc-alpha, Scott Wood, carlos,
	sjmunroe

Hello, steven

Sorry to reply so late, and please see my comments below, thanks.

Regards
Lei

-----Original Message-----
From: Steven Munroe [mailto:munroesj@linux.vnet.ibm.com] 
Sent: Tuesday, June 14, 2016 10:36 PM
To: Lei Xu
Cc: Tulio Magno Quites Machado Filho; libc-alpha@sourceware.org; Scott Wood; carlos@systemhalted.org; sjmunroe@us.ibm.com
Subject: RE: [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions.

> 
> Only if the changes are complaint with the PowerISA Server profile. If it contains any Embedded profile specific instructions it must be isolated to a Embedded 
> specific path and override header file. ie sysdeps/powerpc/powerpc64/e6500/

> The E6500 part claims to be PowerISA™ Version 2.06 complaint, but does not implement the full Server profile (for example decimal and vsx are
> omitted) and does include a number of Embedded specific Book II extensions not implemented for any server processor.

> This is why we have --with-cpu configure option and the supporting directory structure under ./sysdeps/powerpc/

[XuLei] Understand. While this issue is not related to something specific to E6500.
__arch_compare_and_exchange_bool_32_rel has been defined in sysdeps/powerpc/powerpc64/atomic-machine.h, sysdeps/powerpc/powerpc32/atomic-machine.h;

But atomic_compare_and_exchange_bool_rel was not defined in powerpc, so it will be defined as atomic_compare_and_exchange_bool_acq in include/atomic.h,
Which finally calls the __arch_compare_and_exchange_bool_32_acq function, so this need to be corrected in powerpc specific file,
Normally it could be in sysdeps/powerpc/atomic-machine.h, like atomic_compare_and_exchange_val_rel.

However, this modification is not necessary now because Torvald Riegel had submitted another patch to remove atomic_compare_and_exchange_bool_rel,
In that patch, it will use atomic_compare_and_exchange_val_rel finally(USE_ATOMIC_COMPILER_BUILTINS was defined to 0 in powerpc) 
and could fix this issue too.

Thanks for all your support.

Regards
Lei






^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-06-22 10:08 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-12  9:47 [PATCH] Fix the atomic_compare_and_exchange_*_rel definitions Lei Xu
2016-06-13 10:37 ` Torvald Riegel
2016-06-14  4:27   ` Lei Xu
2016-06-14 10:16     ` Torvald Riegel
2016-06-13 14:19 ` Tulio Magno Quites Machado Filho
2016-06-13 14:33   ` Steven Munroe
2016-06-14  6:00     ` Lei Xu
2016-06-14 14:35       ` Steven Munroe
2016-06-22 10:08         ` Lei Xu
2016-06-14  5:58   ` Lei Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).