Patchwork [committed] Use __kernel_cmpxchg for __sync_lock_release

login
register
mail settings
Submitter John David Anglin
Date July 17, 2014, 11:25 p.m.
Message ID <BLU436-SMTP97BE92D7629BBF76B5D07697F40@phx.gbl>
Download mbox | patch
Permalink /patch/371267/
State New
Headers show

Comments

John David Anglin - July 17, 2014, 11:25 p.m.
Because the atomic sync functions in config/pa/linux-atomic.c are not  
lock free, we need to use
__kernel_cmpxchg for the __sync_lock_release.  This was found in  
glibc's pthread_spin_unlock
implementation.

Tested on hppa-unknown-linux-gnu.  Committed to trunk.

Dave
--
John David Anglin	dave.anglin@bell.net
2014-07-17  John David Anglin  <danglin@gcc.gnu.org>

	* config/pa/linux-atomic.c (__sync_lock_release_4): New.
	(SYNC_LOCK_RELEASE): Update to use __kernel_cmpxchg for release.
	Don't use SYNC_LOCK_RELEASE for int type.
Mikael Pettersson - July 18, 2014, 11:28 a.m.
John David Anglin writes:
 > Because the atomic sync functions in config/pa/linux-atomic.c are not  
 > lock free, we need to use
 > __kernel_cmpxchg for the __sync_lock_release.  This was found in  
 > glibc's pthread_spin_unlock
 > implementation.
 > 
 > Tested on hppa-unknown-linux-gnu.  Committed to trunk.

It seems to me that ARM's linux-atomic.c has the same problem.
CC:ing some ARM folks for clarification.

/Mikael

 > 
 > Dave
 > --
 > John David Anglin	dave.anglin@bell.net
 > 
 > 
 > 
 > 
 > ----------------------------------------------------------------------
 > 2014-07-17  John David Anglin  <danglin@gcc.gnu.org>
 > 
 > 	* config/pa/linux-atomic.c (__sync_lock_release_4): New.
 > 	(SYNC_LOCK_RELEASE): Update to use __kernel_cmpxchg for release.
 > 	Don't use SYNC_LOCK_RELEASE for int type.
 > 
 > Index: config/pa/linux-atomic.c
 > ===================================================================
 > --- config/pa/linux-atomic.c	(revision 210671)
 > +++ config/pa/linux-atomic.c	(working copy)
 > @@ -293,13 +293,34 @@
 >  SUBWORD_TEST_AND_SET (unsigned short, 2)
 >  SUBWORD_TEST_AND_SET (unsigned char,  1)
 >  
 > +void HIDDEN
 > +__sync_lock_release_4 (int *ptr)
 > +{
 > +  int failure, oldval;
 > +
 > +  do {
 > +    oldval = *ptr;
 > +    failure = __kernel_cmpxchg (oldval, 0, ptr);
 > +  } while (failure != 0);
 > +}
 > +
 >  #define SYNC_LOCK_RELEASE(TYPE, WIDTH)					\
 >    void HIDDEN								\
 >    __sync_lock_release_##WIDTH (TYPE *ptr)				\
 >    {									\
 > -    *ptr = 0;								\
 > +    int failure;							\
 > +    unsigned int oldval, newval, shift, mask;				\
 > +    int *wordptr = (int *) ((unsigned long) ptr & ~3);			\
 > +									\
 > +    shift = (((unsigned long) ptr & 3) << 3) ^ INVERT_MASK_##WIDTH;	\
 > +    mask = MASK_##WIDTH << shift;					\
 > +									\
 > +    do {								\
 > +      oldval = *wordptr;						\
 > +      newval = oldval & ~mask;						\
 > +      failure = __kernel_cmpxchg (oldval, newval, wordptr);		\
 > +    } while (failure != 0);						\
 >    }
 >  
 > -SYNC_LOCK_RELEASE (int,   4)
 >  SYNC_LOCK_RELEASE (short, 2)
 >  SYNC_LOCK_RELEASE (char,  1)

--
John David Anglin - July 18, 2014, 2:37 p.m.
On 7/18/2014 7:28 AM, Mikael Pettersson wrote:
> John David Anglin writes:
>   > Because the atomic sync functions in config/pa/linux-atomic.c are not
>   > lock free, we need to use
>   > __kernel_cmpxchg for the __sync_lock_release.  This was found in
>   > glibc's pthread_spin_unlock
>   > implementation.
>   >
>   > Tested on hppa-unknown-linux-gnu.  Committed to trunk.
>
> It seems to me that ARM's linux-atomic.c has the same problem.
> CC:ing some ARM folks for clarification.
It might.  However, the issue is very subtle and may be parisc specific.

Carlos O'Donnel and I had a long discussion about it and couldn't come
to a clear understanding as to how the race occurs.  However, without
changing the release used in glibc for the pthread_spin_unlock operation,
the spin lock tests in the kyotocabinet testsuite would consistently fail.

