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

Jonathan Wakely jwakely@redhat.com
Tue Apr 20 11:41:09 GMT 2021


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.

With this applied I see:

/home/jwakely/src/gcc/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_until.cc:64: test02()::<lambda()>: Assertion '!s.try_acquire_until(at)' failed.
FAIL: 30_threads/semaphore/try_acquire_until.cc execution test

and:

WARNING: program timed out.
FAIL: 30_threads/stop_token/stop_callback/destroy.cc execution test

The stop_callback/destroy.cc test is hanging here:

(gdb) thread apply all bt

Thread 3 (Thread 0x3fffabacf190 (LWP 84029)):
#0  0x00003fffabcd4288 in nanosleep () from /lib64/libpthread.so.0
#1  0x000000001000141c in std::thread::_State_impl<std::thread::_Invoker<std::tuple<test01()::{lambda()#1}> > >::_M_run() ()
#2  0x00003fffabf4f760 in execute_native_thread_routine () from /home/jwakely/build/powerpc64le-unknown-linux-gnu/./libstdc++-v3/src/.libs/libstdc++.so.6
#3  0x00003fffabcc8cd4 in start_thread () from /lib64/libpthread.so.0
#4  0x00003fffabbf7e14 in clone () from /lib64/libc.so.6

Thread 2 (Thread 0x3fffab2bf190 (LWP 84030)):
#0  0x00003fffabbef914 in syscall () from /lib64/libc.so.6
#1  0x00000000100017e8 in void std::__atomic_wait_address_bare<std::__atomic_semaphore::_M_acquire()::{lambda()#1}>(int const*, std::__atomic_semaphore::_M_acquire()::{lambda()#1}) ()
#2  0x0000000010001a84 in std::stop_callback<F>::~stop_callback() ()
#3  0x0000000010001d1c in std::thread::_State_impl<std::thread::_Invoker<std::tuple<test01()::{lambda()#2}> > >::_M_run() ()
#4  0x00003fffabf4f760 in execute_native_thread_routine () from /home/jwakely/build/powerpc64le-unknown-linux-gnu/./libstdc++-v3/src/.libs/libstdc++.so.6
#5  0x00003fffabcc8cd4 in start_thread () from /lib64/libpthread.so.0
#6  0x00003fffabbf7e14 in clone () from /lib64/libc.so.6

Thread 1 (Thread 0x3fffac1754d0 (LWP 84025)):
#0  0x00003fffabcca2a8 in pthread_join () from /lib64/libpthread.so.0
#1  0x00003fffabf4fac0 in std::thread::join() () from /home/jwakely/build/powerpc64le-unknown-linux-gnu/./libstdc++-v3/src/.libs/libstdc++.so.6
#2  0x0000000010001f58 in test01() ()
#3  0x0000000010000e58 in main ()

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


More information about the Libstdc++ mailing list