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 [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id CEBBD3858C27 for ; Tue, 20 Apr 2021 09:18:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org CEBBD3858C27 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-232-YvAQoGyoP3GK0PlfwdSvBA-1; Tue, 20 Apr 2021 05:18:17 -0400 X-MC-Unique: YvAQoGyoP3GK0PlfwdSvBA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 9A1BF343A0; Tue, 20 Apr 2021 09:18:16 +0000 (UTC) Received: from localhost (unknown [10.33.36.164]) by smtp.corp.redhat.com (Postfix) with ESMTP id 10E866A045; Tue, 20 Apr 2021 09:18:15 +0000 (UTC) Date: Tue, 20 Apr 2021 10:18:15 +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: 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.13 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=ham 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 09:18:25 -0000 On 19/04/21 12:23 -0700, Thomas Rodgers wrote: > namespace __detail > { >- using __platform_wait_t = int; >- >- constexpr auto __atomic_spin_count_1 = 16; >- constexpr auto __atomic_spin_count_2 = 12; >- >- template >- inline constexpr bool __platform_wait_uses_type > #ifdef _GLIBCXX_HAVE_LINUX_FUTEX >- = is_same_v, __platform_wait_t>; >+ using __platform_wait_t = int; > #else >- = false; >+ using __platform_wait_t = uint64_t; >+#endif >+ static constexpr size_t __platform_wait_alignment >+ = alignof(__platform_wait_t); The value of this constant can't be alignof(__platform_wait_t). As discussed, a futex always needs 4-byte alignment, but on at least one target that GCC supports, alignof(int) == 2. >+ } // namespace __detail >+ >+ template >+ inline constexpr bool __platform_wait_uses_type >+#ifdef _GLIBCXX_HAVE_PLATFORM_WAIT >+ = is_scalar_v<_Tp> >+ && ((sizeof(_Tp) == sizeof(__detail::__platform_wait_t)) >+ && (alignof(_Tp*) >= alignof(__detail::__platform_wait_t))); Now that we have the __platform_wait_alignment it should be used here (so that when we fix the constant, this gets fixed too). >+#else >+ = false; > #endif > >+ namespace __detail >+ { > #ifdef _GLIBCXX_HAVE_LINUX_FUTEX >+#define _GLIBCXX_HAVE_PLATFORM_WAIT 1 > enum class __futex_wait_flags : int > { > #ifdef _GLIBCXX_HAVE_LINUX_FUTEX_PRIVATE >+ >+ static __waiter_type& >+ _S_for(const void* __addr) >+ { >+ static_assert(sizeof(__waiter_type) == sizeof(__waiter_pool_base)); >+ auto& res = __waiter_pool_base::_S_for(__addr); >+ return reinterpret_cast<__waiter_type&>(res); >+ } Nit: this is still indented as if it were a function template. > : _M_a(__expected) { } >@@ -73,8 +73,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > _GLIBCXX_ALWAYS_INLINE void > wait() const noexcept > { >- auto const __old = __atomic_impl::load(&_M_a, memory_order::acquire); >- std::__atomic_wait(&_M_a, __old, [this] { return this->try_wait(); }); >+ auto const __pred = [this] { return this->try_wait(); }; >+ std::__atomic_wait_address(&_M_a, __pred); > } > > _GLIBCXX_ALWAYS_INLINE void >@@ -85,7 +85,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > > private: >- alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; >+ alignas(__alignof__(__detail::__platform_wait_t)) __detail::__platform_wait_t _M_a; This should use the new constant too.