[PATCH] [libstdc++] Refactor/cleanup of atomic wait implementation

Jonathan Wakely jwakely@redhat.com
Tue Apr 20 14:26:55 GMT 2021


On 20/04/21 15:25 +0100, Jonathan Wakely wrote:
>On 20/04/21 12:41 +0100, Jonathan Wakely wrote:
>>On 20/04/21 12:04 +0100, Jonathan Wakely wrote:
>>>On 19/04/21 12:23 -0700, Thomas Rodgers wrote:
>>>>From: Thomas Rodgers <rodgert@twrodgers.com>
>>>>
>>>>This patch address jwakely's feedback from 2021-04-15.
>>>>
>>>>This is a substantial rewrite of the atomic wait/notify (and timed wait
>>>>counterparts) implementation.
>>>>
>>>>The previous __platform_wait looped on EINTR however this behavior is
>>>>not required by the standard. A new _GLIBCXX_HAVE_PLATFORM_WAIT macro
>>>>now controls whether wait/notify are implemented using a platform
>>>>specific primitive or with a platform agnostic mutex/condvar. This
>>>>patch only supplies a definition for linux futexes. A future update
>>>>could add support __ulock_wait/wake on Darwin, for instance.
>>>>
>>>>The members of __waiters were lifted to a new base class. The members
>>>>are now arranged such that overall sizeof(__waiters_base) fits in two
>>>>cache lines (on platforms with at least 64 byte cache lines). The
>>>>definition will also use destructive_interference_size for this if it
>>>>is available.
>>>>
>>>>The __waiters type is now specific to untimed waits. Timed waits have a
>>>>corresponding __timed_waiters type. Much of the code has been moved from
>>>>the previous __atomic_wait() free function to the __waiter_base template
>>>>and a __waiter derived type is provided to implement the un-timed wait
>>>>operations. A similar change has been made to the timed wait
>>>>implementation.
>>>>
>>>>The __atomic_spin code has been extended to take a spin policy which is
>>>>invoked after the initial busy wait loop. The default policy is to
>>>>return from the spin. The timed wait code adds a timed backoff spinning
>>>>policy. The code from <thread> which implements this_thread::sleep_for,
>>>>sleep_until has been moved to a new <bits/std_thread_sleep.h> header
>>>
>>>The commit msg wasn't updated for the latest round of changes
>>>(this_thread_sleep, __waiters_pool_base etc).
>>>
>>>>which allows the thread sleep code to be consumed without pulling in the
>>>>whole of <thread>.
>>>>
>>>>The entry points into the wait/notify code have been restructured to
>>>>support either -
>>>>* Testing the current value of the atomic stored at the given address
>>>>  and waiting on a notification.
>>>>* Applying a predicate to determine if the wait was satisfied.
>>>>The entry points were renamed to make it clear that the wait and wake
>>>>operations operate on addresses. The first variant takes the expected
>>>>value and a function which returns the current value that should be used
>>>>in comparison operations, these operations are named with a _v suffix
>>>>(e.g. 'value'). All atomic<_Tp> wait/notify operations use the first
>>>>variant. Barriers, latches and semaphores use the predicate variant.
>>>>
>>>>This change also centralizes what it means to compare values for the
>>>>purposes of atomic<T>::wait rather than scattering through individual
>>>>predicates.
>>>>
>>>>This change also centralizes the repetitive code which adjusts for
>>>>different user supplied clocks (this should be moved elsewhere
>>>>and all such adjustments should use a common implementation).
>>>>
>>>>This change also removes the hashing of the pointer and uses
>>>>the pointer value directly for indexing into the waiters table.
>>>>
>>>>libstdc++-v3/ChangeLog:
>>>>	* include/Makefile.am: Add new <bits/std_thread_sleep.h> header.
>>>
>>>The name needs updating to correspond to the latest version of the
>>>patch.
>>>
>>>>	* include/Makefile.in: Regenerate.
>>>>	* include/bits/atomic_base.h: Adjust all calls
>>>>	to __atomic_wait/__atomic_notify for new call signatures.
>>>>	* include/bits/atomic_wait.h: Extensive rewrite.
>>>>	* include/bits/atomic_timed_wait.h: Likewise.
>>>>	* include/bits/semaphore_base.h: Adjust all calls
>>>>	to __atomic_wait/__atomic_notify for new call signatures.
>>>>	* include/bits/this_thread_sleep.h: New file.
>>>>	* include/std/atomic: Likewise.
>>>>	* include/std/barrier: Likewise.
>>>>	* include/std/latch: Likewise.
>>>
>>>include/std/thread is missing from the changelog entry. You can use
>>>the 'git gcc-verify' alias to check your commit log will be accepted
>>>by the server-side hook:
>>>
>>>'gcc-verify' is aliased to '!f() { "`git rev-parse --show-toplevel`/contrib/gcc-changelog/git_check_commit.py" $@; } ; f'
>>>
>>>
>>>>	* testsuite/29_atomics/atomic/wait_notify/bool.cc: Simplify
>>>>	test.
>>>>	* testsuite/29_atomics/atomic/wait_notify/generic.cc: Likewise.
>>>>	* testsuite/29_atomics/atomic/wait_notify/pointers.cc: Likewise.
>>>>	* testsuite/29_atomics/atomic_flag/wait_notify.cc: Likewise.
>>>>	* testsuite/29_atomics/atomic_float/wait_notify.cc: Likewise.
>>>>	* testsuite/29_atomics/atomic_integral/wait_notify.cc: Likewise.
>>>>	* testsuite/29_atomics/atomic_ref/wait_notify.cc: Likewise.
>>>
>>>>-    struct __timed_waiters : __waiters
>>>>+    struct __timed_waiters : __waiter_pool_base
>>>
>>>Should this be __timed_waiter_pool for consistency with
>>>__waiter_pool_base and __waiter_pool?
>>>
>>>
>>>>-    inline void
>>>>-    __thread_relax() noexcept
>>>>-    {
>>>>-#if defined __i386__ || defined __x86_64__
>>>>-      __builtin_ia32_pause();
>>>>-#elif defined _GLIBCXX_USE_SCHED_YIELD
>>>>-      __gthread_yield();
>>>>-#endif
>>>>-    }
>>>>+    template<typename _Tp>
>>>>+      struct __waiter_base
>>>>+      {
>>>>+	using __waiter_type = _Tp;
>>>>
>>>>-    inline void
>>>>-    __thread_yield() noexcept
>>>>-    {
>>>>-#if defined _GLIBCXX_USE_SCHED_YIELD
>>>>-     __gthread_yield();
>>>>-#endif
>>>>-    }
>>>
>>>This chunk of the patch doesn't apply, because it's based on an old
>>>version of trunk (before r11-7248).
>>
>>I managed to bodge the patch so it applies, see attached.
>
>The attached patch is what I've pushed to trunk and gcc-11, which
>addresses all my comments from today.

And this disables some tests that are failing consistently (either on
all targets, or solaris). Also pushed to trunk and gcc-11.

I've also just seen this one on solaris:

WARNING: program timed out.
FAIL: 30_threads/barrier/arrive_and_wait.cc execution test

These need to be analysed and the tests re-enabled.



-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.txt
Type: text/x-patch
Size: 2253 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20210420/fabaf53b/attachment.bin>


More information about the Libstdc++ mailing list