From patchwork Thu May 13 23:35:35 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Thomas Rodgers X-Patchwork-Id: 1478256 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=sourceware.org; envelope-from=gcc-patches-bounces@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (4096-bit key; secure) header.d=kolabnow.com header.i=@kolabnow.com header.a=rsa-sha256 header.s=dkim20160331 header.b=NTFI6Tii; dkim-atps=neutral Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4Fh7MF3YFhz9sT6 for ; Fri, 14 May 2021 09:35:53 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 3119F3839C76; Thu, 13 May 2021 23:35:51 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mx.kolabnow.com (mx.kolabnow.com [95.128.36.42]) by sourceware.org (Postfix) with ESMTPS id 24E3B3848021; Thu, 13 May 2021 23:35:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 24E3B3848021 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=appliantology.com Authentication-Results: sourceware.org; spf=none smtp.mailfrom=rodgert@appliantology.com Received: from localhost (unknown [127.0.0.1]) by ext-mx-out001.mykolab.com (Postfix) with ESMTP id 81FF082F; Fri, 14 May 2021 01:35:45 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kolabnow.com; h= content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:date:subject:subject:from:from:received :received:received; s=dkim20160331; t=1620948944; x=1622763345; bh=00p2nuenUMeRDEKfCzSMtKZc0ZDmofwDA3ll3scuCa4=; b=NTFI6Tiie22d p0ND+nSMNHd+12Ao1mtowC7OjRb5nBR4u6XR6ylrOSNSl9RDgT65gccRXe+sayCo jQqV39wGVkr/AdIXc9vlupvaaqDLr0gOIX9AzXfgAcFFpE0QFZ9LwmssHQ2iGz/E LvOJ/9UQJqeqOCzncxyuQYUayd/OWA0rdJMXWZ/eTR2few28SQ7RsFHamO8KPTOf 3fdKMerQjXH+nwWN2XGcG04Hljoh2wr6PMPAxieplvPCNrwchczuZrqcyWTWyR22 ZtP4KEsc0n3vQsawIW0EHOGU+ca6dJOEMui2SImfLNNtJRIaVk4GgslzxjunYuD8 W9YcLDPoC3oGK8CJL4bIot4zhpa8RLCQASn4FTQ0UbRFDK+8S7FVYyH9COJJx5KD BfCHEmPtl0Z4G/MnkQLJd2Ozhu9kpsIGngcWxNPvFmJteOkMuUQQp902iud8rBLu S7mgBM0BrrSI/YNke9u3xT0YtVfsVgFIxDeW169Kd4M1o2wdsGrrmoboaNt3Mcb2 kz3+1HaFDCq6F1uUFjRs0LdVKMsc7hTV0rZyhs03LJDxKgeRL8x5+gsBCRxdCuW0 +60ZGHTaYZu4pS3kcEzyR+9SioiRU0+b9/gsyK6DrF/KpIz7N8ap2+iozT5wjoC1 MGJeHymOg7tPpfkjN03i/aMLThaHiYc= X-Virus-Scanned: amavisd-new at mykolab.com X-Spam-Score: -0.778 X-Spam-Level: X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, GIT_PATCH_0, KAM_SHORT, SPF_HELO_NONE, SPF_NONE, TXREP, URI_HEX autolearn=ham autolearn_force=no version=3.4.2 Received: from mx.kolabnow.com ([127.0.0.1]) by localhost (ext-mx-out001.mykolab.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Cxm3tneW7MEz; Fri, 14 May 2021 01:35:44 +0200 (CEST) Received: from int-mx003.mykolab.com (unknown [10.9.13.3]) by ext-mx-out001.mykolab.com (Postfix) with ESMTPS id 3B7AD825; Fri, 14 May 2021 01:35:41 +0200 (CEST) Received: from ext-subm001.mykolab.com (unknown [10.9.6.1]) by int-mx003.mykolab.com (Postfix) with ESMTPS id 66FD510053; Fri, 14 May 2021 01:35:41 +0200 (CEST) From: Thomas Rodgers To: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org Subject: [PATCH] libstdc++: Fix wrong thread waking on notify [PR100334] Date: Thu, 13 May 2021 16:35:35 -0700 Message-Id: <20210513233535.110937-1-rodgert@appliantology.com> In-Reply-To: <20210510182110.GR3008@redhat.com> References: <20210510182110.GR3008@redhat.com> MIME-Version: 1.0 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: trodgers@redhat.com, Thomas Rodgers Errors-To: gcc-patches-bounces@gcc.gnu.org Sender: "Gcc-patches" From: Thomas Rodgers libstdc++/ChangeLog: * include/bits/atomic_wait.h (__waiter::_M_do_wait_v): loop until value change observed. (__waiter_base::_M_a): Renamed member from _M_addr, changed type to uintptr_t. (__waiter_base::_S_wait_addr): Change return type to uinptr_t, sets LSB if 'laundering' the wait address (__waiter_base::_M_addr): New member, returns wait address, masking off LSB of _M_a. (__waiter_base::_M_laundered): New member, returns true if LSB of _M_a is set. (__waiter_base::_M_notify): Call _M_addr(), check _M_laundered() to determine whether to wake one or all. (__waiter_base::_M_do_spin_v): Call _M_addr(). (__waiter_base::_M_do_spin): Likewise. (__waiter::_M_do_wait_v): Likewise. (__waiter::_M_do_wait): Likewise. (__detail::__atomic_compare): Return true if call to __builtin_memcmp() == 0. (__waiter_base::_S_do_spin_v): Adjust predicate. * testsuite/29_atomics/atomic/wait_notify/100334.cc: New test. * include/bits/atomic_timed_wait.h (__timed_waiter::_M_do_wait_until_v): Call _M_addr(). (__timed_waiter::_M_do_wait_until): Likewise. --- libstdc++-v3/include/bits/atomic_timed_wait.h | 6 +- libstdc++-v3/include/bits/atomic_wait.h | 49 ++++++---- .../29_atomics/atomic/wait_notify/100334.cc | 94 +++++++++++++++++++ 3 files changed, 129 insertions(+), 20 deletions(-) create mode 100644 libstdc++-v3/testsuite/29_atomics/atomic/wait_notify/100334.cc diff --git a/libstdc++-v3/include/bits/atomic_timed_wait.h b/libstdc++-v3/include/bits/atomic_timed_wait.h index ec7ff51cdbc..5fe64fa2219 100644 --- a/libstdc++-v3/include/bits/atomic_timed_wait.h +++ b/libstdc++-v3/include/bits/atomic_timed_wait.h @@ -289,7 +289,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION if (_M_do_spin(__old, std::move(__vfn), __val, __timed_backoff_spin_policy(__atime))) return true; - return __base_type::_M_w._M_do_wait_until(__base_type::_M_addr, __val, __atime); + return __base_type::_M_w._M_do_wait_until(__base_type::_M_addr(), __val, __atime); } // returns true if wait ended before timeout @@ -304,7 +304,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __now = _Clock::now()) { if (__base_type::_M_w._M_do_wait_until( - __base_type::_M_addr, __val, __atime) + __base_type::_M_addr(), __val, __atime) && __pred()) return true; @@ -347,7 +347,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION auto __reltime = chrono::ceil<__wait_clock_t::duration>(__rtime); return __base_type::_M_w._M_do_wait_until( - __base_type::_M_addr, + __base_type::_M_addr(), __val, chrono::steady_clock::now() + __reltime); } diff --git a/libstdc++-v3/include/bits/atomic_wait.h b/libstdc++-v3/include/bits/atomic_wait.h index 984ed70f16c..06ebcc7bce3 100644 --- a/libstdc++-v3/include/bits/atomic_wait.h +++ b/libstdc++-v3/include/bits/atomic_wait.h @@ -181,11 +181,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return false; } + // return true if equal template bool __atomic_compare(const _Tp& __a, const _Tp& __b) { // TODO make this do the correct padding bit ignoring comparison - return __builtin_memcmp(&__a, &__b, sizeof(_Tp)) != 0; + return __builtin_memcmp(&__a, &__b, sizeof(_Tp)) == 0; } struct __waiter_pool_base @@ -276,16 +277,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION using __waiter_type = _Tp; __waiter_type& _M_w; - __platform_wait_t* _M_addr; + uintptr_t _M_a; template - static __platform_wait_t* + static uintptr_t _S_wait_addr(const _Up* __a, __platform_wait_t* __b) { if constexpr (__platform_wait_uses_type<_Up>) - return reinterpret_cast<__platform_wait_t*>(const_cast<_Up*>(__a)); + return reinterpret_cast(const_cast<_Up*>(__a)); else - return __b; + return reinterpret_cast(__b) | 0x1; } static __waiter_type& @@ -299,16 +300,25 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template explicit __waiter_base(const _Up* __addr) noexcept : _M_w(_S_for(__addr)) - , _M_addr(_S_wait_addr(__addr, &_M_w._M_ver)) - { - } + , _M_a(_S_wait_addr(__addr, &_M_w._M_ver)) + { } + + __platform_wait_t* + _M_addr() const noexcept + { return reinterpret_cast<__platform_wait_t*>(_M_a & (-1 << 1)); } + + bool + _M_laundered() const + { return _M_a & 0x1; } void _M_notify(bool __all, bool __bare = false) { - if (_M_addr == &_M_w._M_ver) - __atomic_fetch_add(_M_addr, 1, __ATOMIC_ACQ_REL); - _M_w._M_notify(_M_addr, __all, __bare); + const auto __addr = _M_addr(); + const auto __laundered = _M_laundered(); + if (__laundered) + __atomic_fetch_add(__addr, 1, __ATOMIC_ACQ_REL); + _M_w._M_notify(__addr, (__laundered ? true : __all), __bare); } template) { @@ -339,7 +349,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_do_spin_v(const _Up& __old, _ValFn __vfn, __platform_wait_t& __val, _Spin __spin = _Spin{ }) - { return _S_do_spin_v(_M_addr, __old, __vfn, __val, __spin); } + { return _S_do_spin_v(_M_addr(), __old, __vfn, __val, __spin); } template @@ -358,7 +368,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION bool _M_do_spin(_Pred __pred, __platform_wait_t& __val, _Spin __spin = _Spin{ }) - { return _S_do_spin(_M_addr, __pred, __val, __spin); } + { return _S_do_spin(_M_addr(), __pred, __val, __spin); } }; template @@ -387,7 +397,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __platform_wait_t __val; if (__base_type::_M_do_spin_v(__old, __vfn, __val)) return; - __base_type::_M_w._M_do_wait(__base_type::_M_addr, __val); + + do + { + __base_type::_M_w._M_do_wait(__base_type::_M_addr(), __val); + } + while (__detail::__atomic_compare(__old, __vfn())); } template @@ -399,7 +414,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __platform_wait_t __val; if (__base_type::_M_do_spin(__pred, __val)) return; - __base_type::_M_w._M_do_wait(__base_type::_M_addr, __val); + __base_type::_M_w._M_do_wait(__base_type::_M_addr(), __val); } while (!__pred()); } @@ -452,7 +467,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __atomic_notify_address(const _Tp* __addr, bool __all) noexcept { __detail::__bare_wait __w(__addr); - __w._M_notify(__all, true); + __w._M_notify(__all); } // This call is to be used by atomic types which track contention externally diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/wait_notify/100334.cc b/libstdc++-v3/testsuite/29_atomics/atomic/wait_notify/100334.cc new file mode 100644 index 00000000000..3e63eca42fa --- /dev/null +++ b/libstdc++-v3/testsuite/29_atomics/atomic/wait_notify/100334.cc @@ -0,0 +1,94 @@ +// { dg-options "-std=gnu++2a" } +// { dg-do run { target c++2a } } +// { dg-require-gthreads "" } +// { dg-additional-options "-pthread" { target pthread } } +// { dg-add-options libatomic } + +// Copyright (C) 2021 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// . + +#include +#include + +#include + +template +struct atomics_sharing_same_waiter +{ + std::atomic tmp[49 * 4] = {}; + std::atomic* a[4] = { + { &tmp[0] }, + { &tmp[16 * 4] }, + { &tmp[32 * 4] }, + { &tmp[48 * 4] } + }; +}; + +constexpr unsigned key(void * a) +{ + constexpr uintptr_t ct = 16; + return (uintptr_t(a) >> 2) % ct; +} + +int +main() +{ + // all atomic share the same waiter +// atomics_sharing_same_waiter atomics; + atomics_sharing_same_waiter atomics; + for (auto& atom : atomics.a) + { + atom->store(0); + } + + auto a = &std::__detail::__waiter_pool_base::_S_for(reinterpret_cast(atomics.a[0])); + auto b = &std::__detail::__waiter_pool_base::_S_for(reinterpret_cast(atomics.a[1])); + VERIFY( a == b ); + + auto fut0 = std::async(std::launch::async, [&] { atomics.a[0]->wait(0); }); + auto fut1 = std::async(std::launch::async, [&] { atomics.a[1]->wait(0); }); + auto fut2 = std::async(std::launch::async, [&] { atomics.a[2]->wait(0); }); + auto fut3 = std::async(std::launch::async, [&] { atomics.a[3]->wait(0); }); + + // make sure the all threads already await + std::this_thread::sleep_for(std::chrono::milliseconds{100}); + + atomics.a[2]->store(1); + atomics.a[2]->notify_one(); + + VERIFY(std::future_status::timeout == fut0.wait_for(std::chrono::milliseconds{100})); + VERIFY(atomics.a[0]->load() == 0); + + VERIFY(std::future_status::timeout == fut1.wait_for(std::chrono::milliseconds{100})); + VERIFY(atomics.a[1]->load() == 0); + + VERIFY(std::future_status::ready == fut2.wait_for(std::chrono::milliseconds{100})); + VERIFY(atomics.a[2]->load() == 1); + + VERIFY(std::future_status::timeout == fut3.wait_for(std::chrono::milliseconds{100})); + VERIFY(atomics.a[3]->load() == 0); + + atomics.a[0]->store(1); + atomics.a[0]->notify_one(); + atomics.a[1]->store(1); + atomics.a[1]->notify_one(); + atomics.a[3]->store(1); + atomics.a[3]->notify_one(); + + return 0; +} +