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 >>> >>>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 which implements this_thread::sleep_for, >>>sleep_until has been moved to a new 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 . >>> >>>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::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 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 >>>+ 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.