* [committed] Use __kernel_cmpxchg for __sync_lock_release
@ 2014-07-17 23:50 John David Anglin
2014-07-18 11:40 ` Mikael Pettersson
0 siblings, 1 reply; 4+ messages in thread
From: John David Anglin @ 2014-07-17 23:50 UTC (permalink / raw)
To: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 316 bytes --]
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
[-- Attachment #2: linux-atomic.c.d.txt --]
[-- Type: text/plain, Size: 1426 bytes --]
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)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [committed] Use __kernel_cmpxchg for __sync_lock_release
2014-07-17 23:50 [committed] Use __kernel_cmpxchg for __sync_lock_release John David Anglin
@ 2014-07-18 11:40 ` Mikael Pettersson
2014-07-18 14:46 ` John David Anglin
2014-07-19 9:29 ` Ramana Radhakrishnan
0 siblings, 2 replies; 4+ messages in thread
From: Mikael Pettersson @ 2014-07-18 11:40 UTC (permalink / raw)
To: John David Anglin; +Cc: GCC Patches, ramana.radhakrishnan, richard.earnshaw
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)
--
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [committed] Use __kernel_cmpxchg for __sync_lock_release
2014-07-18 11:40 ` Mikael Pettersson
@ 2014-07-18 14:46 ` John David Anglin
2014-07-19 9:29 ` Ramana Radhakrishnan
1 sibling, 0 replies; 4+ messages in thread
From: John David Anglin @ 2014-07-18 14:46 UTC (permalink / raw)
To: Mikael Pettersson; +Cc: GCC Patches, ramana.radhakrishnan, richard.earnshaw
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
--
John David Anglin dave.anglin@bell.net
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [committed] Use __kernel_cmpxchg for __sync_lock_release
2014-07-18 11:40 ` Mikael Pettersson
2014-07-18 14:46 ` John David Anglin
@ 2014-07-19 9:29 ` Ramana Radhakrishnan
1 sibling, 0 replies; 4+ messages in thread
From: Ramana Radhakrishnan @ 2014-07-19 9:29 UTC (permalink / raw)
To: Mikael Pettersson
Cc: John David Anglin, GCC Patches, Ramana Radhakrishnan, Richard Earnshaw
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)
>
> --
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-07-19 8:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-17 23:50 [committed] Use __kernel_cmpxchg for __sync_lock_release John David Anglin
2014-07-18 11:40 ` Mikael Pettersson
2014-07-18 14:46 ` John David Anglin
2014-07-19 9:29 ` Ramana Radhakrishnan
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).