From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 62C3A386185D for ; Tue, 20 Apr 2021 11:05:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 62C3A386185D Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-575-jRFQW58MP2OD0xmjiW_wXQ-1; Tue, 20 Apr 2021 07:04:58 -0400 X-MC-Unique: jRFQW58MP2OD0xmjiW_wXQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 27678107ACE4; Tue, 20 Apr 2021 11:04:57 +0000 (UTC) Received: from localhost (unknown [10.33.36.164]) by smtp.corp.redhat.com (Postfix) with ESMTP id B2D6C610F1; Tue, 20 Apr 2021 11:04:56 +0000 (UTC) Date: Tue, 20 Apr 2021 12:04:55 +0100 From: Jonathan Wakely To: Thomas Rodgers Cc: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org, trodgers@redhat.com, Thomas Rodgers Subject: Re: [PATCH] [libstdc++] Refactor/cleanup of atomic wait implementation Message-ID: <20210420110455.GF3008@redhat.com> References: <20210415124617.GN3008@redhat.com> <20210419192305.406972-1-rodgert@appliantology.com> MIME-Version: 1.0 In-Reply-To: <20210419192305.406972-1-rodgert@appliantology.com> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline X-Spam-Status: No, score=-8.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=unavailable autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Apr 2021 11:05:03 -0000 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).