| Message ID | b57d3477-a041-7b06-82ac-6d2b6c6bb08c@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 03/14/2017 04:55 PM, Stefan Liebler wrote: > On 02/20/2017 02:51 PM, Torvald Riegel wrote: >> On Mon, 2017-02-20 at 13:15 +0100, Stefan Liebler wrote: >>>>>> A while ago we tried hard to remove all code that would fail silently >>>>>> when a macro had a typo in the name, for example. We still have a >>>>>> few >>>>>> of them left (e.g., in the atomics), but I think this here doesn't >>>>>> warrant an exception. >>>>>> >>>>> Okay. You're right. >>>>> >>>>> In comment of patch 2/2, you've mentioned a header where an >>>>> architecture >>>>> shall define those parameters. >>>>> Thus I've added the file nptl/pthread_spin_parameters.h. >>>> >>>> I think the stub version should be in sysdeps/ but the wiki is missing >>>> this information (default ENOTSUP location): >>>> https://sourceware.org/glibc/wiki/Style_and_Conventions#Proper_sysdeps_Location >>>> >>> In case we need a new header, shall we move it to sysdeps/nptl/ ? >> >> I would guess that this would be the right place for a stub / generic >> variant of such a header, but I'm not quite sure. >> >>>> >>>> I'm still a bit undecided about how to deal with other arch's >>>> pthread_spin_lock.c files that just set SPIN_LOCK_READS_BETWEEN_CMPXCHG >>>> and then include the generic pthread_spin_lock.c. Maybe it's best >>>> if we >>>> just remove them in this patch of yours. >>>> >>> I think the archs currently using the generic implementation have just >>> copied the default value to get rid of the warning "machine-dependent >>> file should define SPIN_LOCK_READS_BETWEEN_CMPXCHG". But this is only an >>> assumption. >>> In general removing those "wrapper"-pthread_spin_lock.c files and using >>> information from a header like the proposed pthread_spin_parameters.h or >>> atomic-machine.h is a good approach. >> >> Okay. So let's do that. >> > Done. The wrapper-pthread_spin_lock.c files in sysdeps subfolders are > removed and the option SPIN_LOCK_READS_BETWEEN_CMPXCHG is not available > anymore. > Instead there is only a loop of plain reads until we observe an not > acquired lock. > See comment in nptl/pthread_spin_lock.c. > >>>> I agree that the there are architecture-specific properties of atomic >>>> operations that have a significant effect on performance. For s390, >>>> you >>>> list the following points: >>>> * atomic exchange (I suppose all atomic read-modify-write ops?) are >>>> implemented through CAS. >>> atomic exchange is implemented by a CAS loop due to lack of an exchange >>> instruction. For exchanging to a "0", s390 can use the load-and-and >>> instruction instead of a CAS loop (See my atomic-machine.h patch; For >>> gcc, we plan to emit such a load-and-and instruction for builtin >>> __atomic_exchange_n in future;). This also saves one register usage >>> compared to the CAS loop. >>> Exchanging to "0" is e.g. used for lll_unlock macro or in >>> malloc_consolidate. >> >> I guess this may then be yet another property atomic-machine.h could >> expose: Whether an atomic fetch-and-and exists and is not implemented >> throough a CAS loop. Something like lll_unlock would then do an >> exchange or fetch-and-and, and if that fails, a CAS loop. >> >>> Starting with z196 zarch CPUs, the following instructions which are used >>> by the appropriate __atomic_fetch_xyz builtins instead of a CAS loop: >>> load-and-add, load-and-and, load-and-exclusive-or, load-and-or. >>> As information: >>> I will update my atomic-machine.h patch to use at least some of the C11 >>> atomic builtins or all depending on gcc version. >> >> Thanks. >> >>>> These properties are specific to the architecture, but they are not >>>> specific to the synchronization code (eg, to spinlocks). Thus, I think >>>> such settings should be in the atomics headers (i.e., in >>>> atomic-machine.h). >>>> This should probably be a separate patch. I would propose the >>>> following >>>> macros (both are bool): >>>> /* Nonzero iff all atomic read-modify-write operations (e.g., atomic >>>> exchange) are implemented using a CAS loop. */ >>>> #define ATOMIC_RMW_USES_CAS 1 >>> Is one macro enough to describe all read-modify-write operations in >>> include/atomic.h? >>> Please also see my comment above. >> >> Yes, it may not be enough (e.g., you say that s390 may have fetch_and >> but not an exchange, but other archs may just have an exchange but no >> custom fetch_and). >> So maybe we should add ATOMIC_EXCHANGE_USES_CAS and >> ATOMIC_FETCH_AND_USES_CAS for now, and add further ones on demand? >> >>>> /* Nonzero iff CAS always assumes it will store, and thus has cache >>>> performance effects similar to a store (e.g., it always puts the >>>> respective cacheline into an exclusive state, even if the comparison >>>> against the expected value fails). */ >>>> ATOMIC_CAS_ALWAYS_PREPARES_FOR_STORE 1 >>>> >>>> I'm not sure whether there are architectures for which the second macro >>>> would not be true. It would be good to investigate this, maybe we >>>> don't >>>> need to add this at all. >>>> >>> We plan that in future gcc will emit e.g. a load-and-test instruction in >>> front of the CAS instruction if the old-value is a constant zero. >> >> That can be useful, but what I outlined would be about a more generic >> case. If you are going to solve this for your arch in gcc, you might >> not need to address this in this patch. >> >>>> >>>> >>>> For the spin lock specifically, we have the following possibilities: >>>> >>>> 1) If we assume that trylock will most likely succeed in practice: >>>> * We just do an exchange. The properties above don't matter. >>>> >>>> 2) If we want to bias towards cases where trylock succeeds, but don't >>>> rule out contention: >>>> * If exchange not a CAS loop, and exchange is faster than CAS, do an >>>> exchange. >>>> * If exchange is a CAS loop, use a weak CAS and not an exchange so we >>>> bail out after the first failed attempt to change the state. >>>> >>>> 3) If we expect contention to be likely: >>>> * If CAS always brings the cache line into an exclusive state, then >>>> load >>>> first. Then do 2). >>>> >>>> Do we have consensus yet on whether we assume 2 or 3 (I don't think 1 >>>> buys us anything over 2)? >>>> >>> Yes. Sounds good. >> >> I read this as stating that you agree that 1) doesn't need to be >> considered. But we still to choose between 2) and 3) :) >> >> I'm not quite sure what to favor because some of the code out there that >> uses trylock just uses it to prevent deadlock (e.g., when trying to >> acquire multiple locks at once), whereas other code uses spin_trylock to >> implement their own back-off and bounded spinning. >> I lean towards assuming that lock acquisitons, including the former use >> case for trylock, are supposed to succeed in the common case. That is, >> I would pick 2), but I have no data to back this up. >> >> Either way, whatever we choose, we should document polish the cases 1-3 >> above and the reasoning behind our choice for one of them, and add it as >> a comment to the spinlock code. >> > > Okay. I've attached an updated patch. It is now using case 2). > This choice applies to pthread_spin_trylock.c and the first attempt to > acquire the lock in pthread_spin_lock.c. > Therefore I've introduced ATOMIC_EXCHANGE_USES_CAS for all architectures > in atomic-machine.h files. There is a check in include/atomic.h which > ensures that it is defined to either 0 or 1. Can you please review the > setting of 0 or 1? > > Bye Stefan Ping
On 21/03/17 15:43, Stefan Liebler wrote: > On 03/14/2017 04:55 PM, Stefan Liebler wrote: >> Okay. I've attached an updated patch. It is now using case 2). >> This choice applies to pthread_spin_trylock.c and the first attempt to >> acquire the lock in pthread_spin_lock.c. >> Therefore I've introduced ATOMIC_EXCHANGE_USES_CAS for all architectures >> in atomic-machine.h files. There is a check in include/atomic.h which >> ensures that it is defined to either 0 or 1. Can you please review the >> setting of 0 or 1? >> >> Bye Stefan > Ping > the aarch64 changes look ok to me (but this is something that ideally would be benchmarked on real hw with interesting workload and i haven't done that because it is non-trivial) on a trivial benchmark it seems to be a bit better than the current code. the performance of the unconteded case can be improved slightly by reverting the unlock change (the release store is stronger than the barrier was, conceptually there is a barrier before and after an armv8 release store to prevent an independent load-acquire to get reordered with it in either direction) power consumption of a contended spin lock on armv8 can be improved using a send-event/wait-event mechanism, but then the atomic_spin_nop needs to be in a loop with an ll/sc pair not with a relaxed load. (i guess we can introduce a target specific spinlock if this turns out to be relevant) (git apply complained about an extra newline at the end of atomic.h)
On 03/22/2017 01:56 PM, Szabolcs Nagy wrote: > On 21/03/17 15:43, Stefan Liebler wrote: >> On 03/14/2017 04:55 PM, Stefan Liebler wrote: >>> Okay. I've attached an updated patch. It is now using case 2). >>> This choice applies to pthread_spin_trylock.c and the first attempt to >>> acquire the lock in pthread_spin_lock.c. >>> Therefore I've introduced ATOMIC_EXCHANGE_USES_CAS for all architectures >>> in atomic-machine.h files. There is a check in include/atomic.h which >>> ensures that it is defined to either 0 or 1. Can you please review the >>> setting of 0 or 1? >>> >>> Bye Stefan >> Ping >> > > the aarch64 changes look ok to me (but this is > something that ideally would be benchmarked on real > hw with interesting workload and i haven't done that > because it is non-trivial) > > on a trivial benchmark it seems to be a bit better > than the current code. > This sounds good. > the performance of the unconteded case can be improved > slightly by reverting the unlock change (the release > store is stronger than the barrier was, conceptually > there is a barrier before and after an armv8 release > store to prevent an independent load-acquire to get > reordered with it in either direction) > Thus you mean something like the following? atomic_thread_fence_release (); atomic_store_relaxed (lock, 0); (Info: I've used scripts/build-many-glibcs.py to get the following objdumps. The sysdeps/powerpc/nptl/pthread_spin_unlock.c is using atomic_store_release, too. For the following powerpc64-linux-gnu objdumps, I've removed the powerpc-spinlock implementation to see the differences) =>aarch64-linux-gnu 0000000000000000 <pthread_spin_unlock>: 0: d5033bbf dmb ish 4: b900001f str wzr, [x0] 8: 52800000 mov w0, #0x0 // #0 c: d65f03c0 ret =>powerpc64-linux-gnu: 0000000000000000 <.pthread_spin_unlock>: 0: 7c 69 1b 78 mr r9,r3 4: 7c 20 04 ac lwsync 8: 39 40 00 00 li r10,0 c: 38 60 00 00 li r3,0 10: 91 49 00 00 stw r10,0(r9) 14: 4e 80 00 20 blr Here is the upstream code as comparison: atomic_full_barrier (); *lock = 0; =>aarch64-linux-gnu 0000000000000000 <pthread_spin_unlock>: 0: aa0003e1 mov x1, x0 4: d5033bbf dmb ish 8: 52800000 mov w0, #0x0 // #0 c: b900003f str wzr, [x1] 10: d65f03c0 ret =>powerpc64-linux-gnu: 0000000000000000 <.pthread_spin_unlock>: 0: 7c 69 1b 78 mr r9,r3 4: 7c 00 04 ac hwsync 8: 39 40 00 00 li r10,0 c: 38 60 00 00 li r3,0 10: 91 49 00 00 stw r10,0(r9) 14: 4e 80 00 20 blr And the code of my latest patch: atomic_store_release (lock, 0); =>aarch64-linux-gnu 0000000000000000 <pthread_spin_unlock>: 0: 889ffc1f stlr wzr, [x0] 4: 52800000 mov w0, #0x0 // #0 8: d65f03c0 ret =>powerpc64-linux-gnu: 0000000000000000 <.pthread_spin_unlock>: 0: 7c 69 1b 78 mr r9,r3 4: 7c 20 04 ac lwsync 8: 39 40 00 00 li r10,0 c: 38 60 00 00 li r3,0 10: 91 49 00 00 stw r10,0(r9) 14: 4e 80 00 20 blr > power consumption of a contended spin lock on armv8 > can be improved using a send-event/wait-event mechanism, > but then the atomic_spin_nop needs to be in a loop with > an ll/sc pair not with a relaxed load. > (i guess we can introduce a target specific spinlock > if this turns out to be relevant) > > (git apply complained about an extra newline at the > end of atomic.h) > Oops. I'll fix it in the next version. Thanks. Stefan
On 23/03/17 16:15, Stefan Liebler wrote: > On 03/22/2017 01:56 PM, Szabolcs Nagy wrote: >> the performance of the unconteded case can be improved >> slightly by reverting the unlock change (the release >> store is stronger than the barrier was, conceptually >> there is a barrier before and after an armv8 release >> store to prevent an independent load-acquire to get >> reordered with it in either direction) >> > Thus you mean something like the following? > atomic_thread_fence_release (); > atomic_store_relaxed (lock, 0); > (Info: I've used scripts/build-many-glibcs.py to get the following objdumps. The > sysdeps/powerpc/nptl/pthread_spin_unlock.c is using atomic_store_release, too. For the following > powerpc64-linux-gnu objdumps, I've removed the powerpc-spinlock implementation to see the differences) > =>aarch64-linux-gnu > 0000000000000000 <pthread_spin_unlock>: > 0: d5033bbf dmb ish > 4: b900001f str wzr, [x0] > 8: 52800000 mov w0, #0x0 // #0 > c: d65f03c0 ret > =>powerpc64-linux-gnu: > 0000000000000000 <.pthread_spin_unlock>: > 0: 7c 69 1b 78 mr r9,r3 > 4: 7c 20 04 ac lwsync > 8: 39 40 00 00 li r10,0 > c: 38 60 00 00 li r3,0 > 10: 91 49 00 00 stw r10,0(r9) > 14: 4e 80 00 20 blr > > Here is the upstream code as comparison: > atomic_full_barrier (); > *lock = 0; > =>aarch64-linux-gnu > 0000000000000000 <pthread_spin_unlock>: > 0: aa0003e1 mov x1, x0 > 4: d5033bbf dmb ish > 8: 52800000 mov w0, #0x0 // #0 > c: b900003f str wzr, [x1] > 10: d65f03c0 ret > =>powerpc64-linux-gnu: > 0000000000000000 <.pthread_spin_unlock>: > 0: 7c 69 1b 78 mr r9,r3 > 4: 7c 00 04 ac hwsync > 8: 39 40 00 00 li r10,0 > c: 38 60 00 00 li r3,0 > 10: 91 49 00 00 stw r10,0(r9) > 14: 4e 80 00 20 blr > i compared this (full barrier) to > And the code of my latest patch: > atomic_store_release (lock, 0); > =>aarch64-linux-gnu > 0000000000000000 <pthread_spin_unlock>: > 0: 889ffc1f stlr wzr, [x0] > 4: 52800000 mov w0, #0x0 // #0 > 8: d65f03c0 ret > =>powerpc64-linux-gnu: > 0000000000000000 <.pthread_spin_unlock>: > 0: 7c 69 1b 78 mr r9,r3 > 4: 7c 20 04 ac lwsync > 8: 39 40 00 00 li r10,0 > c: 38 60 00 00 li r3,0 > 10: 91 49 00 00 stw r10,0(r9) > 14: 4e 80 00 20 blr > to this (release store). but meanwhile i convinced myself that stlr makes more sense architecturally (even though on the particular implementation i tested this on it was slower). so i'd prefer keeping the atomic_store_release.
On 03/21/2017 04:43 PM, Stefan Liebler wrote: > On 03/14/2017 04:55 PM, Stefan Liebler wrote: >> On 02/20/2017 02:51 PM, Torvald Riegel wrote: >>> On Mon, 2017-02-20 at 13:15 +0100, Stefan Liebler wrote: >>>>>>> A while ago we tried hard to remove all code that would fail >>>>>>> silently >>>>>>> when a macro had a typo in the name, for example. We still have a >>>>>>> few >>>>>>> of them left (e.g., in the atomics), but I think this here doesn't >>>>>>> warrant an exception. >>>>>>> >>>>>> Okay. You're right. >>>>>> >>>>>> In comment of patch 2/2, you've mentioned a header where an >>>>>> architecture >>>>>> shall define those parameters. >>>>>> Thus I've added the file nptl/pthread_spin_parameters.h. >>>>> >>>>> I think the stub version should be in sysdeps/ but the wiki is missing >>>>> this information (default ENOTSUP location): >>>>> https://sourceware.org/glibc/wiki/Style_and_Conventions#Proper_sysdeps_Location >>>>> >>>>> >>>> In case we need a new header, shall we move it to sysdeps/nptl/ ? >>> >>> I would guess that this would be the right place for a stub / generic >>> variant of such a header, but I'm not quite sure. >>> >>>>> >>>>> I'm still a bit undecided about how to deal with other arch's >>>>> pthread_spin_lock.c files that just set >>>>> SPIN_LOCK_READS_BETWEEN_CMPXCHG >>>>> and then include the generic pthread_spin_lock.c. Maybe it's best >>>>> if we >>>>> just remove them in this patch of yours. >>>>> >>>> I think the archs currently using the generic implementation have just >>>> copied the default value to get rid of the warning "machine-dependent >>>> file should define SPIN_LOCK_READS_BETWEEN_CMPXCHG". But this is >>>> only an >>>> assumption. >>>> In general removing those "wrapper"-pthread_spin_lock.c files and using >>>> information from a header like the proposed >>>> pthread_spin_parameters.h or >>>> atomic-machine.h is a good approach. >>> >>> Okay. So let's do that. >>> >> Done. The wrapper-pthread_spin_lock.c files in sysdeps subfolders are >> removed and the option SPIN_LOCK_READS_BETWEEN_CMPXCHG is not available >> anymore. >> Instead there is only a loop of plain reads until we observe an not >> acquired lock. >> See comment in nptl/pthread_spin_lock.c. >> >>>>> I agree that the there are architecture-specific properties of atomic >>>>> operations that have a significant effect on performance. For s390, >>>>> you >>>>> list the following points: >>>>> * atomic exchange (I suppose all atomic read-modify-write ops?) are >>>>> implemented through CAS. >>>> atomic exchange is implemented by a CAS loop due to lack of an exchange >>>> instruction. For exchanging to a "0", s390 can use the load-and-and >>>> instruction instead of a CAS loop (See my atomic-machine.h patch; For >>>> gcc, we plan to emit such a load-and-and instruction for builtin >>>> __atomic_exchange_n in future;). This also saves one register usage >>>> compared to the CAS loop. >>>> Exchanging to "0" is e.g. used for lll_unlock macro or in >>>> malloc_consolidate. >>> >>> I guess this may then be yet another property atomic-machine.h could >>> expose: Whether an atomic fetch-and-and exists and is not implemented >>> throough a CAS loop. Something like lll_unlock would then do an >>> exchange or fetch-and-and, and if that fails, a CAS loop. >>> >>>> Starting with z196 zarch CPUs, the following instructions which are >>>> used >>>> by the appropriate __atomic_fetch_xyz builtins instead of a CAS loop: >>>> load-and-add, load-and-and, load-and-exclusive-or, load-and-or. >>>> As information: >>>> I will update my atomic-machine.h patch to use at least some of the C11 >>>> atomic builtins or all depending on gcc version. >>> >>> Thanks. >>> >>>>> These properties are specific to the architecture, but they are not >>>>> specific to the synchronization code (eg, to spinlocks). Thus, I >>>>> think >>>>> such settings should be in the atomics headers (i.e., in >>>>> atomic-machine.h). >>>>> This should probably be a separate patch. I would propose the >>>>> following >>>>> macros (both are bool): >>>>> /* Nonzero iff all atomic read-modify-write operations (e.g., atomic >>>>> exchange) are implemented using a CAS loop. */ >>>>> #define ATOMIC_RMW_USES_CAS 1 >>>> Is one macro enough to describe all read-modify-write operations in >>>> include/atomic.h? >>>> Please also see my comment above. >>> >>> Yes, it may not be enough (e.g., you say that s390 may have fetch_and >>> but not an exchange, but other archs may just have an exchange but no >>> custom fetch_and). >>> So maybe we should add ATOMIC_EXCHANGE_USES_CAS and >>> ATOMIC_FETCH_AND_USES_CAS for now, and add further ones on demand? >>> >>>>> /* Nonzero iff CAS always assumes it will store, and thus has cache >>>>> performance effects similar to a store (e.g., it always puts the >>>>> respective cacheline into an exclusive state, even if the comparison >>>>> against the expected value fails). */ >>>>> ATOMIC_CAS_ALWAYS_PREPARES_FOR_STORE 1 >>>>> >>>>> I'm not sure whether there are architectures for which the second >>>>> macro >>>>> would not be true. It would be good to investigate this, maybe we >>>>> don't >>>>> need to add this at all. >>>>> >>>> We plan that in future gcc will emit e.g. a load-and-test >>>> instruction in >>>> front of the CAS instruction if the old-value is a constant zero. >>> >>> That can be useful, but what I outlined would be about a more generic >>> case. If you are going to solve this for your arch in gcc, you might >>> not need to address this in this patch. >>> >>>>> >>>>> >>>>> For the spin lock specifically, we have the following possibilities: >>>>> >>>>> 1) If we assume that trylock will most likely succeed in practice: >>>>> * We just do an exchange. The properties above don't matter. >>>>> >>>>> 2) If we want to bias towards cases where trylock succeeds, but don't >>>>> rule out contention: >>>>> * If exchange not a CAS loop, and exchange is faster than CAS, do an >>>>> exchange. >>>>> * If exchange is a CAS loop, use a weak CAS and not an exchange so we >>>>> bail out after the first failed attempt to change the state. >>>>> >>>>> 3) If we expect contention to be likely: >>>>> * If CAS always brings the cache line into an exclusive state, then >>>>> load >>>>> first. Then do 2). >>>>> >>>>> Do we have consensus yet on whether we assume 2 or 3 (I don't think 1 >>>>> buys us anything over 2)? >>>>> >>>> Yes. Sounds good. >>> >>> I read this as stating that you agree that 1) doesn't need to be >>> considered. But we still to choose between 2) and 3) :) >>> >>> I'm not quite sure what to favor because some of the code out there that >>> uses trylock just uses it to prevent deadlock (e.g., when trying to >>> acquire multiple locks at once), whereas other code uses spin_trylock to >>> implement their own back-off and bounded spinning. >>> I lean towards assuming that lock acquisitons, including the former use >>> case for trylock, are supposed to succeed in the common case. That is, >>> I would pick 2), but I have no data to back this up. >>> >>> Either way, whatever we choose, we should document polish the cases 1-3 >>> above and the reasoning behind our choice for one of them, and add it as >>> a comment to the spinlock code. >>> >> >> Okay. I've attached an updated patch. It is now using case 2). >> This choice applies to pthread_spin_trylock.c and the first attempt to >> acquire the lock in pthread_spin_lock.c. >> Therefore I've introduced ATOMIC_EXCHANGE_USES_CAS for all architectures >> in atomic-machine.h files. There is a check in include/atomic.h which >> ensures that it is defined to either 0 or 1. Can you please review the >> setting of 0 or 1? >> >> Bye Stefan > Ping > Ping
On 03/14/2017 04:55 PM, Stefan Liebler wrote: > On 02/20/2017 02:51 PM, Torvald Riegel wrote: >> On Mon, 2017-02-20 at 13:15 +0100, Stefan Liebler wrote: >>>>>> A while ago we tried hard to remove all code that would fail silently >>>>>> when a macro had a typo in the name, for example. We still have a >>>>>> few >>>>>> of them left (e.g., in the atomics), but I think this here doesn't >>>>>> warrant an exception. >>>>>> >>>>> Okay. You're right. >>>>> >>>>> In comment of patch 2/2, you've mentioned a header where an >>>>> architecture >>>>> shall define those parameters. >>>>> Thus I've added the file nptl/pthread_spin_parameters.h. >>>> >>>> I think the stub version should be in sysdeps/ but the wiki is missing >>>> this information (default ENOTSUP location): >>>> https://sourceware.org/glibc/wiki/Style_and_Conventions#Proper_sysdeps_Location >>>> >>> In case we need a new header, shall we move it to sysdeps/nptl/ ? >> >> I would guess that this would be the right place for a stub / generic >> variant of such a header, but I'm not quite sure. >> >>>> >>>> I'm still a bit undecided about how to deal with other arch's >>>> pthread_spin_lock.c files that just set SPIN_LOCK_READS_BETWEEN_CMPXCHG >>>> and then include the generic pthread_spin_lock.c. Maybe it's best >>>> if we >>>> just remove them in this patch of yours. >>>> >>> I think the archs currently using the generic implementation have just >>> copied the default value to get rid of the warning "machine-dependent >>> file should define SPIN_LOCK_READS_BETWEEN_CMPXCHG". But this is only an >>> assumption. >>> In general removing those "wrapper"-pthread_spin_lock.c files and using >>> information from a header like the proposed pthread_spin_parameters.h or >>> atomic-machine.h is a good approach. >> >> Okay. So let's do that. >> > Done. The wrapper-pthread_spin_lock.c files in sysdeps subfolders are > removed and the option SPIN_LOCK_READS_BETWEEN_CMPXCHG is not available > anymore. > Instead there is only a loop of plain reads until we observe an not > acquired lock. > See comment in nptl/pthread_spin_lock.c. > >>>> I agree that the there are architecture-specific properties of atomic >>>> operations that have a significant effect on performance. For s390, >>>> you >>>> list the following points: >>>> * atomic exchange (I suppose all atomic read-modify-write ops?) are >>>> implemented through CAS. >>> atomic exchange is implemented by a CAS loop due to lack of an exchange >>> instruction. For exchanging to a "0", s390 can use the load-and-and >>> instruction instead of a CAS loop (See my atomic-machine.h patch; For >>> gcc, we plan to emit such a load-and-and instruction for builtin >>> __atomic_exchange_n in future;). This also saves one register usage >>> compared to the CAS loop. >>> Exchanging to "0" is e.g. used for lll_unlock macro or in >>> malloc_consolidate. >> >> I guess this may then be yet another property atomic-machine.h could >> expose: Whether an atomic fetch-and-and exists and is not implemented >> throough a CAS loop. Something like lll_unlock would then do an >> exchange or fetch-and-and, and if that fails, a CAS loop. >> >>> Starting with z196 zarch CPUs, the following instructions which are used >>> by the appropriate __atomic_fetch_xyz builtins instead of a CAS loop: >>> load-and-add, load-and-and, load-and-exclusive-or, load-and-or. >>> As information: >>> I will update my atomic-machine.h patch to use at least some of the C11 >>> atomic builtins or all depending on gcc version. >> >> Thanks. >> >>>> These properties are specific to the architecture, but they are not >>>> specific to the synchronization code (eg, to spinlocks). Thus, I think >>>> such settings should be in the atomics headers (i.e., in >>>> atomic-machine.h). >>>> This should probably be a separate patch. I would propose the >>>> following >>>> macros (both are bool): >>>> /* Nonzero iff all atomic read-modify-write operations (e.g., atomic >>>> exchange) are implemented using a CAS loop. */ >>>> #define ATOMIC_RMW_USES_CAS 1 >>> Is one macro enough to describe all read-modify-write operations in >>> include/atomic.h? >>> Please also see my comment above. >> >> Yes, it may not be enough (e.g., you say that s390 may have fetch_and >> but not an exchange, but other archs may just have an exchange but no >> custom fetch_and). >> So maybe we should add ATOMIC_EXCHANGE_USES_CAS and >> ATOMIC_FETCH_AND_USES_CAS for now, and add further ones on demand? >> >>>> /* Nonzero iff CAS always assumes it will store, and thus has cache >>>> performance effects similar to a store (e.g., it always puts the >>>> respective cacheline into an exclusive state, even if the comparison >>>> against the expected value fails). */ >>>> ATOMIC_CAS_ALWAYS_PREPARES_FOR_STORE 1 >>>> >>>> I'm not sure whether there are architectures for which the second macro >>>> would not be true. It would be good to investigate this, maybe we >>>> don't >>>> need to add this at all. >>>> >>> We plan that in future gcc will emit e.g. a load-and-test instruction in >>> front of the CAS instruction if the old-value is a constant zero. >> >> That can be useful, but what I outlined would be about a more generic >> case. If you are going to solve this for your arch in gcc, you might >> not need to address this in this patch. >> >>>> >>>> >>>> For the spin lock specifically, we have the following possibilities: >>>> >>>> 1) If we assume that trylock will most likely succeed in practice: >>>> * We just do an exchange. The properties above don't matter. >>>> >>>> 2) If we want to bias towards cases where trylock succeeds, but don't >>>> rule out contention: >>>> * If exchange not a CAS loop, and exchange is faster than CAS, do an >>>> exchange. >>>> * If exchange is a CAS loop, use a weak CAS and not an exchange so we >>>> bail out after the first failed attempt to change the state. >>>> >>>> 3) If we expect contention to be likely: >>>> * If CAS always brings the cache line into an exclusive state, then >>>> load >>>> first. Then do 2). >>>> >>>> Do we have consensus yet on whether we assume 2 or 3 (I don't think 1 >>>> buys us anything over 2)? >>>> >>> Yes. Sounds good. >> >> I read this as stating that you agree that 1) doesn't need to be >> considered. But we still to choose between 2) and 3) :) >> >> I'm not quite sure what to favor because some of the code out there that >> uses trylock just uses it to prevent deadlock (e.g., when trying to >> acquire multiple locks at once), whereas other code uses spin_trylock to >> implement their own back-off and bounded spinning. >> I lean towards assuming that lock acquisitons, including the former use >> case for trylock, are supposed to succeed in the common case. That is, >> I would pick 2), but I have no data to back this up. >> >> Either way, whatever we choose, we should document polish the cases 1-3 >> above and the reasoning behind our choice for one of them, and add it as >> a comment to the spinlock code. >> > > Okay. I've attached an updated patch. It is now using case 2). > This choice applies to pthread_spin_trylock.c and the first attempt to > acquire the lock in pthread_spin_lock.c. > Therefore I've introduced ATOMIC_EXCHANGE_USES_CAS for all architectures > in atomic-machine.h files. There is a check in include/atomic.h which > ensures that it is defined to either 0 or 1. Can you please review the > setting of 0 or 1? > > Bye Stefan I link the post of Torvald Riegel in this spinlock mailthread, so we have all the discussions here. See "Re: [PATCH v3 0/6] Add support for ISO C11 threads.h" (https://www.sourceware.org/ml/libc-alpha/2017-03/msg00615.html): Current DR log: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2109.htm 470: C11's requirements on implementations are weaker than what POSIX requires. Arguably, POSIX should require what C11 requires. We need to check the lowlevellock implementation, in particular on archs that use LL/SC (I'm not aware of an arch with a true CAS instruction that can fail spuriously). Our generic lowlevellock implementation on master still uses the old-style atomics (ie, atomic_compare_and_exchange_bool_acq); if we move to C11 atomics, C11 (with DR 470's proposed corrigendum applied) would require our atomic_compare_exchange_weak_acquire, whereas current POSIX would require the strong variant of the CAS. Thus, what we do is okay in terms of C11. I don't recall right now whether we have an open Austin Group bug about POSIX allowing a spurious failure; I believe we have, or we should, because this is also related to what memory order is required for lock acquisitions (generally, not just trylock). (Stefan, that's why I'm CC'ing you too, this is relevant for pthread_spin_trylock too, but I can't remember right now whether we already discussed this or not.) Thus you mean we have to use a "atomic_compare_exchange_strong_acquire" in pthread_spin_trylock due to POSIX? I've found your "Bug 17428 - lll_trylock barrier semantics when lock was not acquired differ between architectures" (https://sourceware.org/bugzilla/show_bug.cgi?id=17428) and an older post: "Re: [PATCHv2] powerpc: Spinlock optimization and cleanup" (https://sourceware.org/ml/libc-alpha/2015-10/msg00012.html). Bye Stefan
On 03/27/2017 03:08 PM, Stefan Liebler wrote: > On 03/21/2017 04:43 PM, Stefan Liebler wrote: >> On 03/14/2017 04:55 PM, Stefan Liebler wrote: >>> On 02/20/2017 02:51 PM, Torvald Riegel wrote: >>>> On Mon, 2017-02-20 at 13:15 +0100, Stefan Liebler wrote: >>>>>>>> A while ago we tried hard to remove all code that would fail >>>>>>>> silently >>>>>>>> when a macro had a typo in the name, for example. We still have a >>>>>>>> few >>>>>>>> of them left (e.g., in the atomics), but I think this here doesn't >>>>>>>> warrant an exception. >>>>>>>> >>>>>>> Okay. You're right. >>>>>>> >>>>>>> In comment of patch 2/2, you've mentioned a header where an >>>>>>> architecture >>>>>>> shall define those parameters. >>>>>>> Thus I've added the file nptl/pthread_spin_parameters.h. >>>>>> >>>>>> I think the stub version should be in sysdeps/ but the wiki is >>>>>> missing >>>>>> this information (default ENOTSUP location): >>>>>> https://sourceware.org/glibc/wiki/Style_and_Conventions#Proper_sysdeps_Location >>>>>> >>>>>> >>>>>> >>>>> In case we need a new header, shall we move it to sysdeps/nptl/ ? >>>> >>>> I would guess that this would be the right place for a stub / generic >>>> variant of such a header, but I'm not quite sure. >>>> >>>>>> >>>>>> I'm still a bit undecided about how to deal with other arch's >>>>>> pthread_spin_lock.c files that just set >>>>>> SPIN_LOCK_READS_BETWEEN_CMPXCHG >>>>>> and then include the generic pthread_spin_lock.c. Maybe it's best >>>>>> if we >>>>>> just remove them in this patch of yours. >>>>>> >>>>> I think the archs currently using the generic implementation have just >>>>> copied the default value to get rid of the warning "machine-dependent >>>>> file should define SPIN_LOCK_READS_BETWEEN_CMPXCHG". But this is >>>>> only an >>>>> assumption. >>>>> In general removing those "wrapper"-pthread_spin_lock.c files and >>>>> using >>>>> information from a header like the proposed >>>>> pthread_spin_parameters.h or >>>>> atomic-machine.h is a good approach. >>>> >>>> Okay. So let's do that. >>>> >>> Done. The wrapper-pthread_spin_lock.c files in sysdeps subfolders are >>> removed and the option SPIN_LOCK_READS_BETWEEN_CMPXCHG is not available >>> anymore. >>> Instead there is only a loop of plain reads until we observe an not >>> acquired lock. >>> See comment in nptl/pthread_spin_lock.c. >>> >>>>>> I agree that the there are architecture-specific properties of atomic >>>>>> operations that have a significant effect on performance. For s390, >>>>>> you >>>>>> list the following points: >>>>>> * atomic exchange (I suppose all atomic read-modify-write ops?) are >>>>>> implemented through CAS. >>>>> atomic exchange is implemented by a CAS loop due to lack of an >>>>> exchange >>>>> instruction. For exchanging to a "0", s390 can use the load-and-and >>>>> instruction instead of a CAS loop (See my atomic-machine.h patch; For >>>>> gcc, we plan to emit such a load-and-and instruction for builtin >>>>> __atomic_exchange_n in future;). This also saves one register usage >>>>> compared to the CAS loop. >>>>> Exchanging to "0" is e.g. used for lll_unlock macro or in >>>>> malloc_consolidate. >>>> >>>> I guess this may then be yet another property atomic-machine.h could >>>> expose: Whether an atomic fetch-and-and exists and is not implemented >>>> throough a CAS loop. Something like lll_unlock would then do an >>>> exchange or fetch-and-and, and if that fails, a CAS loop. >>>> >>>>> Starting with z196 zarch CPUs, the following instructions which are >>>>> used >>>>> by the appropriate __atomic_fetch_xyz builtins instead of a CAS loop: >>>>> load-and-add, load-and-and, load-and-exclusive-or, load-and-or. >>>>> As information: >>>>> I will update my atomic-machine.h patch to use at least some of the >>>>> C11 >>>>> atomic builtins or all depending on gcc version. >>>> >>>> Thanks. >>>> >>>>>> These properties are specific to the architecture, but they are not >>>>>> specific to the synchronization code (eg, to spinlocks). Thus, I >>>>>> think >>>>>> such settings should be in the atomics headers (i.e., in >>>>>> atomic-machine.h). >>>>>> This should probably be a separate patch. I would propose the >>>>>> following >>>>>> macros (both are bool): >>>>>> /* Nonzero iff all atomic read-modify-write operations (e.g., atomic >>>>>> exchange) are implemented using a CAS loop. */ >>>>>> #define ATOMIC_RMW_USES_CAS 1 >>>>> Is one macro enough to describe all read-modify-write operations in >>>>> include/atomic.h? >>>>> Please also see my comment above. >>>> >>>> Yes, it may not be enough (e.g., you say that s390 may have fetch_and >>>> but not an exchange, but other archs may just have an exchange but no >>>> custom fetch_and). >>>> So maybe we should add ATOMIC_EXCHANGE_USES_CAS and >>>> ATOMIC_FETCH_AND_USES_CAS for now, and add further ones on demand? >>>> >>>>>> /* Nonzero iff CAS always assumes it will store, and thus has cache >>>>>> performance effects similar to a store (e.g., it always puts the >>>>>> respective cacheline into an exclusive state, even if the comparison >>>>>> against the expected value fails). */ >>>>>> ATOMIC_CAS_ALWAYS_PREPARES_FOR_STORE 1 >>>>>> >>>>>> I'm not sure whether there are architectures for which the second >>>>>> macro >>>>>> would not be true. It would be good to investigate this, maybe we >>>>>> don't >>>>>> need to add this at all. >>>>>> >>>>> We plan that in future gcc will emit e.g. a load-and-test >>>>> instruction in >>>>> front of the CAS instruction if the old-value is a constant zero. >>>> >>>> That can be useful, but what I outlined would be about a more generic >>>> case. If you are going to solve this for your arch in gcc, you might >>>> not need to address this in this patch. >>>> >>>>>> >>>>>> >>>>>> For the spin lock specifically, we have the following possibilities: >>>>>> >>>>>> 1) If we assume that trylock will most likely succeed in practice: >>>>>> * We just do an exchange. The properties above don't matter. >>>>>> >>>>>> 2) If we want to bias towards cases where trylock succeeds, but don't >>>>>> rule out contention: >>>>>> * If exchange not a CAS loop, and exchange is faster than CAS, do an >>>>>> exchange. >>>>>> * If exchange is a CAS loop, use a weak CAS and not an exchange so we >>>>>> bail out after the first failed attempt to change the state. >>>>>> >>>>>> 3) If we expect contention to be likely: >>>>>> * If CAS always brings the cache line into an exclusive state, then >>>>>> load >>>>>> first. Then do 2). >>>>>> >>>>>> Do we have consensus yet on whether we assume 2 or 3 (I don't think 1 >>>>>> buys us anything over 2)? >>>>>> >>>>> Yes. Sounds good. >>>> >>>> I read this as stating that you agree that 1) doesn't need to be >>>> considered. But we still to choose between 2) and 3) :) >>>> >>>> I'm not quite sure what to favor because some of the code out there >>>> that >>>> uses trylock just uses it to prevent deadlock (e.g., when trying to >>>> acquire multiple locks at once), whereas other code uses >>>> spin_trylock to >>>> implement their own back-off and bounded spinning. >>>> I lean towards assuming that lock acquisitons, including the former use >>>> case for trylock, are supposed to succeed in the common case. That is, >>>> I would pick 2), but I have no data to back this up. >>>> >>>> Either way, whatever we choose, we should document polish the cases 1-3 >>>> above and the reasoning behind our choice for one of them, and add >>>> it as >>>> a comment to the spinlock code. >>>> >>> >>> Okay. I've attached an updated patch. It is now using case 2). >>> This choice applies to pthread_spin_trylock.c and the first attempt to >>> acquire the lock in pthread_spin_lock.c. >>> Therefore I've introduced ATOMIC_EXCHANGE_USES_CAS for all architectures >>> in atomic-machine.h files. There is a check in include/atomic.h which >>> ensures that it is defined to either 0 or 1. Can you please review the >>> setting of 0 or 1? >>> >>> Bye Stefan >> Ping >> > > Ping > PING
On Wed, 2017-03-22 at 12:56 +0000, Szabolcs Nagy wrote: > On 21/03/17 15:43, Stefan Liebler wrote: > > On 03/14/2017 04:55 PM, Stefan Liebler wrote: > >> Okay. I've attached an updated patch. It is now using case 2). > >> This choice applies to pthread_spin_trylock.c and the first attempt to > >> acquire the lock in pthread_spin_lock.c. > >> Therefore I've introduced ATOMIC_EXCHANGE_USES_CAS for all architectures > >> in atomic-machine.h files. There is a check in include/atomic.h which > >> ensures that it is defined to either 0 or 1. Can you please review the > >> setting of 0 or 1? > >> > >> Bye Stefan > > Ping > > > > the aarch64 changes look ok to me (but this is > something that ideally would be benchmarked on real > hw with interesting workload and i haven't done that > because it is non-trivial) This is something that we need to continue working on. I don't think it's required for this patch. But any further tuning will need some benchmark. I won't have time to work on benchmarks in the foreseeable future I believe; it would be great if you, Stefan, or someone else could continue to work on this. > power consumption of a contended spin lock on armv8 > can be improved using a send-event/wait-event mechanism, > but then the atomic_spin_nop needs to be in a loop with > an ll/sc pair not with a relaxed load. > (i guess we can introduce a target specific spinlock > if this turns out to be relevant) Interesting. I expect the machine maintainers to drive such optimizations in the future; performance differences should be made reproducable using benchmarks contributed to glibc.
On Tue, 2017-03-14 at 16:55 +0100, Stefan Liebler wrote: > +/* This is equal to 1 if atomic_exchange is implemented by a CAS loop > + and 0 if atomic_exchange is implemented with an instruction faster than > + CAS. */ I'd change this to: "ATOMIC_EXCHANGE_USES_CAS is equal to 1 if atomic_exchange operations are implemented based on a CAS loop; otherwise, this is 0 and we assume that the atomic_exchange operations could provide better performance than a CAS loop." > +#if ! defined ATOMIC_EXCHANGE_USES_CAS || ! (ATOMIC_EXCHANGE_USES_CAS == 0 \ > + || ATOMIC_EXCHANGE_USES_CAS == 1) > +# error ATOMIC_EXCHANGE_USES_CAS has to be defined to 0 or 1 > +#endif > + > #endif /* atomic.h */ > + Extra new line. > diff --git a/nptl/pthread_spin_lock.c b/nptl/pthread_spin_lock.c > index 4d03b78..c2fb8e7 100644 > --- a/nptl/pthread_spin_lock.c > > +++ b/nptl/pthread_spin_lock.c > > @@ -19,27 +19,38 @@ > #include <atomic.h> > #include "pthreadP.h" > > -/* A machine-specific version can define SPIN_LOCK_READS_BETWEEN_CMPXCHG > > - to the number of plain reads that it's optimal to spin on between uses > > - of atomic_compare_and_exchange_val_acq. If spinning forever is optimal > > - then use -1. If no plain reads here would ever be optimal, use 0. */ > > -#ifndef SPIN_LOCK_READS_BETWEEN_CMPXCHG > > -# warning machine-dependent file should define SPIN_LOCK_READS_BETWEEN_CMPXCHG > > -# define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000 > > -#endif > > - > > int > pthread_spin_lock (pthread_spinlock_t *lock) > { > + int val = 0; > > + > > /* atomic_exchange usually takes less instructions than > atomic_compare_and_exchange. On the other hand, > atomic_compare_and_exchange potentially generates less bus traffic > when the lock is locked. I'd remove these sentences, and rather come back to this ... > - We assume that the first try mostly will be successful, and we use > > - atomic_exchange. For the subsequent tries we use > > - atomic_compare_and_exchange. */ > > - if (atomic_exchange_acq (lock, 1) == 0) > > + We assume that the first try mostly will be successful, thus we use > > + atomic_exchange if it is not implemented by a CAS loop. Otherwise .. here. Change to: We assume that the first try mostly will be successful, thus we use atomic_exchange if it is not implemented by a CAS loop (we also assume that atomic_exchange can be faster if it succeeds, see ATOMIC_EXCHANGE_USES_CAS). Otherwise, > > + we use a weak CAS and not an exchange so we bail out after the first > > + failed attempt to change the state. > > + For the subsequent tries we use atomic_compare_and_exchange after we > > + observe a not acquired lock. s/subsequent tries/subsequent attempts,/ s/a not acquired lock/that the lock is not acquired/ > + See also comment in pthread_spin_trylock. Also see the comment ... > > + We use acquire MO to synchronize-with the release MO store in > > + pthread_spin_unlock, and thus ensure that prior critical sections > > + happen-before this critical section. */ > > +#if ATOMIC_EXCHANGE_USES_CAS == 0 We make this a binary macro, so #if !... or #if != 0 is fine. > + /* Try to acquire the lock with an exchange instruction as this architecture > > + has such an instruction and we assume it is faster than a CAS. > > + The acquisition succeeds if the lock had not been acquired before. */ change to: ... if the lock is not in an acquired state. (What you are saying is that the lock had (never) been acquired before, which doesn't make sense.) > + if (__glibc_likely (atomic_exchange_acquire (lock, 1) == 0)) > > + return 0; > > +#elif ATOMIC_EXCHANGE_USES_CAS == 1 just #else > + /* Try to acquire the lock with a CAS instruction as this architecture > > + has no exchange instruction. The acquisition succeeds if the lock is not > > + acquired. */ > > + if (__glibc_likely (atomic_compare_exchange_weak_acquire (lock, &val, 1))) > > return 0; > +#endif > > > do > { > @@ -47,23 +58,24 @@ pthread_spin_lock (pthread_spinlock_t *lock) > to cmpxchg is not a good idea on many targets as that will force > expensive memory synchronizations among processors and penalize other > running threads. > - On the other hand, we do want to update memory state on the local core > > - once in a while to avoid spinning indefinitely until some event that > > - will happen to update local memory as a side-effect. */ > > - if (SPIN_LOCK_READS_BETWEEN_CMPXCHG >= 0) > > + There is no technical reason for throwing in a CAS every now and then, > > + and so far we have no evidence that it can improve performance. > > + If that would be the case, we have to adjust other spin-waiting loops > > + elsewhere, too! > > + Thus we use relaxed MO reads until we observe the lock to not be > > + acquired anymore. */ > > + do > > { > - int wait = SPIN_LOCK_READS_BETWEEN_CMPXCHG; Please add this comment here (we've been adding it elsewhere too, in similar cases): /* TODO Back-off. */ > + atomic_spin_nop (); > > > - while (*lock != 0 && wait > 0) > > - --wait; > > - } > > - else > > - { > > - while (*lock != 0) > > - ; > > + val = atomic_load_relaxed (lock); > > } > + while (val != 0); > > + > > + /* We need acquire memory order here for the same reason as mentioned > > + for the first try to lock the spinlock. */ > > } > - while (atomic_compare_and_exchange_val_acq (lock, 1, 0) != 0); > > + while (!atomic_compare_exchange_weak_acquire (lock, &val, 1)); > > > return 0; > } > diff --git a/nptl/pthread_spin_trylock.c b/nptl/pthread_spin_trylock.c > index 593bba3..a3e9e44 100644 > --- a/nptl/pthread_spin_trylock.c > > +++ b/nptl/pthread_spin_trylock.c > > @@ -23,5 +23,51 @@ > int > pthread_spin_trylock (pthread_spinlock_t *lock) > { > - return atomic_exchange_acq (lock, 1) ? EBUSY : 0; > > + /* For the spin try lock, we have the following possibilities: > > + > > + 1) If we assume that trylock will most likely succeed in practice: > > + * We just do an exchange. > > + > > + 2) If we want to bias towards cases where trylock succeeds, but don't > > + rule out contention: > > + * If exchange is not implemented by a CAS loop, and exchange is faster > > + than CAS, do an exchange. > > + * If exchange is implemented by a CAS loop, use a weak CAS and not an > > + exchange so we bail out after the first failed attempt to change the state. > > + > > + 3) If we expect contention to be likely: > > + * If CAS always brings the cache line into an exclusive state even if the > > + spinlock is already acquired, then load the value first with > > + atomic_load_relaxed and test if lock is not acquired. Then do 2). > > + > > + We prefer case 2) as some of the code out there that uses trylock just > > + uses it to prevent deadlock (e.g., when trying to acquire multiple locks > > + at once), whereas other code uses spin_trylock to implement their own > > + back-off and bounded spinning. > > + Our assumption is that lock acquisitions, including the former use case > > + for trylock, are supposed to succeed in the common case. > > + Thus we choose case 2) as it uses the faster exchange instruction if the > > + architecture has such an instruction. For those architectures case 2) > > + is the same as 1). On the other architectures we are now using one CAS > > + with zero as expected value instead of a CAS loop to exchange to one. */ I'd just say that we assume that 2) is the common case, and that this won't be slower than 1) in the common case. We'd have to rephrase the other sentences too much to make them easy to understand, and I don't think they are really necessary. > + > > + /* We use acquire MO to synchronize-with the release MO store in > > + pthread_spin_unlock, and thus ensure that prior critical sections > > + happen-before this critical section. */ > > +#if ATOMIC_EXCHANGE_USES_CAS == 0 > > + /* Try to acquire the lock with an exchange instruction as this architecture > > + has such an instruction and we assume it is faster than a CAS. > > + The acquisition succeeds if the lock had not been acquired before. */ See above. > + if (atomic_exchange_acquire (lock, 1) == 0) > > + return 0; > > +#elif ATOMIC_EXCHANGE_USES_CAS == 1 See above. > + /* Try to acquire the lock with a CAS instruction as this architecture > > + has no exchange instruction. The acquisition succeeds if the lock is not > > + acquired. */ > > + int val = 0; > > + if (atomic_compare_exchange_weak_acquire (lock, &val, 1)) > > + return 0; Here is where we may address the spurious failure issue: do { /* Make sure we always expect a lock that is not acquired. */ int val = 0; if (atomic_compare_exchange_weak_acquire (lock, &val, 1)) return 0; } /* atomic_compare_exchange_weak_acquire can fail spuriously. Whereas C++11 and C11 make it clear that trylock operations can fail spuriously, POSIX does not explicitly specify this; it only specifies that failing synchronization operations do not need to have synchronization effects themselves, but a spurious failure is something that could contradict a happens-before established earlier (e.g., that we need to observe that the lock is acquired). Therefore, we emulate a strong CAS by simply checking with a relaxed MO load that the lock is really acquired before returning EBUSY; the additional overhead this may cause is on the slow path. */ while (atomic_load_relaxed (lock) == 0); > +#endif > > + > > + return EBUSY; > > } > diff --git a/sysdeps/aarch64/atomic-machine.h b/sysdeps/aarch64/atomic-machine.h > index a5d2213..eb59a5b 100644 > --- a/sysdeps/aarch64/atomic-machine.h > > +++ b/sysdeps/aarch64/atomic-machine.h > > @@ -38,6 +38,7 @@ typedef uintmax_t uatomic_max_t; > > #define __HAVE_64B_ATOMICS 1 > #define USE_ATOMIC_COMPILER_BUILTINS 1 > +#define ATOMIC_EXCHANGE_USES_CAS 0 I'll ask Szabolcs again about this. We may need a comment here explaining the choice. > > /* Compare and exchange. > For all "bool" routines, we return FALSE if exchange succesful. */ > diff --git a/sysdeps/alpha/atomic-machine.h b/sysdeps/alpha/atomic-machine.h > index 06e93f2..e55ecdd 100644 > --- a/sysdeps/alpha/atomic-machine.h > > +++ b/sysdeps/alpha/atomic-machine.h > > @@ -44,6 +44,7 @@ typedef uintmax_t uatomic_max_t; > > #define __HAVE_64B_ATOMICS 1 > #define USE_ATOMIC_COMPILER_BUILTINS 0 > +#define ATOMIC_EXCHANGE_USES_CAS 0 This should be 1, I believe. > > > #ifdef UP > diff --git a/sysdeps/arm/atomic-machine.h b/sysdeps/arm/atomic-machine.h > index eeac7f0..2556438 100644 > --- a/sysdeps/arm/atomic-machine.h > > +++ b/sysdeps/arm/atomic-machine.h > > @@ -35,6 +35,7 @@ typedef uintmax_t uatomic_max_t; > > #define __HAVE_64B_ATOMICS 0 > #define USE_ATOMIC_COMPILER_BUILTINS 0 > +#define ATOMIC_EXCHANGE_USES_CAS 0 This should be 1. Szabolcs? > > void __arm_link_error (void); > > diff --git a/sysdeps/microblaze/atomic-machine.h b/sysdeps/microblaze/atomic-machine.h > index dc5309c..3a9b58f 100644 > --- a/sysdeps/microblaze/atomic-machine.h > > +++ b/sysdeps/microblaze/atomic-machine.h > > @@ -37,6 +37,7 @@ typedef uintmax_t uatomic_max_t; > > #define __HAVE_64B_ATOMICS 0 > #define USE_ATOMIC_COMPILER_BUILTINS 0 > +#define ATOMIC_EXCHANGE_USES_CAS 0 1, not 0. Have you been actually looking at these? The next line in the file is a pretty obvious hint that this is an LLSC machine, and atomic_exchange isn't defined anywhere: > /* Microblaze does not have byte and halfword forms of load and reserve and > diff --git a/sysdeps/mips/atomic-machine.h b/sysdeps/mips/atomic-machine.h > index 54c182b..3d9da0c 100644 > --- a/sysdeps/mips/atomic-machine.h > > +++ b/sysdeps/mips/atomic-machine.h > > @@ -50,6 +50,8 @@ typedef uintmax_t uatomic_max_t; > #define __HAVE_64B_ATOMICS 1 > #endif > > +#define ATOMIC_EXCHANGE_USES_CAS 0 > > + Please ask the MIPS maintainers to review this. > /* See the comments in <sys/asm.h> about the use of the sync instruction. */ > #ifndef MIPS_SYNC > # define MIPS_SYNC sync > diff --git a/sysdeps/powerpc/powerpc32/atomic-machine.h b/sysdeps/powerpc/powerpc32/atomic-machine.h > index 20d5e85..8f4407b 100644 > --- a/sysdeps/powerpc/powerpc32/atomic-machine.h > > +++ b/sysdeps/powerpc/powerpc32/atomic-machine.h > > @@ -35,6 +35,7 @@ > > #define __HAVE_64B_ATOMICS 0 > #define USE_ATOMIC_COMPILER_BUILTINS 0 > +#define ATOMIC_EXCHANGE_USES_CAS 0 1 > > /* > * The 32-bit exchange_bool is different on powerpc64 because the subf > diff --git a/sysdeps/powerpc/powerpc64/atomic-machine.h b/sysdeps/powerpc/powerpc64/atomic-machine.h > index 40c308e..9c4a55b 100644 > --- a/sysdeps/powerpc/powerpc64/atomic-machine.h > > +++ b/sysdeps/powerpc/powerpc64/atomic-machine.h > > @@ -35,6 +35,7 @@ > > #define __HAVE_64B_ATOMICS 1 > #define USE_ATOMIC_COMPILER_BUILTINS 0 > +#define ATOMIC_EXCHANGE_USES_CAS 0 1 > > /* The 32-bit exchange_bool is different on powerpc64 because the subf > does signed 64-bit arithmetic while the lwarx is 32-bit unsigned > diff --git a/sysdeps/sparc/sparc32/atomic-machine.h b/sysdeps/sparc/sparc32/atomic-machine.h > index acd029e..9b2eb47 100644 > --- a/sysdeps/sparc/sparc32/atomic-machine.h > > +++ b/sysdeps/sparc/sparc32/atomic-machine.h > > @@ -49,6 +49,7 @@ typedef uintmax_t uatomic_max_t; > > #define __HAVE_64B_ATOMICS 0 > #define USE_ATOMIC_COMPILER_BUILTINS 0 > +#define ATOMIC_EXCHANGE_USES_CAS 0 I believe this should be 1. > > /* We have no compare and swap, just test and set. > diff --git a/sysdeps/sparc/sparc32/sparcv9/atomic-machine.h b/sysdeps/sparc/sparc32/sparcv9/atomic-machine.h > index 7f7895e..c3486b4 100644 > --- a/sysdeps/sparc/sparc32/sparcv9/atomic-machine.h > > +++ b/sysdeps/sparc/sparc32/sparcv9/atomic-machine.h > > @@ -46,6 +46,7 @@ typedef uintmax_t uatomic_max_t; > > #define __HAVE_64B_ATOMICS 0 > #define USE_ATOMIC_COMPILER_BUILTINS 0 > +#define ATOMIC_EXCHANGE_USES_CAS 0 Likewise. > > > #define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \ > diff --git a/sysdeps/sparc/sparc64/atomic-machine.h b/sysdeps/sparc/sparc64/atomic-machine.h > index 44a43ff..21ef009 100644 > --- a/sysdeps/sparc/sparc64/atomic-machine.h > > +++ b/sysdeps/sparc/sparc64/atomic-machine.h > > @@ -46,6 +46,7 @@ typedef uintmax_t uatomic_max_t; > > #define __HAVE_64B_ATOMICS 1 > #define USE_ATOMIC_COMPILER_BUILTINS 0 > +#define ATOMIC_EXCHANGE_USES_CAS 0 Likewise. > > > #define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \ > diff --git a/sysdeps/tile/tilegx/atomic-machine.h b/sysdeps/tile/tilegx/atomic-machine.h > index 6345251..e77f670 100644 > --- a/sysdeps/tile/tilegx/atomic-machine.h > > +++ b/sysdeps/tile/tilegx/atomic-machine.h > > @@ -31,6 +31,7 @@ > #endif > > #define USE_ATOMIC_COMPILER_BUILTINS 0 > +#define ATOMIC_EXCHANGE_USES_CAS 0 Ask the tile maintainers. I assume this could be either 1 or 0, because both go into the kernel. > > /* Pick appropriate 8- or 4-byte instruction. */ > #define __atomic_update(mem, v, op) \ > diff --git a/sysdeps/tile/tilepro/atomic-machine.h b/sysdeps/tile/tilepro/atomic-machine.h > index 33a8b85..45e36de 100644 > --- a/sysdeps/tile/tilepro/atomic-machine.h > > +++ b/sysdeps/tile/tilepro/atomic-machine.h > > @@ -23,6 +23,7 @@ > > #define __HAVE_64B_ATOMICS 0 > #define USE_ATOMIC_COMPILER_BUILTINS 0 > +#define ATOMIC_EXCHANGE_USES_CAS 0 Likewise. > > /* 32-bit integer compare-and-exchange. */ > static __inline __attribute__ ((always_inline)) > diff --git a/sysdeps/unix/sysv/linux/hppa/atomic-machine.h b/sysdeps/unix/sysv/linux/hppa/atomic-machine.h > index 2cd2235..9adcab7 100644 > --- a/sysdeps/unix/sysv/linux/hppa/atomic-machine.h > > +++ b/sysdeps/unix/sysv/linux/hppa/atomic-machine.h > > @@ -38,6 +38,7 @@ typedef uintmax_t uatomic_max_t; > > #define __HAVE_64B_ATOMICS 0 > #define USE_ATOMIC_COMPILER_BUILTINS 0 > +#define ATOMIC_EXCHANGE_USES_CAS 1 Same as for tile. > > /* prev = *addr; > if (prev == old) Otherwise, this patch looks good to me.
On Thu, 6 Apr 2017, Torvald Riegel wrote: > Have you been actually looking at these? The next line in the file is a > pretty obvious hint that this is an LLSC machine, and atomic_exchange > isn't defined anywhere: > > > /* Microblaze does not have byte and halfword forms of load and reserve and > > diff --git a/sysdeps/mips/atomic-machine.h b/sysdeps/mips/atomic-machine.h > > index 54c182b..3d9da0c 100644 > > --- a/sysdeps/mips/atomic-machine.h > > > > +++ b/sysdeps/mips/atomic-machine.h > > > > @@ -50,6 +50,8 @@ typedef uintmax_t uatomic_max_t; > > #define __HAVE_64B_ATOMICS 1 > > #endif > > > > +#define ATOMIC_EXCHANGE_USES_CAS 0 > > > > + > > Please ask the MIPS maintainers to review this. MIPS is an LLSC machine. However, XLP has a direct atomic exchange instruction (so that will be used if _MIPS_ARCH_XLP is defined, in the case where this header is using compiler builtins).
From b8dc58807825423ce5e569018ce9dbc3bd170e67 Mon Sep 17 00:00:00 2001 From: Stefan Liebler <stli@linux.vnet.ibm.com> Date: Tue, 14 Mar 2017 14:10:05 +0100 Subject: [PATCH 1/2] Optimize generic spinlock code and use C11 like atomic macros. This patch optimizes the generic spinlock code. The type pthread_spinlock_t is a typedef to volatile int on all archs. Passing a volatile pointer to the atomic macros which are not mapped to the C11 atomic builtins can lead to extra stores and loads to stack if such a macro creates a temporary variable by using "__typeof (*(mem)) tmp;". Thus, those macros which are used by spinlock code - atomic_exchange_acquire, atomic_load_relaxed, atomic_compare_exchange_weak - have to be adjusted. According to the comment from Szabolcs Nagy, the type of a cast expression is unqualified (see http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_423.htm): __typeof ((__typeof (*(mem)) *(mem)) tmp; Thus from spinlock perspective the variable tmp is of type int instead of type volatile int. This patch adjusts those macros in include/atomic.h. With this construct GCC >= 5 omits the extra stores and loads. The atomic macros are replaced by the C11 like atomic macros and thus the code is aligned to it. The pthread_spin_unlock implementation is now using release memory order instead of sequentially consistent memory order. The issue with passed volatile int pointers applies to the C11 like atomic macros as well as the ones used before. I've added a glibc_likely hint to the first atomic exchange in pthread_spin_lock in order to return immediately to the caller if the lock is free. Without the hint, there is an additional jump if the lock is free. I've added the atomic_spin_nop macro within the loop of plain reads. The plain reads are also realized by C11 like atomic_load_relaxed macro. The new define ATOMIC_EXCHANGE_USES_CAS determines if the first try to acquire the spinlock in pthread_spin_lock or pthread_spin_trylock is an exchange or a CAS. This is defined in atomic-machine.h for all architectures. The define SPIN_LOCK_READS_BETWEEN_CMPXCHG is now removed. There is no technical reason for throwing in a CAS every now and then, and so far we have no evidence that it can improve performance. If that would be the case, we have to adjust other spin-waiting loops elsewhere, too! Using a CAS loop without plain reads is not a good idea on many targets and wasn't used by one. Thus there is now no option to do so. Architectures are now using the generic spinlock automatically if they do not provide an own implementation. Thus the pthread_spin_lock.c files in sysdeps folder are deleted. ChangeLog: * NEWS: Mention new spinlock implementation. * include/atomic.h: (__atomic_val_bysize): Cast type to omit volatile qualifier. (atomic_exchange_acq): Likewise. (atomic_load_relaxed): Likewise. (ATOMIC_EXCHANGE_USES_CAS): Check definition. * nptl/pthread_spin_init.c (pthread_spin_init): Use atomic_store_relaxed. * nptl/pthread_spin_lock.c (pthread_spin_lock): Use C11-like atomic macros. * nptl/pthread_spin_trylock.c (pthread_spin_trylock): Likewise. * nptl/pthread_spin_unlock.c (pthread_spin_unlock): Use atomic_store_release. * sysdeps/aarch64/nptl/pthread_spin_lock.c: Delete File. * sysdeps/arm/nptl/pthread_spin_lock.c: Likewise. * sysdeps/hppa/nptl/pthread_spin_lock.c: Likewise. * sysdeps/m68k/nptl/pthread_spin_lock.c: Likewise. * sysdeps/microblaze/nptl/pthread_spin_lock.c: Likewise. * sysdeps/mips/nptl/pthread_spin_lock.c: Likewise. * sysdeps/nios2/nptl/pthread_spin_lock.c: Likewise. * sysdeps/aarch64/atomic-machine.h (ATOMIC_EXCHANGE_USES_CAS): Define. * sysdeps/alpha/atomic-machine.h: Likewise. * sysdeps/arm/atomic-machine.h: Likewise. * sysdeps/i386/atomic-machine.h: Likewise. * sysdeps/ia64/atomic-machine.h: Likewise. * sysdeps/m68k/coldfire/atomic-machine.h: Likewise. * sysdeps/m68k/m680x0/m68020/atomic-machine.h: Likewise. * sysdeps/microblaze/atomic-machine.h: Likewise. * sysdeps/mips/atomic-machine.h: Likewise. * sysdeps/powerpc/powerpc32/atomic-machine.h: Likewise. * sysdeps/powerpc/powerpc64/atomic-machine.h: Likewise. * sysdeps/s390/atomic-machine.h: Likewise. * sysdeps/sparc/sparc32/atomic-machine.h: Likewise. * sysdeps/sparc/sparc32/sparcv9/atomic-machine.h: Likewise. * sysdeps/sparc/sparc64/atomic-machine.h: Likewise. * sysdeps/tile/tilegx/atomic-machine.h: Likewise. * sysdeps/tile/tilepro/atomic-machine.h: Likewise. * sysdeps/unix/sysv/linux/hppa/atomic-machine.h: Likewise. * sysdeps/unix/sysv/linux/m68k/coldfire/atomic-machine.h: Likewise. * sysdeps/unix/sysv/linux/nios2/atomic-machine.h: Likewise. * sysdeps/unix/sysv/linux/sh/atomic-machine.h: Likewise. * sysdeps/x86_64/atomic-machine.h: Likewise. --- NEWS | 11 ++++ include/atomic.h | 17 ++++-- nptl/pthread_spin_init.c | 3 +- nptl/pthread_spin_lock.c | 64 +++++++++++++--------- nptl/pthread_spin_trylock.c | 48 +++++++++++++++- nptl/pthread_spin_unlock.c | 6 +- sysdeps/aarch64/atomic-machine.h | 1 + sysdeps/aarch64/nptl/pthread_spin_lock.c | 24 -------- sysdeps/alpha/atomic-machine.h | 1 + sysdeps/arm/atomic-machine.h | 1 + sysdeps/arm/nptl/pthread_spin_lock.c | 23 -------- sysdeps/hppa/nptl/pthread_spin_lock.c | 23 -------- sysdeps/i386/atomic-machine.h | 1 + sysdeps/ia64/atomic-machine.h | 1 + sysdeps/m68k/coldfire/atomic-machine.h | 1 + sysdeps/m68k/m680x0/m68020/atomic-machine.h | 1 + sysdeps/m68k/nptl/pthread_spin_lock.c | 24 -------- sysdeps/microblaze/atomic-machine.h | 1 + sysdeps/microblaze/nptl/pthread_spin_lock.c | 24 -------- sysdeps/mips/atomic-machine.h | 2 + sysdeps/mips/nptl/pthread_spin_lock.c | 23 -------- sysdeps/nios2/nptl/pthread_spin_lock.c | 24 -------- sysdeps/powerpc/powerpc32/atomic-machine.h | 1 + sysdeps/powerpc/powerpc64/atomic-machine.h | 1 + sysdeps/s390/atomic-machine.h | 2 + sysdeps/sparc/sparc32/atomic-machine.h | 1 + sysdeps/sparc/sparc32/sparcv9/atomic-machine.h | 1 + sysdeps/sparc/sparc64/atomic-machine.h | 1 + sysdeps/tile/tilegx/atomic-machine.h | 1 + sysdeps/tile/tilepro/atomic-machine.h | 1 + sysdeps/unix/sysv/linux/hppa/atomic-machine.h | 1 + .../unix/sysv/linux/m68k/coldfire/atomic-machine.h | 1 + sysdeps/unix/sysv/linux/nios2/atomic-machine.h | 1 + sysdeps/unix/sysv/linux/sh/atomic-machine.h | 1 + sysdeps/x86_64/atomic-machine.h | 1 + 35 files changed, 139 insertions(+), 199 deletions(-) delete mode 100644 sysdeps/aarch64/nptl/pthread_spin_lock.c delete mode 100644 sysdeps/arm/nptl/pthread_spin_lock.c delete mode 100644 sysdeps/hppa/nptl/pthread_spin_lock.c delete mode 100644 sysdeps/m68k/nptl/pthread_spin_lock.c delete mode 100644 sysdeps/microblaze/nptl/pthread_spin_lock.c delete mode 100644 sysdeps/mips/nptl/pthread_spin_lock.c delete mode 100644 sysdeps/nios2/nptl/pthread_spin_lock.c diff --git a/NEWS b/NEWS index 9efe1e5..19ac934 100644 --- a/NEWS +++ b/NEWS @@ -17,6 +17,17 @@ Version 2.26 by default. Applications needing features missing from TIRPC should consider the rpcsvc-proto project developed by Thorsten Kukuk (SUSE). +* The synchronization that pthread_spin_unlock performs has been changed + to now be equivalent to a C11 atomic store with release memory order to + the spin lock's memory location. This ensures correct synchronization + for the spin lock's operations and critical sections protected by a spin + lock. Previously, several (but not all) architectures used stronger + synchronization (e.g., containing what is often called a full barrier). + This change can improve performance, but may affect odd fringe uses of + spin locks that depend on the previous behavior (e.g., using spin locks + as atomic variables to try to implement Dekker's mutual exclusion + algorithm). + Security related changes: [Add security related changes here] diff --git a/include/atomic.h b/include/atomic.h index 7f32640..4d1b0df 100644 --- a/include/atomic.h +++ b/include/atomic.h @@ -54,7 +54,7 @@ and following args. */ #define __atomic_val_bysize(pre, post, mem, ...) \ ({ \ - __typeof (*mem) __atg1_result; \ + __typeof ((__typeof (*(mem))) *(mem)) __atg1_result; \ if (sizeof (*mem) == 1) \ __atg1_result = pre##_8_##post (mem, __VA_ARGS__); \ else if (sizeof (*mem) == 2) \ @@ -162,9 +162,9 @@ /* Store NEWVALUE in *MEM and return the old value. */ #ifndef atomic_exchange_acq # define atomic_exchange_acq(mem, newvalue) \ - ({ __typeof (*(mem)) __atg5_oldval; \ + ({ __typeof ((__typeof (*(mem))) *(mem)) __atg5_oldval; \ __typeof (mem) __atg5_memp = (mem); \ - __typeof (*(mem)) __atg5_value = (newvalue); \ + __typeof ((__typeof (*(mem))) *(mem)) __atg5_value = (newvalue); \ \ do \ __atg5_oldval = *__atg5_memp; \ @@ -668,7 +668,7 @@ void __atomic_link_error (void); # ifndef atomic_load_relaxed # define atomic_load_relaxed(mem) \ - ({ __typeof (*(mem)) __atg100_val; \ + ({ __typeof ((__typeof (*(mem))) *(mem)) __atg100_val; \ __asm ("" : "=r" (__atg100_val) : "0" (*(mem))); \ __atg100_val; }) # endif @@ -818,4 +818,13 @@ void __atomic_link_error (void); # define atomic_spin_nop() do { /* nothing */ } while (0) #endif +/* This is equal to 1 if atomic_exchange is implemented by a CAS loop + and 0 if atomic_exchange is implemented with an instruction faster than + CAS. */ +#if ! defined ATOMIC_EXCHANGE_USES_CAS || ! (ATOMIC_EXCHANGE_USES_CAS == 0 \ + || ATOMIC_EXCHANGE_USES_CAS == 1) +# error ATOMIC_EXCHANGE_USES_CAS has to be defined to 0 or 1 +#endif + #endif /* atomic.h */ + diff --git a/nptl/pthread_spin_init.c b/nptl/pthread_spin_init.c index 01dec5e..fe30913 100644 --- a/nptl/pthread_spin_init.c +++ b/nptl/pthread_spin_init.c @@ -22,6 +22,7 @@ int pthread_spin_init (pthread_spinlock_t *lock, int pshared) { - *lock = 0; + /* Relaxed MO is fine because this is an initializing store. */ + atomic_store_relaxed (lock, 0); return 0; } diff --git a/nptl/pthread_spin_lock.c b/nptl/pthread_spin_lock.c index 4d03b78..c2fb8e7 100644 --- a/nptl/pthread_spin_lock.c +++ b/nptl/pthread_spin_lock.c @@ -19,27 +19,38 @@ #include <atomic.h> #include "pthreadP.h" -/* A machine-specific version can define SPIN_LOCK_READS_BETWEEN_CMPXCHG - to the number of plain reads that it's optimal to spin on between uses - of atomic_compare_and_exchange_val_acq. If spinning forever is optimal - then use -1. If no plain reads here would ever be optimal, use 0. */ -#ifndef SPIN_LOCK_READS_BETWEEN_CMPXCHG -# warning machine-dependent file should define SPIN_LOCK_READS_BETWEEN_CMPXCHG -# define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000 -#endif - int pthread_spin_lock (pthread_spinlock_t *lock) { + int val = 0; + /* atomic_exchange usually takes less instructions than atomic_compare_and_exchange. On the other hand, atomic_compare_and_exchange potentially generates less bus traffic when the lock is locked. - We assume that the first try mostly will be successful, and we use - atomic_exchange. For the subsequent tries we use - atomic_compare_and_exchange. */ - if (atomic_exchange_acq (lock, 1) == 0) + We assume that the first try mostly will be successful, thus we use + atomic_exchange if it is not implemented by a CAS loop. Otherwise + we use a weak CAS and not an exchange so we bail out after the first + failed attempt to change the state. + For the subsequent tries we use atomic_compare_and_exchange after we + observe a not acquired lock. + See also comment in pthread_spin_trylock. + We use acquire MO to synchronize-with the release MO store in + pthread_spin_unlock, and thus ensure that prior critical sections + happen-before this critical section. */ +#if ATOMIC_EXCHANGE_USES_CAS == 0 + /* Try to acquire the lock with an exchange instruction as this architecture + has such an instruction and we assume it is faster than a CAS. + The acquisition succeeds if the lock had not been acquired before. */ + if (__glibc_likely (atomic_exchange_acquire (lock, 1) == 0)) + return 0; +#elif ATOMIC_EXCHANGE_USES_CAS == 1 + /* Try to acquire the lock with a CAS instruction as this architecture + has no exchange instruction. The acquisition succeeds if the lock is not + acquired. */ + if (__glibc_likely (atomic_compare_exchange_weak_acquire (lock, &val, 1))) return 0; +#endif do { @@ -47,23 +58,24 @@ pthread_spin_lock (pthread_spinlock_t *lock) to cmpxchg is not a good idea on many targets as that will force expensive memory synchronizations among processors and penalize other running threads. - On the other hand, we do want to update memory state on the local core - once in a while to avoid spinning indefinitely until some event that - will happen to update local memory as a side-effect. */ - if (SPIN_LOCK_READS_BETWEEN_CMPXCHG >= 0) + There is no technical reason for throwing in a CAS every now and then, + and so far we have no evidence that it can improve performance. + If that would be the case, we have to adjust other spin-waiting loops + elsewhere, too! + Thus we use relaxed MO reads until we observe the lock to not be + acquired anymore. */ + do { - int wait = SPIN_LOCK_READS_BETWEEN_CMPXCHG; + atomic_spin_nop (); - while (*lock != 0 && wait > 0) - --wait; - } - else - { - while (*lock != 0) - ; + val = atomic_load_relaxed (lock); } + while (val != 0); + + /* We need acquire memory order here for the same reason as mentioned + for the first try to lock the spinlock. */ } - while (atomic_compare_and_exchange_val_acq (lock, 1, 0) != 0); + while (!atomic_compare_exchange_weak_acquire (lock, &val, 1)); return 0; } diff --git a/nptl/pthread_spin_trylock.c b/nptl/pthread_spin_trylock.c index 593bba3..a3e9e44 100644 --- a/nptl/pthread_spin_trylock.c +++ b/nptl/pthread_spin_trylock.c @@ -23,5 +23,51 @@ int pthread_spin_trylock (pthread_spinlock_t *lock) { - return atomic_exchange_acq (lock, 1) ? EBUSY : 0; + /* For the spin try lock, we have the following possibilities: + + 1) If we assume that trylock will most likely succeed in practice: + * We just do an exchange. + + 2) If we want to bias towards cases where trylock succeeds, but don't + rule out contention: + * If exchange is not implemented by a CAS loop, and exchange is faster + than CAS, do an exchange. + * If exchange is implemented by a CAS loop, use a weak CAS and not an + exchange so we bail out after the first failed attempt to change the state. + + 3) If we expect contention to be likely: + * If CAS always brings the cache line into an exclusive state even if the + spinlock is already acquired, then load the value first with + atomic_load_relaxed and test if lock is not acquired. Then do 2). + + We prefer case 2) as some of the code out there that uses trylock just + uses it to prevent deadlock (e.g., when trying to acquire multiple locks + at once), whereas other code uses spin_trylock to implement their own + back-off and bounded spinning. + Our assumption is that lock acquisitions, including the former use case + for trylock, are supposed to succeed in the common case. + Thus we choose case 2) as it uses the faster exchange instruction if the + architecture has such an instruction. For those architectures case 2) + is the same as 1). On the other architectures we are now using one CAS + with zero as expected value instead of a CAS loop to exchange to one. */ + + /* We use acquire MO to synchronize-with the release MO store in + pthread_spin_unlock, and thus ensure that prior critical sections + happen-before this critical section. */ +#if ATOMIC_EXCHANGE_USES_CAS == 0 + /* Try to acquire the lock with an exchange instruction as this architecture + has such an instruction and we assume it is faster than a CAS. + The acquisition succeeds if the lock had not been acquired before. */ + if (atomic_exchange_acquire (lock, 1) == 0) + return 0; +#elif ATOMIC_EXCHANGE_USES_CAS == 1 + /* Try to acquire the lock with a CAS instruction as this architecture + has no exchange instruction. The acquisition succeeds if the lock is not + acquired. */ + int val = 0; + if (atomic_compare_exchange_weak_acquire (lock, &val, 1)) + return 0; +#endif + + return EBUSY; } diff --git a/nptl/pthread_spin_unlock.c b/nptl/pthread_spin_unlock.c index 5fd73e5..f83b696 100644 --- a/nptl/pthread_spin_unlock.c +++ b/nptl/pthread_spin_unlock.c @@ -23,7 +23,9 @@ int pthread_spin_unlock (pthread_spinlock_t *lock) { - atomic_full_barrier (); - *lock = 0; + /* The atomic_store_release synchronizes-with the atomic_exchange_acquire + or atomic_compare_exchange_weak_acquire in pthread_spin_lock / + pthread_spin_trylock. */ + atomic_store_release (lock, 0); return 0; } diff --git a/sysdeps/aarch64/atomic-machine.h b/sysdeps/aarch64/atomic-machine.h index a5d2213..eb59a5b 100644 --- a/sysdeps/aarch64/atomic-machine.h +++ b/sysdeps/aarch64/atomic-machine.h @@ -38,6 +38,7 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 1 #define USE_ATOMIC_COMPILER_BUILTINS 1 +#define ATOMIC_EXCHANGE_USES_CAS 0 /* Compare and exchange. For all "bool" routines, we return FALSE if exchange succesful. */ diff --git a/sysdeps/aarch64/nptl/pthread_spin_lock.c b/sysdeps/aarch64/nptl/pthread_spin_lock.c deleted file mode 100644 index fcfcb40..0000000 --- a/sysdeps/aarch64/nptl/pthread_spin_lock.c +++ /dev/null @@ -1,24 +0,0 @@ -/* Copyright (C) 2008-2017 Free Software Foundation, Inc. - - This file is part of the GNU C Library. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public License as - published by the Free Software Foundation; either version 2.1 of the - License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library; if not, see - <http://www.gnu.org/licenses/>. */ - -#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000 - -/* We can't use the normal "#include <nptl/pthread_spin_lock.c>" because - it will resolve to this very file. Using "sysdeps/.." as reference to the - top level directory does the job. */ -#include <sysdeps/../nptl/pthread_spin_lock.c> diff --git a/sysdeps/alpha/atomic-machine.h b/sysdeps/alpha/atomic-machine.h index 06e93f2..e55ecdd 100644 --- a/sysdeps/alpha/atomic-machine.h +++ b/sysdeps/alpha/atomic-machine.h @@ -44,6 +44,7 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 1 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 0 #ifdef UP diff --git a/sysdeps/arm/atomic-machine.h b/sysdeps/arm/atomic-machine.h index eeac7f0..2556438 100644 --- a/sysdeps/arm/atomic-machine.h +++ b/sysdeps/arm/atomic-machine.h @@ -35,6 +35,7 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 0 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 0 void __arm_link_error (void); diff --git a/sysdeps/arm/nptl/pthread_spin_lock.c b/sysdeps/arm/nptl/pthread_spin_lock.c deleted file mode 100644 index 037b3b8..0000000 --- a/sysdeps/arm/nptl/pthread_spin_lock.c +++ /dev/null @@ -1,23 +0,0 @@ -/* Copyright (C) 2008-2017 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library. If not, see - <http://www.gnu.org/licenses/>. */ - -#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000 - -/* We can't use the normal "#include <nptl/pthread_spin_lock.c>" because - it will resolve to this very file. Using "sysdeps/.." as reference to the - top level directory does the job. */ -#include <sysdeps/../nptl/pthread_spin_lock.c> diff --git a/sysdeps/hppa/nptl/pthread_spin_lock.c b/sysdeps/hppa/nptl/pthread_spin_lock.c deleted file mode 100644 index 14f36a6..0000000 --- a/sysdeps/hppa/nptl/pthread_spin_lock.c +++ /dev/null @@ -1,23 +0,0 @@ -/* Copyright (C) 2005-2017 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library. If not, see - <http://www.gnu.org/licenses/>. */ - -#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000 - -/* We can't use the normal "#include <nptl/pthread_spin_lock.c>" because - it will resolve to this very file. Using "sysdeps/.." as reference to the - top level directory does the job. */ -#include <sysdeps/../nptl/pthread_spin_lock.c> diff --git a/sysdeps/i386/atomic-machine.h b/sysdeps/i386/atomic-machine.h index 77759f7..0e24200 100644 --- a/sysdeps/i386/atomic-machine.h +++ b/sysdeps/i386/atomic-machine.h @@ -56,6 +56,7 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 0 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 0 #define atomic_compare_and_exchange_val_acq(mem, newval, oldval) \ diff --git a/sysdeps/ia64/atomic-machine.h b/sysdeps/ia64/atomic-machine.h index ecf9750..971084e 100644 --- a/sysdeps/ia64/atomic-machine.h +++ b/sysdeps/ia64/atomic-machine.h @@ -45,6 +45,7 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 1 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 0 #define __arch_compare_and_exchange_bool_8_acq(mem, newval, oldval) \ diff --git a/sysdeps/m68k/coldfire/atomic-machine.h b/sysdeps/m68k/coldfire/atomic-machine.h index 9aeb993..72b037e 100644 --- a/sysdeps/m68k/coldfire/atomic-machine.h +++ b/sysdeps/m68k/coldfire/atomic-machine.h @@ -52,6 +52,7 @@ typedef uintmax_t uatomic_max_t; /* If we have just non-atomic operations, we can as well make them wide. */ #define __HAVE_64B_ATOMICS 1 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 1 /* The only basic operation needed is compare and exchange. */ #define atomic_compare_and_exchange_val_acq(mem, newval, oldval) \ diff --git a/sysdeps/m68k/m680x0/m68020/atomic-machine.h b/sysdeps/m68k/m680x0/m68020/atomic-machine.h index 00dc22d..a319471 100644 --- a/sysdeps/m68k/m680x0/m68020/atomic-machine.h +++ b/sysdeps/m68k/m680x0/m68020/atomic-machine.h @@ -46,6 +46,7 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 1 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 1 #define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \ ({ __typeof (*(mem)) __ret; \ diff --git a/sysdeps/m68k/nptl/pthread_spin_lock.c b/sysdeps/m68k/nptl/pthread_spin_lock.c deleted file mode 100644 index 62795f4..0000000 --- a/sysdeps/m68k/nptl/pthread_spin_lock.c +++ /dev/null @@ -1,24 +0,0 @@ -/* Copyright (C) 2010-2017 Free Software Foundation, Inc. - This file is part of the GNU C Library. - Contributed by Maxim Kuvyrkov <maxim@codesourcery.com>, 2010. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library. If not, see - <http://www.gnu.org/licenses/>. */ - -#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000 - -/* We can't use the normal "#include <nptl/pthread_spin_lock.c>" because - it will resolve to this very file. Using "sysdeps/.." as reference to the - top level directory does the job. */ -#include <sysdeps/../nptl/pthread_spin_lock.c> diff --git a/sysdeps/microblaze/atomic-machine.h b/sysdeps/microblaze/atomic-machine.h index dc5309c..3a9b58f 100644 --- a/sysdeps/microblaze/atomic-machine.h +++ b/sysdeps/microblaze/atomic-machine.h @@ -37,6 +37,7 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 0 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 0 /* Microblaze does not have byte and halfword forms of load and reserve and diff --git a/sysdeps/microblaze/nptl/pthread_spin_lock.c b/sysdeps/microblaze/nptl/pthread_spin_lock.c deleted file mode 100644 index fcfcb40..0000000 --- a/sysdeps/microblaze/nptl/pthread_spin_lock.c +++ /dev/null @@ -1,24 +0,0 @@ -/* Copyright (C) 2008-2017 Free Software Foundation, Inc. - - This file is part of the GNU C Library. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public License as - published by the Free Software Foundation; either version 2.1 of the - License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library; if not, see - <http://www.gnu.org/licenses/>. */ - -#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000 - -/* We can't use the normal "#include <nptl/pthread_spin_lock.c>" because - it will resolve to this very file. Using "sysdeps/.." as reference to the - top level directory does the job. */ -#include <sysdeps/../nptl/pthread_spin_lock.c> diff --git a/sysdeps/mips/atomic-machine.h b/sysdeps/mips/atomic-machine.h index 54c182b..3d9da0c 100644 --- a/sysdeps/mips/atomic-machine.h +++ b/sysdeps/mips/atomic-machine.h @@ -50,6 +50,8 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 1 #endif +#define ATOMIC_EXCHANGE_USES_CAS 0 + /* See the comments in <sys/asm.h> about the use of the sync instruction. */ #ifndef MIPS_SYNC # define MIPS_SYNC sync diff --git a/sysdeps/mips/nptl/pthread_spin_lock.c b/sysdeps/mips/nptl/pthread_spin_lock.c deleted file mode 100644 index 19d87a5..0000000 --- a/sysdeps/mips/nptl/pthread_spin_lock.c +++ /dev/null @@ -1,23 +0,0 @@ -/* Copyright (C) 2012-2017 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library. If not, see - <http://www.gnu.org/licenses/>. */ - -#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000 - -/* We can't use the normal "#include <nptl/pthread_spin_lock.c>" because - it will resolve to this very file. Using "sysdeps/.." as reference to the - top level directory does the job. */ -#include <sysdeps/../nptl/pthread_spin_lock.c> diff --git a/sysdeps/nios2/nptl/pthread_spin_lock.c b/sysdeps/nios2/nptl/pthread_spin_lock.c deleted file mode 100644 index b203469..0000000 --- a/sysdeps/nios2/nptl/pthread_spin_lock.c +++ /dev/null @@ -1,24 +0,0 @@ -/* pthread spin-lock implementation for Nios II. - Copyright (C) 2005-2017 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library. If not, see - <http://www.gnu.org/licenses/>. */ - -#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000 - -/* We can't use the normal "#include <nptl/pthread_spin_lock.c>" because - it will resolve to this very file. Using "sysdeps/.." as reference to the - top level directory does the job. */ -#include <sysdeps/../nptl/pthread_spin_lock.c> diff --git a/sysdeps/powerpc/powerpc32/atomic-machine.h b/sysdeps/powerpc/powerpc32/atomic-machine.h index 20d5e85..8f4407b 100644 --- a/sysdeps/powerpc/powerpc32/atomic-machine.h +++ b/sysdeps/powerpc/powerpc32/atomic-machine.h @@ -35,6 +35,7 @@ #define __HAVE_64B_ATOMICS 0 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 0 /* * The 32-bit exchange_bool is different on powerpc64 because the subf diff --git a/sysdeps/powerpc/powerpc64/atomic-machine.h b/sysdeps/powerpc/powerpc64/atomic-machine.h index 40c308e..9c4a55b 100644 --- a/sysdeps/powerpc/powerpc64/atomic-machine.h +++ b/sysdeps/powerpc/powerpc64/atomic-machine.h @@ -35,6 +35,7 @@ #define __HAVE_64B_ATOMICS 1 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 0 /* The 32-bit exchange_bool is different on powerpc64 because the subf does signed 64-bit arithmetic while the lwarx is 32-bit unsigned diff --git a/sysdeps/s390/atomic-machine.h b/sysdeps/s390/atomic-machine.h index 690d2e3..adaca40 100644 --- a/sysdeps/s390/atomic-machine.h +++ b/sysdeps/s390/atomic-machine.h @@ -67,6 +67,8 @@ typedef uintmax_t uatomic_max_t; # define __HAVE_64B_ATOMICS 0 #endif +#define ATOMIC_EXCHANGE_USES_CAS 1 + /* Implement some of the non-C11 atomic macros from include/atomic.h with help of the C11 atomic builtins. The other non-C11 atomic macros are using the macros defined here. */ diff --git a/sysdeps/sparc/sparc32/atomic-machine.h b/sysdeps/sparc/sparc32/atomic-machine.h index acd029e..9b2eb47 100644 --- a/sysdeps/sparc/sparc32/atomic-machine.h +++ b/sysdeps/sparc/sparc32/atomic-machine.h @@ -49,6 +49,7 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 0 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 0 /* We have no compare and swap, just test and set. diff --git a/sysdeps/sparc/sparc32/sparcv9/atomic-machine.h b/sysdeps/sparc/sparc32/sparcv9/atomic-machine.h index 7f7895e..c3486b4 100644 --- a/sysdeps/sparc/sparc32/sparcv9/atomic-machine.h +++ b/sysdeps/sparc/sparc32/sparcv9/atomic-machine.h @@ -46,6 +46,7 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 0 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 0 #define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \ diff --git a/sysdeps/sparc/sparc64/atomic-machine.h b/sysdeps/sparc/sparc64/atomic-machine.h index 44a43ff..21ef009 100644 --- a/sysdeps/sparc/sparc64/atomic-machine.h +++ b/sysdeps/sparc/sparc64/atomic-machine.h @@ -46,6 +46,7 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 1 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 0 #define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \ diff --git a/sysdeps/tile/tilegx/atomic-machine.h b/sysdeps/tile/tilegx/atomic-machine.h index 6345251..e77f670 100644 --- a/sysdeps/tile/tilegx/atomic-machine.h +++ b/sysdeps/tile/tilegx/atomic-machine.h @@ -31,6 +31,7 @@ #endif #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 0 /* Pick appropriate 8- or 4-byte instruction. */ #define __atomic_update(mem, v, op) \ diff --git a/sysdeps/tile/tilepro/atomic-machine.h b/sysdeps/tile/tilepro/atomic-machine.h index 33a8b85..45e36de 100644 --- a/sysdeps/tile/tilepro/atomic-machine.h +++ b/sysdeps/tile/tilepro/atomic-machine.h @@ -23,6 +23,7 @@ #define __HAVE_64B_ATOMICS 0 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 0 /* 32-bit integer compare-and-exchange. */ static __inline __attribute__ ((always_inline)) diff --git a/sysdeps/unix/sysv/linux/hppa/atomic-machine.h b/sysdeps/unix/sysv/linux/hppa/atomic-machine.h index 2cd2235..9adcab7 100644 --- a/sysdeps/unix/sysv/linux/hppa/atomic-machine.h +++ b/sysdeps/unix/sysv/linux/hppa/atomic-machine.h @@ -38,6 +38,7 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 0 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 1 /* prev = *addr; if (prev == old) diff --git a/sysdeps/unix/sysv/linux/m68k/coldfire/atomic-machine.h b/sysdeps/unix/sysv/linux/m68k/coldfire/atomic-machine.h index 6755a05..c1b6950 100644 --- a/sysdeps/unix/sysv/linux/m68k/coldfire/atomic-machine.h +++ b/sysdeps/unix/sysv/linux/m68k/coldfire/atomic-machine.h @@ -38,6 +38,7 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 0 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 1 /* The only basic operation needed is compare and exchange. */ /* For ColdFire we'll have to trap into the kernel mode anyway, diff --git a/sysdeps/unix/sysv/linux/nios2/atomic-machine.h b/sysdeps/unix/sysv/linux/nios2/atomic-machine.h index 6111ccf..d98fa66 100644 --- a/sysdeps/unix/sysv/linux/nios2/atomic-machine.h +++ b/sysdeps/unix/sysv/linux/nios2/atomic-machine.h @@ -33,6 +33,7 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 0 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 1 #define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \ (abort (), (__typeof (*mem)) 0) diff --git a/sysdeps/unix/sysv/linux/sh/atomic-machine.h b/sysdeps/unix/sysv/linux/sh/atomic-machine.h index 3c58b70..428b71e 100644 --- a/sysdeps/unix/sysv/linux/sh/atomic-machine.h +++ b/sysdeps/unix/sysv/linux/sh/atomic-machine.h @@ -46,6 +46,7 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 0 #define USE_ATOMIC_COMPILER_BUILTINS 0 +#define ATOMIC_EXCHANGE_USES_CAS 1 /* SH kernel has implemented a gUSA ("g" User Space Atomicity) support for the user space atomicity. The atomicity macros use this scheme. diff --git a/sysdeps/x86_64/atomic-machine.h b/sysdeps/x86_64/atomic-machine.h index 2e8a9aa..c454734 100644 --- a/sysdeps/x86_64/atomic-machine.h +++ b/sysdeps/x86_64/atomic-machine.h @@ -59,6 +59,7 @@ typedef uintmax_t uatomic_max_t; #define __HAVE_64B_ATOMICS 1 #define USE_ATOMIC_COMPILER_BUILTINS 1 +#define ATOMIC_EXCHANGE_USES_CAS 0 #define atomic_compare_and_exchange_val_acq(mem, newval, oldval) \ __sync_val_compare_and_swap (mem, oldval, newval) -- 2.7.4