Dave
Ramana Radhakrishnan - July 19, 2014, 8:44 a.m.
On Fri, Jul 18, 2014 at 12:28 PM, Mikael Pettersson
<mikpelinux@gmail.com> wrote:
> John David Anglin writes:
>  > Because the atomic sync functions in config/pa/linux-atomic.c are not
>  > lock free, we need to use
>  > __kernel_cmpxchg for the __sync_lock_release.  This was found in
>  > glibc's pthread_spin_unlock
>  > implementation.
>  >
>  > Tested on hppa-unknown-linux-gnu.  Committed to trunk.
>
> It seems to me that ARM's linux-atomic.c has the same problem.
> CC:ing some ARM folks for clarification.

Prima-facie I'm worried about the 8 byte store case as that's not
guaranteed atomic on all MP implementations (you need LPAE IIRC to
guarantee this).

All other cases look sane to me as the 1,2, 4 byte accesses should be atomic.

Ramana
>
> /Mikael
>
>  >
>  > Dave
>  > --
>  > John David Anglin    dave.anglin@bell.net
>  >
>  >
>  >
>  >
>  > ----------------------------------------------------------------------
>  > 2014-07-17  John David Anglin  <danglin@gcc.gnu.org>
>  >
>  >      * config/pa/linux-atomic.c (__sync_lock_release_4): New.
>  >      (SYNC_LOCK_RELEASE): Update to use __kernel_cmpxchg for release.
>  >      Don't use SYNC_LOCK_RELEASE for int type.
>  >
>  > Index: config/pa/linux-atomic.c
>  > ===================================================================
>  > --- config/pa/linux-atomic.c (revision 210671)
>  > +++ config/pa/linux-atomic.c (working copy)
>  > @@ -293,13 +293,34 @@
>  >  SUBWORD_TEST_AND_SET (unsigned short, 2)
>  >  SUBWORD_TEST_AND_SET (unsigned char,  1)
>  >
>  > +void HIDDEN
>  > +__sync_lock_release_4 (int *ptr)
>  > +{
>  > +  int failure, oldval;
>  > +
>  > +  do {
>  > +    oldval = *ptr;
>  > +    failure = __kernel_cmpxchg (oldval, 0, ptr);
>  > +  } while (failure != 0);
>  > +}
>  > +
>  >  #define SYNC_LOCK_RELEASE(TYPE, WIDTH)                                      \
>  >    void HIDDEN                                                               \
>  >    __sync_lock_release_##WIDTH (TYPE *ptr)                           \
>  >    {                                                                 \
>  > -    *ptr = 0;                                                               \
>  > +    int failure;                                                    \
>  > +    unsigned int oldval, newval, shift, mask;                               \
>  > +    int *wordptr = (int *) ((unsigned long) ptr & ~3);                      \
>  > +                                                                    \
>  > +    shift = (((unsigned long) ptr & 3) << 3) ^ INVERT_MASK_##WIDTH; \
>  > +    mask = MASK_##WIDTH << shift;                                   \
>  > +                                                                    \
>  > +    do {                                                            \
>  > +      oldval = *wordptr;                                            \
>  > +      newval = oldval & ~mask;                                              \
>  > +      failure = __kernel_cmpxchg (oldval, newval, wordptr);         \
>  > +    } while (failure != 0);                                         \
>  >    }
>  >
>  > -SYNC_LOCK_RELEASE (int,   4)
>  >  SYNC_LOCK_RELEASE (short, 2)
>  >  SYNC_LOCK_RELEASE (char,  1)
>
> --

Patch

Index: config/pa/linux-atomic.c
===================================================================
--- config/pa/linux-atomic.c	(revision 210671)
+++ config/pa/linux-atomic.c	(working copy)
@@ -293,13 +293,34 @@ 
 SUBWORD_TEST_AND_SET (unsigned short, 2)
 SUBWORD_TEST_AND_SET (unsigned char,  1)
 
+void HIDDEN
+__sync_lock_release_4 (int *ptr)
+{
+  int failure, oldval;
+
+  do {
+    oldval = *ptr;
+    failure = __kernel_cmpxchg (oldval, 0, ptr);
+  } while (failure != 0);
+}
+
 #define SYNC_LOCK_RELEASE(TYPE, WIDTH)					\
   void HIDDEN								\
   __sync_lock_release_##WIDTH (TYPE *ptr)				\
   {									\
-    *ptr = 0;								\
+    int failure;							\
+    unsigned int oldval, newval, shift, mask;				\
+    int *wordptr = (int *) ((unsigned long) ptr & ~3);			\
+									\
+    shift = (((unsigned long) ptr & 3) << 3) ^ INVERT_MASK_##WIDTH;	\
+    mask = MASK_##WIDTH << shift;					\
+									\
+    do {								\
+      oldval = *wordptr;						\
+      newval = oldval & ~mask;						\
+      failure = __kernel_cmpxchg (oldval, newval, wordptr);		\
+    } while (failure != 0);						\
   }
 
-SYNC_LOCK_RELEASE (int,   4)
 SYNC_LOCK_RELEASE (short, 2)
 SYNC_LOCK_RELEASE (char,  1)