| Message ID | BLU436-SMTP97BE92D7629BBF76B5D07697F40@phx.gbl |
|---|---|
| State | New |
| Headers | show |
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) --
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
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) > > --
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)