From patchwork Wed Sep 8 02:52:34 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Malte Skarupke X-Patchwork-Id: 44900 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 646E83851C0B for ; Wed, 8 Sep 2021 02:56:50 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 646E83851C0B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1631069810; bh=R3032T+wa9ZcghJ639SatfoLidRloSyQu8LF79G33zo=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=ViqpsNNALYcxxhRxz8dj7JxRtmN+heKEtCY0Q5PfYVzJjiyzUjEXzenJH0T08F5sm ynSG1CAyWJpUUV+wpRqDma3w/iMrHyE+dNPkWk2/Dn5OHxfSGCr7vTZ3RWYh8GK9tv Wbs2z/9kBNSqEhM4DTlzmRYR316SLMirL7ltjDNw= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mout.web.de (mout.web.de [212.227.17.11]) by sourceware.org (Postfix) with ESMTPS id 5B317385E82E for ; Wed, 8 Sep 2021 02:54:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5B317385E82E X-UI-Sender-Class: c548c8c5-30a9-4db5-a2e7-cb6cb037b8f9 Received: from auth2-smtp.messagingengine.com ([66.111.4.228]) by smtp.web.de (mrweb106 [213.165.67.124]) with ESMTPSA (Nemesis) id 1MJFdB-1me6SU3f35-00KO90; Wed, 08 Sep 2021 04:54:14 +0200 Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailauth.nyi.internal (Postfix) with ESMTP id 7FBDB27C005B; Tue, 7 Sep 2021 22:54:12 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute6.internal (MEProxy); Tue, 07 Sep 2021 22:54:12 -0400 X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrudefiedgieduucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpefhvffufffkofgjfhgggfestdekre dtredttdenucfhrhhomhepofgrlhhtvgcuufhkrghruhhpkhgvuceomhgrlhhtvghskhgr rhhuphhkvgesfigvsgdruggvqeenucggtffrrghtthgvrhhnpefggeelleeihfefleetie eludejkefgjefhueeitdeltdeikefgvdffjedtieffteenucffohhmrghinhepphhrohgs rggslhihuggrnhgtvgdrtghomhenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmh epmhgrihhlfhhrohhmpehmrghlthgvshhkrghruhhpkhgvodhmvghsmhhtphgruhhthhhp vghrshhonhgrlhhithihqddutddujedtfedvleeiqdduuddvgedvkeeiledqmhgrlhhtvg hskhgrrhhuphhkvgeppeifvggsrdguvgesfhgrshhtmhgrihhlrdhfmh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 7 Sep 2021 22:54:11 -0400 (EDT) To: libc-alpha@sourceware.org Subject: [PATCH v2 1/6] nptl: Fix pthread_cond_signal missing a sleeper (#BZ 25847) Date: Tue, 7 Sep 2021 22:52:34 -0400 Message-Id: <20210908025239.1326480-2-malteskarupke@web.de> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20210908025239.1326480-1-malteskarupke@web.de> References: <20210908025239.1326480-1-malteskarupke@web.de> MIME-Version: 1.0 X-Provags-ID: V03:K1:sBGuHLRzlzZVfr0ZhO8TlXDMBy34pMaW0m2qt19YGradlVyCB5e GQhCv2/z3o1mZ3px/vUb0HjZ7h/E0AMid93F2L2OQjK5Q+CdUZxL+hiI5qCzYNp1eP4iwgy Gux3ddfd9qt7KmJDLIK4z1ktLg6eCOi/dHDvBTmoczwNhJMRd5rr0z2g3f+xl7sxuYJC+sn dlt3WPx/zxRAobuMT6I2Q== X-UI-Out-Filterresults: notjunk:1;V03:K0:7t6BBYxn17U=:AWLphKXYuEOpEb82kd5RJA WUziCZ9n27fKkOyy6Yinv87dRHB81U6gQw+wp2Gj+SUpYXKn7ZH/YkeO8zOx+sV/5nAE/TY3D SntT5g1BZq2/y5I+8Uvr3dxQdj9J1+Qz6Pbb1CIZCEKd31VZHcatqrGo8G0f9zZoHwlqcrlfd P2RSboTjRaRVjZFUA911BITMxPH85w9v+2zQfTCe9IvTTkazc0/w7JPe8hU9cB1jKSHzLtnPr ti5+RBE5Nfu2PYmraq+5rQ1hRKVNEoJnQzcsOdHoTWjfctxiyy8EqqJqXPewGzzOvTxph6s0/ ZpZezW9Pg++ubHGpXzdkVIyiWlu/7FYDDOwK1GBbxZ52Oe9qLG3IUz1kF296EiIjowrm1CnIR ONwayQlP8dCTJoEaf/ciiUOU1N551n2Kg7uX2QuaWLPnq5k0ZbA3EUrovG+3/Z/8OKjuRIdHZ 65RC23nJReLhVlORa3Y9t+OTFLBnnMUJjDBtKFFdl+zSFsoWiwuj0WO2EyK7IFj8gZXcxS3RZ UGiNN07jbOy9whqrj3dwRZyZeLh/QrbvQsUF6LFPTvmcrZLgV3Y1jOQ4vU3C6he5ijVcIxt0O A+owQU2I67zds12o7XIfdT8trj3jpNRVzJ7KMEcOJIoAnQDWl59U70PSyGHopE8ErUsdCD1k3 vhq4hu3t0pkhUDm2pR6jQtCrN+9Xt1MAt/yNQhdRPg0daMayBw0pNK5Gj/yrt7M/Nd2vaA6kR hFbc07U6oX5MAMhLCcpd/mnwO+yKKEdujD6Zi6rgX9KXtmnIg6WjcLR+mc8w6IcOZlqE6YYL3 X3UpoXo1A7u4mkFXpl4WEaHBQv5QHHzWAz8ovYZbce1jUIzwvCECBG20szXoX7vhCgV2GobgC KPp/8W2RYO9uQHZy4q4jDyeENg+RNMcst8Pm2CHvwyA7EAOalp2Z/O8HfZcaxytraI/lnMnpY 63Sj4M52uo3OJRQZMUb0qkGjmEjjLnhWTKXQtWApHrilzh8E+fR2aHSuD+KM+l9nUoRC9A7WO vvq+4PQVwm0ANC85Yx6/fQ5hOqzCFmdxDq/SC1/igX2/9Fyjl/xrM84gCTyWTUGPnjN52D5Bt zbUzQ7ko1IMU9w= X-Spam-Status: No, score=-12.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Malte Skarupke via Libc-alpha From: Malte Skarupke Reply-To: Malte Skarupke Cc: Malte Skarupke , triegel@redhat.com, malteskarupke@fastmail.fm Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" There was a rare bug in pthread_cond_wait's handling of the case when a signal was stolen because a waiter took a long time to leave pthread_cond_wait. I wrote about the bug here: https://probablydance.com/2020/10/31/using-tla-in-the-real-world-to-understand-a-glibc-bug/ The bug was subtle and only happened in an edge-case of an edge-case so rather than fixing it, I decided to remove the outer edge-case: By broadening the scope of grefs, stealing of signals becomes impossible. A signaling thread will always wait for all waiters to leave pthread_cond_wait before closing a group, so now no waiter from the past can come back and steal a signal from a future group. This is patch 1/6, it contains the minimal amount of changes necessary to fix the bug. This leads to an unnecessary amount of atomic operations, but the other patches in this series will undo most of that damage. --- nptl/pthread_cond_wait.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) -- 2.25.1 diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c index dc8c511f1a..cb674e2634 100644 --- a/nptl/pthread_cond_wait.c +++ b/nptl/pthread_cond_wait.c @@ -409,6 +409,12 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, we only need to synchronize when decrementing the reference count. */ unsigned int flags = atomic_fetch_add_relaxed (&cond->__data.__wrefs, 8); int private = __condvar_get_private (flags); + /* Acquire a group reference and use acquire MO for that so that we + synchronize with the dummy read-modify-write in + __condvar_quiesce_and_switch_g1 if we read from the same group. This will + make us see the closed flag on __g_signals that designates a concurrent + attempt to reuse the group's slot. */ + atomic_fetch_add_acquire (cond->__data.__g_refs + g, 2); /* Now that we are registered as a waiter, we can release the mutex. Waiting on the condvar must be atomic with releasing the mutex, so if @@ -420,6 +426,7 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, err = __pthread_mutex_unlock_usercnt (mutex, 0); if (__glibc_unlikely (err != 0)) { + __condvar_dec_grefs (cond, g, private); __condvar_cancel_waiting (cond, seq, g, private); __condvar_confirm_wakeup (cond, private); return err; @@ -471,24 +478,14 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, break; /* No signals available after spinning, so prepare to block. - We first acquire a group reference and use acquire MO for that so - that we synchronize with the dummy read-modify-write in - __condvar_quiesce_and_switch_g1 if we read from that. In turn, - in this case this will make us see the closed flag on __g_signals - that designates a concurrent attempt to reuse the group's slot. - We use acquire MO for the __g_signals check to make the - __g1_start check work (see spinning above). - Note that the group reference acquisition will not mask the - release MO when decrementing the reference count because we use - an atomic read-modify-write operation and thus extend the release - sequence. */ - atomic_fetch_add_acquire (cond->__data.__g_refs + g, 2); + First check the closed flag on __g_signals that designates a + concurrent attempt to reuse the group's slot. We use acquire MO for + the __g_signals check to make the __g1_start check work (see + above). */ if (((atomic_load_acquire (cond->__data.__g_signals + g) & 1) != 0) || (seq < (__condvar_load_g1_start_relaxed (cond) >> 1))) { - /* Our group is closed. Wake up any signalers that might be - waiting. */ - __condvar_dec_grefs (cond, g, private); + /* Our group is closed. */ goto done; } @@ -516,10 +513,8 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, the lock during cancellation is not possible. */ __condvar_cancel_waiting (cond, seq, g, private); result = err; - goto done; + goto acquire_lock; } - else - __condvar_dec_grefs (cond, g, private); /* Reload signals. See above for MO. */ signals = atomic_load_acquire (cond->__data.__g_signals + g); @@ -598,11 +593,13 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, done: - /* Confirm that we have been woken. We do that before acquiring the mutex - to allow for execution of pthread_cond_destroy while having acquired the - mutex. */ + /* Decrement group reference count and confirm that we have been woken. We do + that before acquiring the mutex to allow for execution of + pthread_cond_destroy while having acquired the mutex. */ + __condvar_dec_grefs (cond, g, private); __condvar_confirm_wakeup (cond, private); + acquire_lock: /* Woken up; now re-acquire the mutex. If this doesn't fail, return RESULT, which is set to ETIMEDOUT if a timeout occured, or zero otherwise. */ err = __pthread_mutex_cond_lock (mutex); From patchwork Wed Sep 8 02:52:35 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Malte Skarupke X-Patchwork-Id: 44898 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 498953858C60 for ; Wed, 8 Sep 2021 02:55:25 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 498953858C60 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1631069725; bh=rbq2MrBJrpMsh9WMIq7uknKC68E2adVHYRHjWtS9nus=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=AoWcQyJk0OTykVnbVIlzNXlJvB/1K/q7PabrCNIstjlmHPp3ezTYTq3dppaZPHoT2 vRWVHr1jYvToVh5VXvFktklmIHk9jILC9NOAecLTJEH2luDmSF/9OLWn6wEKjkIXSl NwBeEe3D/moKwWtUPPuzUcYkhSyphkrvtpBOjibk= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mout.web.de (mout.web.de [212.227.15.14]) by sourceware.org (Postfix) with ESMTPS id ED5203858D35 for ; Wed, 8 Sep 2021 02:54:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org ED5203858D35 X-UI-Sender-Class: c548c8c5-30a9-4db5-a2e7-cb6cb037b8f9 Received: from auth2-smtp.messagingengine.com ([66.111.4.228]) by smtp.web.de (mrweb006 [213.165.67.108]) with ESMTPSA (Nemesis) id 1M8Bvz-1mKFDZ30k7-005O2I; Wed, 08 Sep 2021 04:54:14 +0200 Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailauth.nyi.internal (Postfix) with ESMTP id 78B0B27C0061; Tue, 7 Sep 2021 22:54:13 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute4.internal (MEProxy); Tue, 07 Sep 2021 22:54:13 -0400 X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrudefiedgiedvucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpefhvffufffkofgjfhgggfestdekre dtredttdenucfhrhhomhepofgrlhhtvgcuufhkrghruhhpkhgvuceomhgrlhhtvghskhgr rhhuphhkvgesfigvsgdruggvqeenucggtffrrghtthgvrhhnpeevgfehffefffehleelie fftddvudffkedtkeduheeiueeugffgvdejgeduvdfgjeenucevlhhushhtvghrufhiiigv pedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehmrghlthgvshhkrghruhhpkhgvodhmvg hsmhhtphgruhhthhhpvghrshhonhgrlhhithihqddutddujedtfedvleeiqdduuddvgedv keeiledqmhgrlhhtvghskhgrrhhuphhkvgeppeifvggsrdguvgesfhgrshhtmhgrihhlrd hfmh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 7 Sep 2021 22:54:12 -0400 (EDT) To: libc-alpha@sourceware.org Subject: [PATCH v2 2/6] nptl: Remove the signal-stealing code. It is no longer needed. Date: Tue, 7 Sep 2021 22:52:35 -0400 Message-Id: <20210908025239.1326480-3-malteskarupke@web.de> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20210908025239.1326480-1-malteskarupke@web.de> References: <20210908025239.1326480-1-malteskarupke@web.de> MIME-Version: 1.0 X-Provags-ID: V03:K1:pJMH/cMtfbFa5WtmOU+vbWgdLeIScgAZ8vbg3Aar/dr6RVTmH3D pWWw+uFW5MlDjYSnauEJNTnf/yj9zfFr577khZSU+yhAFDzma7kw3SjwFnJE46f2g70Wwj0 3t19HsifLKF38PIiU+BkoQVqiwRCxgOAxGysGebZDlOxmLxEDyFUO/vi4hhboKPpgOnc9NS ZzMYmYwroPieORJU8elpg== X-UI-Out-Filterresults: notjunk:1;V03:K0:rbZkGzESHdc=:jDV4Br3mZz7ohblhkue8Qf xMJCJzTNsXQUGRyvMHHIcV9Se4IfrjvYxayEcUA56nzU8ycRU+sEIc4sTJaRxYYUlgUrajPGA t/4rPLf412fRMAF9IwMYGviGFT581augwSHfx8QWU6x31Dt+URNnw72zGPVzdP69eiVOR0zN6 CPbi6rPdpujRACPBW5PuAhmKGOPTo4Njb3T3uP7vxkO/isDgP4ymLE9wpx6GgQMniov2Py0Wj U4GrnsGf2ShLqErT6puiExzerdO1bQDNK769jfXTv5YPPN+Mx7tAz+myTTvcLcqr4NR/j1Aq/ iLQP0C6PhEdi/IEStxz17KAAlCDoiuU+2TReToHJ0Tc8/s6ISmKJPxG1LhzieBA1kobeuLlWp h0tIM60p1wUrdKma4dWlBOaOSMzUmfn9glQbhx6eT4JsFlE8PVW1q4Xe+cBiIMso3K1RtSVbM 6Fzg8/Zx8LvK5wTC+os7MpqPl8rj8Z0yGhR1D7nNxJqt890j6j8uMsc6NA+EBdb4vAR7nqxMV aqup+sG46iXAsg9HoPARIjDRDWbK0wtxNqSxRufo1mb+hLyGkmUeG/k09EUfbMAebIjhF91Ml 6f6fEWDCokY6brkE+QLpj0IcZQIgXktKha0sFBwqRap0kNFKcNTiKroKq44akjLh+lOZBV1KR Mddm/oIscTUtOWhPKpeRd/TYRMM0C5p/FvKQfYppJxXeE35Jj3MZ9798LwCqqZp6uFpgj0Tcs /wE7S6S0mMm1t9WaxAp5k9qXSjoqKLXbVBNfxdNpmCX/nimRbcrJ/PmZ1hdGTEd3obtKl8e9i HiZ9fdfGHA5cmcnaYWI0NpvSsXRFxuDSqIuP5s/ym4Is1qHgFmlCLYMYx81t1/L/4RWPZCuEH hRAWoQjeiGQDy091R7Oq7gQA3fF5MYkwrb8sOF/Mvljm2NqIyNOF6xjMqHucZF51YcDCyeItl 5h9AKrTMtpCciZWYlwDaR53y6iP6FUwzDqQZ89P974xJpn9PuVq1LNX8+6P9rAropi/j7ImbA guv/lS5Q7amoBeKWVmlFXlxViIJjQaRafqa39xlpS/2Ejbf4EOuI59sfZC4RAnu9IoAG53CZc BKbdOQWFEUqthkXFlPa3qsaaOFOLf3oKCuSb7cjVtY3gkYcICHyawsB8g== X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Malte Skarupke via Libc-alpha From: Malte Skarupke Reply-To: Malte Skarupke Cc: Malte Skarupke , triegel@redhat.com, malteskarupke@fastmail.fm Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" After my last change, stealing of signals can no longer happen. This patch removes the code that handled the case when a signal was stolen. --- nptl/pthread_cond_wait.c | 63 ---------------------------------------- 1 file changed, 63 deletions(-) -- 2.25.1 diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c index cb674e2634..fb31090e26 100644 --- a/nptl/pthread_cond_wait.c +++ b/nptl/pthread_cond_wait.c @@ -528,69 +528,6 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, while (!atomic_compare_exchange_weak_acquire (cond->__data.__g_signals + g, &signals, signals - 2)); - /* We consumed a signal but we could have consumed from a more recent group - that aliased with ours due to being in the same group slot. If this - might be the case our group must be closed as visible through - __g1_start. */ - uint64_t g1_start = __condvar_load_g1_start_relaxed (cond); - if (seq < (g1_start >> 1)) - { - /* We potentially stole a signal from a more recent group but we do not - know which group we really consumed from. - We do not care about groups older than current G1 because they are - closed; we could have stolen from these, but then we just add a - spurious wake-up for the current groups. - We will never steal a signal from current G2 that was really intended - for G2 because G2 never receives signals (until it becomes G1). We - could have stolen a signal from G2 that was conservatively added by a - previous waiter that also thought it stole a signal -- but given that - that signal was added unnecessarily, it's not a problem if we steal - it. - Thus, the remaining case is that we could have stolen from the current - G1, where "current" means the __g1_start value we observed. However, - if the current G1 does not have the same slot index as we do, we did - not steal from it and do not need to undo that. This is the reason - for putting a bit with G2's index into__g1_start as well. */ - if (((g1_start & 1) ^ 1) == g) - { - /* We have to conservatively undo our potential mistake of stealing - a signal. We can stop trying to do that when the current G1 - changes because other spinning waiters will notice this too and - __condvar_quiesce_and_switch_g1 has checked that there are no - futex waiters anymore before switching G1. - Relaxed MO is fine for the __g1_start load because we need to - merely be able to observe this fact and not have to observe - something else as well. - ??? Would it help to spin for a little while to see whether the - current G1 gets closed? This might be worthwhile if the group is - small or close to being closed. */ - unsigned int s = atomic_load_relaxed (cond->__data.__g_signals + g); - while (__condvar_load_g1_start_relaxed (cond) == g1_start) - { - /* Try to add a signal. We don't need to acquire the lock - because at worst we can cause a spurious wake-up. If the - group is in the process of being closed (LSB is true), this - has an effect similar to us adding a signal. */ - if (((s & 1) != 0) - || atomic_compare_exchange_weak_relaxed - (cond->__data.__g_signals + g, &s, s + 2)) - { - /* If we added a signal, we also need to add a wake-up on - the futex. We also need to do that if we skipped adding - a signal because the group is being closed because - while __condvar_quiesce_and_switch_g1 could have closed - the group, it might stil be waiting for futex waiters to - leave (and one of those waiters might be the one we stole - the signal from, which cause it to block using the - futex). */ - futex_wake (cond->__data.__g_signals + g, 1, private); - break; - } - /* TODO Back off. */ - } - } - } - done: /* Decrement group reference count and confirm that we have been woken. We do From patchwork Wed Sep 8 02:52:36 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Malte Skarupke X-Patchwork-Id: 44901 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 598E238515F6 for ; Wed, 8 Sep 2021 02:57:33 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 598E238515F6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1631069853; bh=vJennULMqZ8Fd8YU2GNd+zD3K5v4nH16Ct7L505RX2Q=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=Ycgy16QX58QtsNkhhoqwt2FQYGfeqG6q/MTAbUSxW7tFl0krbHTEN/QrwfPAWmD9S KGVeOYmxf0ifDHRt4OjOjZNkZ9jBFUGTVJgQD7kKWqNDLZrX4PMvSLBhvEYwj3vJ9M tdjHj5fes0HiXkMdgFv1w2R1H3HIldvXgd4LQp9w= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mout.web.de (mout.web.de [212.227.15.3]) by sourceware.org (Postfix) with ESMTPS id 03C613858C60 for ; Wed, 8 Sep 2021 02:54:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 03C613858C60 X-UI-Sender-Class: c548c8c5-30a9-4db5-a2e7-cb6cb037b8f9 Received: from auth2-smtp.messagingengine.com ([66.111.4.228]) by smtp.web.de (mrweb002 [213.165.67.108]) with ESMTPSA (Nemesis) id 0LjJWR-1mwrlI43hv-00dVuZ; Wed, 08 Sep 2021 04:54:16 +0200 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailauth.nyi.internal (Postfix) with ESMTP id 539A627C005B; Tue, 7 Sep 2021 22:54:14 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Tue, 07 Sep 2021 22:54:14 -0400 X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrudefiedgiedvucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpefhvffufffkofgjfhgggfestdekre dtredttdenucfhrhhomhepofgrlhhtvgcuufhkrghruhhpkhgvuceomhgrlhhtvghskhgr rhhuphhkvgesfigvsgdruggvqeenucggtffrrghtthgvrhhnpeevgfehffefffehleelie fftddvudffkedtkeduheeiueeugffgvdejgeduvdfgjeenucevlhhushhtvghrufhiiigv pedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehmrghlthgvshhkrghruhhpkhgvodhmvg hsmhhtphgruhhthhhpvghrshhonhgrlhhithihqddutddujedtfedvleeiqdduuddvgedv keeiledqmhgrlhhtvghskhgrrhhuphhkvgeppeifvggsrdguvgesfhgrshhtmhgrihhlrd hfmh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 7 Sep 2021 22:54:13 -0400 (EDT) To: libc-alpha@sourceware.org Subject: [PATCH v2 3/6] nptl: Optimization by not incrementing wrefs in pthread_cond_wait Date: Tue, 7 Sep 2021 22:52:36 -0400 Message-Id: <20210908025239.1326480-4-malteskarupke@web.de> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20210908025239.1326480-1-malteskarupke@web.de> References: <20210908025239.1326480-1-malteskarupke@web.de> MIME-Version: 1.0 X-Provags-ID: V03:K1:+KHPHT+0WVnRUBt4McvW5ktscJ9zjoXNKHKaYf8LDTzRSft0zgS DhqzHvJKND2l9sMRJeegkhMKZLwByNkkZExz47ZrpV7HqzIpQnIoTuyFc53uNIOl9Qn8eiL hbx6SoIBcV1s4k5hSm2xfhzJLbZCtEIrq4ookQSkJM/9zF1h1f905LUHLGHo6hZzeySoqrH DupTN8NnzhMifjLrsubYA== X-UI-Out-Filterresults: notjunk:1;V03:K0:eLaDvcWY9G0=:47gkzEQcZqHaPJyIslUZ07 jf33YCGJunWUkWrVSu0U9jorI1ooaRqEI7ImP1WiMRXEqiv7k8SXlHMcCmn85Q/fkQd8IHuK2 zsa4u279aKv9UBZZQGsBSQWKJjO9qglCEi2jTk+QK0ANch3JJb58iemwevnUChgvIvMbqLYce bQ+h41kV6ML8xp+eBU5/ChNnVovntwiYhKa9piwrs0aXbXerUPVaMAqOEv9T7T2U/HvfIMNI1 +a/11QWtvL1UuUCzB2wLEAu0l8PLDC7Tz4iHuCjKAUiE4y+sM192p4CId+7+0Q0y3++zr84tc BFdGy1dnzZC7MCbSbK36bQKyL6JWDkRoYWoqo8A7YKJfoMIgekwA6a7nOnls7adjtif7D7VN2 Z8d2Xwa5b2Czqqtv/75Qoe8qwvbEWBeW5jt1kUumm5LUjuzqEyoxhvQ1DXm+gj1oW9A2uRxtH fK9q3NQUojvyGLvI49VZSdkrEIiTX3VCvMWLn59QRHkrSE+0rIrpoRkeHo6LRZ9YeIynnvha8 /uzxCcQcKWWrDMNhWwHRiiOW9Ur1BreGLM6O7Sot+a5s9Qq4zYWGkpES3n3PFSNAYjWsmifGO OjgFX9m98nolfk001kHp8yaZN4856Ay6jPMPoWhBhSQWGQv1juvVKqEfy1wpTHA8u/CopVpBs Syf2ydX9Zuwl3+q8e5j7ab+Jc0B+ys3iT3Z9QQ9beJVk0s5HAEMx4Qadi/wHvGCce7n41n2h5 ZrdQ/iayeYYmuZMUltqn8ci4HL3OMTOu3XKtHyAjRF8UTKnZ+MdLhuzjqXPTPiHqXrFnwOVaM B0pypLnVYzCQ9kxfiCmAwFceCMPzW/VJs4qfINDcMpTVkJmTi15OGhtmT/2EvPBqYZCRSxRCc ZpiE0HtPu2syzzN8pebtnpqENdG/mjjcT/Ukv+NutQZz6CnHjjFpT3uaygNAoA89V0Dn1jHY2 xOx9NhtYJ72zKL1srpJJT+ixRsK4aWaQ9cHqVV9iBv5ZgErPeCOc3QtLOcid8oF9i1ASG3RQV OFC+peEfcYhAkyT64LCIsAuOVyRATD7oO7rr5uAqGCCPxlv8KSelWKDW5CPkEKc36qpC+TcJm qU1XyROAXSmtaI= X-Spam-Status: No, score=-13.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Malte Skarupke via Libc-alpha From: Malte Skarupke Reply-To: Malte Skarupke Cc: Malte Skarupke , triegel@redhat.com, malteskarupke@fastmail.fm Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" After I broadened the scope of grefs, it covered mostly the same scope as wrefs. The duplicate atomic increment/decrement was unnecessary. In this patch I remove the increment/decrement of wrefs. One exception is the case when pthread_cancel is handled. The interaction between __condvar_cleanup_waiting and pthread_cond_destroy is complicated and required both variables. So in order to preserve the existing behavior, I now increment/decrement wrefs in __condvar_cleanup_waiting. Another change is that quiesce_and_switch_g1 now clears the wake-request flag that it sets. It used to be cleared when the last waiter in the old group leaves pthread_cond_wait. The problem with this was that it could result in a race with the new pthread_cond_destroy behavior, where the leaving thread would allow pthread_cond_destroy to finish and then modify the wake-request flag after the destroy. This was pointed out in the review of an earlier version of this patch, and the fix is to make quiesce_and_switch_g1 clear up its own flag. --- nptl/nptl-printers.py | 5 ++- nptl/nptl_lock_constants.pysym | 2 +- nptl/pthread_cond_broadcast.c | 9 ++-- nptl/pthread_cond_common.c | 17 ++++---- nptl/pthread_cond_destroy.c | 30 ++++++++++---- nptl/pthread_cond_signal.c | 22 ++++++---- nptl/pthread_cond_wait.c | 75 ++++++++++++---------------------- 7 files changed, 81 insertions(+), 79 deletions(-) -- 2.25.1 diff --git a/nptl/nptl-printers.py b/nptl/nptl-printers.py index 464a9c253c..eddd62b234 100644 --- a/nptl/nptl-printers.py +++ b/nptl/nptl-printers.py @@ -313,6 +313,7 @@ class ConditionVariablePrinter(object): data = cond['__data'] self.wrefs = data['__wrefs'] + self.grefs = data['__g_refs'] self.values = [] self.read_values() @@ -350,8 +351,10 @@ class ConditionVariablePrinter(object): are waiting for it. """ + num_readers_g0 = self.grefs[0] >> PTHREAD_COND_GREFS_SHIFT + num_readers_g1 = self.grefs[1] >> PTHREAD_COND_GREFS_SHIFT self.values.append(('Threads known to still execute a wait function', - self.wrefs >> PTHREAD_COND_WREFS_SHIFT)) + num_readers_g0 + num_readers_g1)) def read_attributes(self): """Read the condvar's attributes.""" diff --git a/nptl/nptl_lock_constants.pysym b/nptl/nptl_lock_constants.pysym index ade4398e0c..2141cfa1f0 100644 --- a/nptl/nptl_lock_constants.pysym +++ b/nptl/nptl_lock_constants.pysym @@ -50,7 +50,7 @@ PTHREAD_COND_SHARED_MASK __PTHREAD_COND_SHARED_MASK PTHREAD_COND_CLOCK_MONOTONIC_MASK __PTHREAD_COND_CLOCK_MONOTONIC_MASK COND_CLOCK_BITS -- These values are hardcoded: -PTHREAD_COND_WREFS_SHIFT 3 +PTHREAD_COND_GREFS_SHIFT 1 -- Rwlock attributes PTHREAD_RWLOCK_PREFER_READER_NP diff --git a/nptl/pthread_cond_broadcast.c b/nptl/pthread_cond_broadcast.c index f1275b2f15..2ae16cbb8f 100644 --- a/nptl/pthread_cond_broadcast.c +++ b/nptl/pthread_cond_broadcast.c @@ -40,10 +40,13 @@ ___pthread_cond_broadcast (pthread_cond_t *cond) { LIBC_PROBE (cond_broadcast, 1, cond); - unsigned int wrefs = atomic_load_relaxed (&cond->__data.__wrefs); - if (wrefs >> 3 == 0) + /* See pthread_cond_signal for why relaxed MO is enough here. */ + unsigned int grefs0 = atomic_load_relaxed (cond->__data.__g_refs); + unsigned int grefs1 = atomic_load_relaxed (cond->__data.__g_refs + 1); + if ((grefs0 >> 1) == 0 && (grefs1 >> 1) == 0) return 0; - int private = __condvar_get_private (wrefs); + unsigned int flags = atomic_load_relaxed (&cond->__data.__wrefs); + int private = __condvar_get_private (flags); __condvar_acquire_lock (cond, private); diff --git a/nptl/pthread_cond_common.c b/nptl/pthread_cond_common.c index c35b9ef03a..001dbd41e2 100644 --- a/nptl/pthread_cond_common.c +++ b/nptl/pthread_cond_common.c @@ -366,18 +366,15 @@ __condvar_quiesce_and_switch_g1 (pthread_cond_t *cond, uint64_t wseq, __g_signals, which will prevent waiters from blocking using a futex on __g_signals and also notifies them that the group is closed. As a result, they will eventually remove their group reference, allowing us - to close switch group roles. */ + to close and switch group roles. */ /* First, set the closed flag on __g_signals. This tells waiters that are about to wait that they shouldn't do that anymore. This basically serves as an advance notificaton of the upcoming change to __g1_start; waiters interpret it as if __g1_start was larger than their waiter sequence position. This allows us to change __g1_start after waiting - for all existing waiters with group references to leave, which in turn - makes recovery after stealing a signal simpler because it then can be - skipped if __g1_start indicates that the group is closed (otherwise, - we would have to recover always because waiters don't know how big their - groups are). Relaxed MO is fine. */ + for all existing waiters with group references to leave. + Relaxed MO is fine. */ atomic_fetch_or_relaxed (cond->__data.__g_signals + g1, 1); /* Wait until there are no group references anymore. The fetch-or operation @@ -419,10 +416,10 @@ __condvar_quiesce_and_switch_g1 (pthread_cond_t *cond, uint64_t wseq, r = atomic_load_relaxed (cond->__data.__g_refs + g1); } } - /* Acquire MO so that we synchronize with the release operation that waiters - use to decrement __g_refs and thus happen after the waiters we waited - for. */ - atomic_thread_fence_acquire (); + /* Clear the wake-request flag. Acquire MO so that we synchronize with the + release operation that waiters use to decrement __g_refs and thus happen + after the waiters we waited for. */ + atomic_fetch_and_acquire (cond->__data.__g_refs + g1, ~(unsigned int)1); /* Update __g1_start, which finishes closing this group. The value we add will never be negative because old_orig_size can only be zero when we diff --git a/nptl/pthread_cond_destroy.c b/nptl/pthread_cond_destroy.c index 81ee958dcb..e14ec89adf 100644 --- a/nptl/pthread_cond_destroy.c +++ b/nptl/pthread_cond_destroy.c @@ -37,22 +37,36 @@ signal or broadcast calls. Thus, we can assume that all waiters that are still accessing the condvar have been woken. We wait until they have confirmed to have woken up by - decrementing __wrefs. */ + decrementing __g_refs. */ int __pthread_cond_destroy (pthread_cond_t *cond) { LIBC_PROBE (cond_destroy, 1, cond); - /* Set the wake request flag. We could also spin, but destruction that is - concurrent with still-active waiters is probably neither common nor - performance critical. Acquire MO to synchronize with waiters confirming - that they finished. */ - unsigned int wrefs = atomic_fetch_or_acquire (&cond->__data.__wrefs, 4); - int private = __condvar_get_private (wrefs); + unsigned int flags = atomic_load_relaxed (&cond->__data.__wrefs); + int private = __condvar_get_private (flags); + for (unsigned g = 0; g < 2; ++g) + { + while (true) + { + /* Set the wake request flag. We could also spin, but destruction + that is concurrent with still-active waiters is probably neither + common nor performance critical. Acquire MO to synchronize with + waiters confirming that they finished. */ + unsigned r = atomic_fetch_or_acquire (cond->__data.__g_refs + g, 1); + r |= 1; + if (r == 1) + break; + futex_wait_simple (cond->__data.__g_refs + g, r, private); + } + } + + /* Same as above, except to synchronize with canceled threads. This wake + flag never gets cleared, so it's enough to set it once. */ + unsigned int wrefs = atomic_fetch_or_acquire (&cond->__data.__wrefs, 4) | 4; while (wrefs >> 3 != 0) { futex_wait_simple (&cond->__data.__wrefs, wrefs, private); - /* See above. */ wrefs = atomic_load_acquire (&cond->__data.__wrefs); } /* The memory the condvar occupies can now be reused. */ diff --git a/nptl/pthread_cond_signal.c b/nptl/pthread_cond_signal.c index 171193b13e..2043d8ac64 100644 --- a/nptl/pthread_cond_signal.c +++ b/nptl/pthread_cond_signal.c @@ -36,13 +36,21 @@ ___pthread_cond_signal (pthread_cond_t *cond) { LIBC_PROBE (cond_signal, 1, cond); - /* First check whether there are waiters. Relaxed MO is fine for that for - the same reasons that relaxed MO is fine when observing __wseq (see - below). */ - unsigned int wrefs = atomic_load_relaxed (&cond->__data.__wrefs); - if (wrefs >> 3 == 0) + /* First check whether there are waiters. Relaxed MO is fine for that, and + it doesn't matter that there are two separate loads. It could only + matter if another thread is calling pthread_cond_wait at the same time + as this function, but then there is no happens-before relationship with + this thread, and the caller can't tell which call came first. If we miss + a waiter, the caller would have to assume that pthread_cond_signal got + called before pthread_wait, and they have no way of telling otherwise. + If they do have a way of telling then there is a happens-before + relationship and we're guaranteed to see the waiter here. */ + unsigned int grefs0 = atomic_load_relaxed (cond->__data.__g_refs); + unsigned int grefs1 = atomic_load_relaxed (cond->__data.__g_refs + 1); + if ((grefs0 >> 1) == 0 && (grefs1 >> 1) == 0) return 0; - int private = __condvar_get_private (wrefs); + unsigned int flags = atomic_load_relaxed (&cond->__data.__wrefs); + int private = __condvar_get_private (flags); __condvar_acquire_lock (cond, private); @@ -51,7 +59,7 @@ ___pthread_cond_signal (pthread_cond_t *cond) 1) We can pick any position that is allowed by external happens-before constraints. In particular, if another __pthread_cond_wait call happened before us, this waiter must be eligible for being woken by - us. The only way do establish such a happens-before is by signaling + us. The only way to establish such a happens-before is by signaling while having acquired the mutex associated with the condvar and ensuring that the signal's critical section happens after the waiter. Thus, the mutex ensures that we see that waiter's __wseq increase. diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c index fb31090e26..dde007475c 100644 --- a/nptl/pthread_cond_wait.c +++ b/nptl/pthread_cond_wait.c @@ -43,19 +43,6 @@ struct _condvar_cleanup_buffer }; -/* Decrease the waiter reference count. */ -static void -__condvar_confirm_wakeup (pthread_cond_t *cond, int private) -{ - /* If destruction is pending (i.e., the wake-request flag is nonzero) and we - are the last waiter (prior value of __wrefs was 1 << 3), then wake any - threads waiting in pthread_cond_destroy. Release MO to synchronize with - these threads. Don't bother clearing the wake-up request flag. */ - if ((atomic_fetch_add_release (&cond->__data.__wrefs, -8) >> 2) == 3) - futex_wake (&cond->__data.__wrefs, INT_MAX, private); -} - - /* Cancel waiting after having registered as a waiter previously. SEQ is our position and G is our group index. The goal of cancellation is to make our group smaller if that is still @@ -151,14 +138,7 @@ __condvar_dec_grefs (pthread_cond_t *cond, unsigned int g, int private) /* Release MO to synchronize-with the acquire load in __condvar_quiesce_and_switch_g1. */ if (atomic_fetch_add_release (cond->__data.__g_refs + g, -2) == 3) - { - /* Clear the wake-up request flag before waking up. We do not need more - than relaxed MO and it doesn't matter if we apply this for an aliased - group because we wake all futex waiters right after clearing the - flag. */ - atomic_fetch_and_relaxed (cond->__data.__g_refs + g, ~(unsigned int) 1); - futex_wake (cond->__data.__g_refs + g, INT_MAX, private); - } + futex_wake (cond->__data.__g_refs + g, INT_MAX, private); } /* Clean-up for cancellation of waiters waiting for normal signals. We cancel @@ -172,6 +152,15 @@ __condvar_cleanup_waiting (void *arg) pthread_cond_t *cond = cbuffer->cond; unsigned g = cbuffer->wseq & 1; + /* Normally we are not allowed to touch cond anymore after calling + __condvar_dec_grefs, because pthread_cond_destroy looks at __g_refs to + determine when all waiters have woken. Since we will do more work in + this function, we are using an extra channel to communicate to + pthread_cond_destroy that it is not allowed to finish yet: We + increment the refcount starting at the fourth bit on __wrefs. Relaxed + MO is enough. The synchronization happens because __condvar_dec_grefs + uses release MO. */ + atomic_fetch_add_relaxed (&cond->__data.__wrefs, 8); __condvar_dec_grefs (cond, g, cbuffer->private); __condvar_cancel_waiting (cond, cbuffer->wseq >> 1, g, cbuffer->private); @@ -183,7 +172,12 @@ __condvar_cleanup_waiting (void *arg) conservatively. */ futex_wake (cond->__data.__g_signals + g, 1, cbuffer->private); - __condvar_confirm_wakeup (cond, cbuffer->private); + /* If destruction is pending (i.e., the wake-request flag is nonzero) and we + are the last waiter (prior value of __wrefs was 1 << 3), then wake any + threads waiting in pthread_cond_destroy. Release MO to synchronize with + these threads. Don't bother clearing the wake-up request flag. */ + if ((atomic_fetch_add_release (&cond->__data.__wrefs, -8) >> 2) == 3) + futex_wake (&cond->__data.__wrefs, INT_MAX, cbuffer->private); /* XXX If locking the mutex fails, should we just stop execution? This might be better than silently ignoring the error. */ @@ -287,20 +281,21 @@ __condvar_cleanup_waiting (void *arg) __g1_orig_size: Initial size of G1 * The two least-significant bits represent the condvar-internal lock. * Only accessed while having acquired the condvar-internal lock. - __wrefs: Waiter reference counter. + __wrefs: Flags and count of waiters who called pthread_cancel. * Bit 2 is true if waiters should run futex_wake when they remove the last reference. pthread_cond_destroy uses this as futex word. * Bit 1 is the clock ID (0 == CLOCK_REALTIME, 1 == CLOCK_MONOTONIC). * Bit 0 is true iff this is a process-shared condvar. - * Simple reference count used by both waiters and pthread_cond_destroy. - (If the format of __wrefs is changed, update nptl_lock_constants.pysym - and the pretty printers.) + * Simple reference count used by __condvar_cleanup_waiting and pthread_cond_destroy. + (If the format of __wrefs is changed, update the pretty printers.) For each of the two groups, we have: __g_refs: Futex waiter reference count. * LSB is true if waiters should run futex_wake when they remove the last reference. * Reference count used by waiters concurrently with signalers that have acquired the condvar-internal lock. + (If the format of __g_refs is changed, update nptl_lock_constants.pysym + and the pretty printers.) __g_signals: The number of signals that can still be consumed. * Used as a futex word by waiters. Used concurrently by waiters and signalers. @@ -329,18 +324,6 @@ __condvar_cleanup_waiting (void *arg) sufficient because if a waiter can see a sufficiently large value, it could have also consume a signal in the waiters group. - Waiters try to grab a signal from __g_signals without holding a reference - count, which can lead to stealing a signal from a more recent group after - their own group was already closed. They cannot always detect whether they - in fact did because they do not know when they stole, but they can - conservatively add a signal back to the group they stole from; if they - did so unnecessarily, all that happens is a spurious wake-up. To make this - even less likely, __g1_start contains the index of the current g2 too, - which allows waiters to check if there aliasing on the group slots; if - there wasn't, they didn't steal from the current G1, which means that the - G1 they stole from must have been already closed and they do not need to - fix anything. - It is essential that the last field in pthread_cond_t is __g_signals[1]: The previous condvar used a pointer-sized field in pthread_cond_t, so a PTHREAD_COND_INITIALIZER from that condvar implementation might only @@ -405,16 +388,14 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, unsigned int g = wseq & 1; uint64_t seq = wseq >> 1; - /* Increase the waiter reference count. Relaxed MO is sufficient because - we only need to synchronize when decrementing the reference count. */ - unsigned int flags = atomic_fetch_add_relaxed (&cond->__data.__wrefs, 8); - int private = __condvar_get_private (flags); /* Acquire a group reference and use acquire MO for that so that we synchronize with the dummy read-modify-write in __condvar_quiesce_and_switch_g1 if we read from the same group. This will make us see the closed flag on __g_signals that designates a concurrent attempt to reuse the group's slot. */ atomic_fetch_add_acquire (cond->__data.__g_refs + g, 2); + unsigned int flags = atomic_load_relaxed (&cond->__data.__wrefs); + int private = __condvar_get_private (flags); /* Now that we are registered as a waiter, we can release the mutex. Waiting on the condvar must be atomic with releasing the mutex, so if @@ -428,7 +409,6 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, { __condvar_dec_grefs (cond, g, private); __condvar_cancel_waiting (cond, seq, g, private); - __condvar_confirm_wakeup (cond, private); return err; } @@ -480,8 +460,8 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, /* No signals available after spinning, so prepare to block. First check the closed flag on __g_signals that designates a concurrent attempt to reuse the group's slot. We use acquire MO for - the __g_signals check to make the __g1_start check work (see - above). */ + the __g_signals check to make sure we read the current value of + __g1_start (see above). */ if (((atomic_load_acquire (cond->__data.__g_signals + g) & 1) != 0) || (seq < (__condvar_load_g1_start_relaxed (cond) >> 1))) { @@ -522,9 +502,7 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, } /* Try to grab a signal. Use acquire MO so that we see an up-to-date value - of __g1_start below (see spinning above for a similar case). In - particular, if we steal from a more recent group, we will also see a - more recent __g1_start below. */ + of __g1_start when spinning above. */ while (!atomic_compare_exchange_weak_acquire (cond->__data.__g_signals + g, &signals, signals - 2)); @@ -534,7 +512,6 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, that before acquiring the mutex to allow for execution of pthread_cond_destroy while having acquired the mutex. */ __condvar_dec_grefs (cond, g, private); - __condvar_confirm_wakeup (cond, private); acquire_lock: /* Woken up; now re-acquire the mutex. If this doesn't fail, return RESULT, From patchwork Wed Sep 8 02:52:37 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Malte Skarupke X-Patchwork-Id: 44899 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 05D5F38515F1 for ; Wed, 8 Sep 2021 02:56:08 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 05D5F38515F1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1631069768; bh=NEAxP2Bog1u7V10l+0x39gHVwqG0dyFfAOevhW/4vtQ=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=EinAQ30XO4e8R/Ajf+ZBj6eeGfaS1Azcfn0R4/PyLo49GzCvVIfLu1OqiQPK+yxi3 kJ17pb/XQjHeDsECks36ovRtuaEMOrKEi0v22EYQ/0FGB+6Z+qXOkzF7WtjbkCKYJE gqSRJNTjefHHrhTgI5TE5E5YI2pEmD2HXMtMX5KU= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mout.web.de (mout.web.de [212.227.17.11]) by sourceware.org (Postfix) with ESMTPS id 2F6DD385C412 for ; Wed, 8 Sep 2021 02:54:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2F6DD385C412 X-UI-Sender-Class: c548c8c5-30a9-4db5-a2e7-cb6cb037b8f9 Received: from auth2-smtp.messagingengine.com ([66.111.4.228]) by smtp.web.de (mrweb101 [213.165.67.124]) with ESMTPSA (Nemesis) id 0Meje0-1mYtWK0jNI-00OCfO; Wed, 08 Sep 2021 04:54:16 +0200 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailauth.nyi.internal (Postfix) with ESMTP id E771527C0060; Tue, 7 Sep 2021 22:54:14 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Tue, 07 Sep 2021 22:54:14 -0400 X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrudefiedgiedvucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpefhvffufffkofgjfhgggfestdekre dtredttdenucfhrhhomhepofgrlhhtvgcuufhkrghruhhpkhgvuceomhgrlhhtvghskhgr rhhuphhkvgesfigvsgdruggvqeenucggtffrrghtthgvrhhnpeevgfehffefffehleelie fftddvudffkedtkeduheeiueeugffgvdejgeduvdfgjeenucevlhhushhtvghrufhiiigv pedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehmrghlthgvshhkrghruhhpkhgvodhmvg hsmhhtphgruhhthhhpvghrshhonhgrlhhithihqddutddujedtfedvleeiqdduuddvgedv keeiledqmhgrlhhtvghskhgrrhhuphhkvgeppeifvggsrdguvgesfhgrshhtmhgrihhlrd hfmh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 7 Sep 2021 22:54:14 -0400 (EDT) To: libc-alpha@sourceware.org Subject: [PATCH v2 4/6] nptl: Make test-cond-printers check the number of waiters Date: Tue, 7 Sep 2021 22:52:37 -0400 Message-Id: <20210908025239.1326480-5-malteskarupke@web.de> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20210908025239.1326480-1-malteskarupke@web.de> References: <20210908025239.1326480-1-malteskarupke@web.de> MIME-Version: 1.0 X-Provags-ID: V03:K1:hCd2bAXuirey1CXzkyM8G4k5plFokaRDIEAuxWzqO8fmnJPBxqs H9nWjWuz3pjHl3cX8C9je5CxecMOKlb6gpnO+Br1jgbur7XRz+u18JiK0utZnHHqjz7LGzJ 9hIxyXKrWYAB931PC9nE5uin9CmJIdnGEIhM62Cb+vqVL7VJ0uVuiXIXJntEszsCP8YK54p vgFOHqQo8dos6TNC9Xy3Q== X-UI-Out-Filterresults: notjunk:1;V03:K0:4YoaCjh7Jws=:AYhcIHSroyFe9X8IFMohSB 9zJNQL2kHfXK4lky43sfwBDRdL7K9Tgb/OcgK6LEeoWpSkjtiPkBpPunnh6XWRH8020toIzbp GVrYiEAATMDR3ZL7zTZ7YeoTf4iFA9mC7N0fd+fXGRHMeTMlPQe6GF2XlEVwXsueUSTBn1KIt QP48JNRP+bu/tDJSl8MKG9/hCV2y3mePYCt7kuZmg7UP2kGbVpBWWuiGIcNzz4Yk+Cqa7KNUH yd03ppaVOSnNrOhfW9toEwpDuNRox6cynZ4VFA+mWfP7bW71SGDuqBUyO3TkjiM8VCJ7FRGsX /9CW+LcDEjQnvWBwYaH1u6NRXt4XZAs2DVU9V0FQ2rPywu5pQUwJ4q3OTzpprUnvmHwqBAz1j HgOtRSXWZfE/RAymPne0FD4v396n5iKo8M0ppWNReGEaOqisaaErZ2gmS22Ao+x6zQib1Z3j0 D0QPYrxMcfWyyYjsSGWGqMNg+91NxV2jRmkzzsWt4NSc2/SM3sOuMJQpJKeuIGFebNnzST37H T01eryc8EDWOlnHoGyeFEPW1xhLcCOWybI9z/oxXckMjeJ/mAYYBzsotViv/VrW9i8JJPiqIJ HMkvRzO3qfdEXbaF5RcYsDpxZB8Lhm/a//x8v+fcnU4rYI4sJQBISzriYCI+a7ZHQoPvcKzrI jzH0XD8as9hYjsEvzbGfftBHvhi9dsii3VWfWHz4bnKOLvluokgokP2bZ1GXqG3PKYpzQfIua zjgymq6tVhzweExdmWvJ6hvEvQxoUfmxAk90cKENABcE4UH3+LhSlTDvEmfEFDQJx6uVx4afE Sm2ZWFyliVqSVlcCJzttFyiyqY+BwolzW8hOi0XLmDiRKXLtITanTWnhZ+a5Km/hhYO545183 6bW99t3jfPcKluQuuD8bg23St/71cEORSKnC6R7TcRm01eLAurGFm1p2YDvdkWqN7whGTJpi0 +XsGsXsgbpXl5PFfQvPjhLIlf3Ha/djk0Yo34QbGQ++OacDWNiKHv2rhT67q7kJwsjK2IQ/92 ALbuzn5UyLQIDe/D7bfuEVrDcZx/nJw0mhK2wqc6n8FGT2q8Hf08XUu85OXeA12cc0pH8G3Ge qp/ZSA+GJzSKrY= X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Malte Skarupke via Libc-alpha From: Malte Skarupke Reply-To: Malte Skarupke Cc: Malte Skarupke , triegel@redhat.com, malteskarupke@fastmail.fm Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" In my last change I changed the semantics of how to determine the number of waiters on a condition variable. The existing test only tested that the printers print something. They didn't cover the case when there is a thread sleeping on the condition variable. In this patch I changed the test to ensure that the correct number is printed. This is just to double-check the changes from my previous patch. --- nptl/test-cond-printers.c | 56 +++++++++++++++++++++++++++++++++----- nptl/test-cond-printers.py | 5 ++++ 2 files changed, 54 insertions(+), 7 deletions(-) -- 2.25.1 diff --git a/nptl/test-cond-printers.c b/nptl/test-cond-printers.c index 8eaba2bb41..6c19d3e725 100644 --- a/nptl/test-cond-printers.c +++ b/nptl/test-cond-printers.c @@ -26,7 +26,14 @@ #define PASS 0 #define FAIL 1 -static int test_status_destroyed (pthread_cond_t *condvar); +static int test_status (pthread_cond_t *condvar); + +typedef struct +{ + pthread_mutex_t *mutex; + pthread_cond_t *condvar; + int *wait_thread_asleep; +} test_state; int main (void) @@ -36,22 +43,57 @@ main (void) int result = FAIL; if (pthread_condattr_init (&attr) == 0 - && test_status_destroyed (&condvar) == PASS) + && test_status (&condvar) == PASS) result = PASS; /* Else, one of the pthread_cond* functions failed. */ return result; } +static void * +wait (void *arg) +{ + test_state *state = (test_state *)arg; + void *result = PASS; + if (pthread_mutex_lock (state->mutex) != 0) + result = (void *)FAIL; + *state->wait_thread_asleep = 1; + if (pthread_cond_signal (state->condvar) != 0) + result = (void *)FAIL; + if (pthread_cond_wait (state->condvar, state->mutex) != 0) + result = (void *)FAIL; + if (pthread_mutex_unlock (state->mutex) != 0) + result = (void *)FAIL; + return result; +} + /* Initializes CONDVAR, then destroys it. */ static int -test_status_destroyed (pthread_cond_t *condvar) +test_status (pthread_cond_t *condvar) { - int result = FAIL; + int result = PASS; - if (pthread_cond_init (condvar, NULL) == 0 - && pthread_cond_destroy (condvar) == 0) - result = PASS; /* Test status (destroyed). */ + pthread_mutex_t mutex; + result |= pthread_mutex_init (&mutex, NULL); + result |= pthread_cond_init (condvar, NULL); + int wait_thread_asleep = 0; + test_state state = { &mutex, condvar, &wait_thread_asleep }; + result |= pthread_mutex_lock (&mutex); + pthread_t thread; + result |= pthread_create (&thread, NULL, wait, &state); + while (!wait_thread_asleep) + { + result |= pthread_cond_wait (condvar, &mutex); + } + result |= pthread_cond_signal (condvar); /* Test about to signal */ + result |= pthread_mutex_unlock (&mutex); + result |= pthread_cond_destroy (condvar); + void *retval = NULL; + result |= pthread_join (thread, &retval); /* Test status (destroyed). */ + result |= pthread_mutex_destroy (&mutex); + result = result ? FAIL : PASS; + if (retval != NULL) + result = FAIL; return result; } diff --git a/nptl/test-cond-printers.py b/nptl/test-cond-printers.py index cb76a584e1..635f468e11 100644 --- a/nptl/test-cond-printers.py +++ b/nptl/test-cond-printers.py @@ -33,6 +33,11 @@ try: var = 'condvar' to_string = 'pthread_cond_t' + break_at(test_source, 'Test about to signal') + continue_cmd() # Go to test_status_destroyed + test_printer(var, to_string, {'Threads known to still execute a wait function': '1'}) + + break_at(test_source, 'Test status (destroyed)') continue_cmd() # Go to test_status_destroyed test_printer(var, to_string, {'Threads known to still execute a wait function': '0'}) From patchwork Wed Sep 8 02:52:38 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Malte Skarupke X-Patchwork-Id: 44902 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 8F11E38515EF for ; Wed, 8 Sep 2021 02:58:22 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 8F11E38515EF DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1631069902; bh=9Pef7XVN1ql7bctQ2cB1vJ23lVKyVn9c2D1AEspSe7M=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=O6/gLOptuBySRVkeqUkIg4RqosrN2UdhVEd0CU1zh+uOxUwT0q+BTrn8hfNMyaZbX v/oCthYqy0fu3KNzspAaWDVQfv/aYiXLBrVVGlQP1jaDHuHXwpsWbT2EWCjNF9hGWo +PXc8Q+PP9DzMw+bm2eEmazuzdiErZw9DTaTdSu4= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mout.web.de (mout.web.de [212.227.17.12]) by sourceware.org (Postfix) with ESMTPS id 6293238515E9 for ; Wed, 8 Sep 2021 02:54:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6293238515E9 X-UI-Sender-Class: c548c8c5-30a9-4db5-a2e7-cb6cb037b8f9 Received: from auth2-smtp.messagingengine.com ([66.111.4.228]) by smtp.web.de (mrweb103 [213.165.67.124]) with ESMTPSA (Nemesis) id 0MOArg-1mKVw51p5Q-005V5y; Wed, 08 Sep 2021 04:54:17 +0200 Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailauth.nyi.internal (Postfix) with ESMTP id 6367E27C0061; Tue, 7 Sep 2021 22:54:15 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute5.internal (MEProxy); Tue, 07 Sep 2021 22:54:15 -0400 X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrudefiedgieduucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpefhvffufffkofgjfhgggfestdekre dtredttdenucfhrhhomhepofgrlhhtvgcuufhkrghruhhpkhgvuceomhgrlhhtvghskhgr rhhuphhkvgesfigvsgdruggvqeenucggtffrrghtthgvrhhnpeevgfehffefffehleelie fftddvudffkedtkeduheeiueeugffgvdejgeduvdfgjeenucevlhhushhtvghrufhiiigv pedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehmrghlthgvshhkrghruhhpkhgvodhmvg hsmhhtphgruhhthhhpvghrshhonhgrlhhithihqddutddujedtfedvleeiqdduuddvgedv keeiledqmhgrlhhtvghskhgrrhhuphhkvgeppeifvggsrdguvgesfhgrshhtmhgrihhlrd hfmh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 7 Sep 2021 22:54:14 -0400 (EDT) To: libc-alpha@sourceware.org Subject: [PATCH v2 5/6] nptl: Rename __wrefs to __crefs because its meaning has changed Date: Tue, 7 Sep 2021 22:52:38 -0400 Message-Id: <20210908025239.1326480-6-malteskarupke@web.de> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20210908025239.1326480-1-malteskarupke@web.de> References: <20210908025239.1326480-1-malteskarupke@web.de> MIME-Version: 1.0 X-Provags-ID: V03:K1:raZZUVDCfvwUxYXFTrOLV59fUjKG7ApKblKSSByLgGqUiWOkyph GdU2b5S7AHUBDfzBqqXe6srDzkMsMR2KbuCE8K5s2gVJsbImXijMl45dcpjV6pNMiFKq13s iOV/pcsp3Ux1pgVXbd06ytfr6Rx8E8ArCk6bAuUcg/8H0Pm0RQpVkwa+wAN41iamjkFoNKX s0UoVV7rXBa68H35ZyCog== X-UI-Out-Filterresults: notjunk:1;V03:K0:6t3u7kKkSOY=:GXxYrRmFb61e8E4Xv/R/dU UZ8MkIm+4VzqPDQqE/P9KCRKFw0sBkwDlrQNGA8zs4qzpwERW9u+Bl9bG9WMdtjXRtATcw0Nr T+AvkI/OsjGdQjsXJ8dlLy4Z/vHYZ8V32DcDpLOy5RoA6rsyUb5b2lUKzGbcQLv+llcPk5SOd EHgpjHWDWcXcN/NOG9iXyhArK4aEwf06nWyqWNR4HDdemMRdNvWfA2AtY+NEC5hO3PE8r5tHZ j2Pu10FMBiwKpnDzD954bBr+uAvPn6sr+I3iDK6TQocBb/adNrXEKf/QGeVi+fPNjjLRertnj LMhTkX8ht9DVzplyGrhZ7S+dxFCzp7FgE55eiAUbBgdm0qx3LpELiyJ/x8VFz0S9n1KzXp5jw g9NVQ1tTxsrLzC+c9XkHO/i359gkNUnMJ/J+lQcIGhSQCGO1j+MEX4m+Tj76ortR5+QFr6yIb lvfdBRNoldpar374ERyasU2Z8MBbvSR8XUBG2cPAY85ArjKH8kAyBBqv+oVWrQt5Kn8VP2wdi P1IDQeK7WyYwtx3q4titiOB3wqp2MXAo/s7vXUSCqpPg3jsvc/qASwgIvpFsBJ286s12DiNr+ 78/1iIft2MGwCbSqHPjKWbF3my3jt3AIVVRqqFGXVQi2kwLpjorxv2sYPjAnF60ik0q+7m7TG bZSd7q2DYJ4+NdNrMGz9y7u0SbWR8VDjPNqqywJYU5U+5eBk2FZGP1jz2lIB+mstx5XcFtxJO cimdKMpsc1hz2/cLpDsLN3jmFGRLFq1KkpNazlAQQAMgWZDL2C7OZOVdOetzqVGXzuxdUCurm a+69ZdJfSlYz1YLntPnzaNdsggRKy+3QTL7g1bBnxnXIBYshDmRTBUS4vO3gaqbI9f8aShm8Z nvTzxBh95lqpM8il6h+YmCejDuICEGjtRFNWEAjOUqUOZVjEv8G1hsMG5ck7YKpcFnUDFlQDZ XRjE1POE/AZJpwD8QZ99r7fYn8wYuHtTPTC9fxGtn7OclZfQZjna/aYr7KPYVfTIXa800glki h6nIdzSTLud/jIAudkzdLb/AEvdGWxMmzHuqNyURdTr7FtHFum3zHeUhI3HQLVy9S/1PBwmp5 bYm6yPZZhgEjpyZ2lO6COhdIOi9j5cNZ2efYJY4VDOQHO98uPa8ZPX2Hw== X-Spam-Status: No, score=-13.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Malte Skarupke via Libc-alpha From: Malte Skarupke Reply-To: Malte Skarupke Cc: Malte Skarupke , triegel@redhat.com, malteskarupke@fastmail.fm Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" When I remove the increment/decrement of wrefs in pthread_cond_wait, it no longer really had the meaning of representing the number of waiters. It is still used as a reference count for threads that call pthread_cancel, so crefs it is. --- nptl/nptl-printers.py | 6 +++--- nptl/pthread_cond_broadcast.c | 2 +- nptl/pthread_cond_common.c | 4 ++-- nptl/pthread_cond_destroy.c | 10 +++++----- nptl/pthread_cond_init.c | 4 ++-- nptl/pthread_cond_signal.c | 2 +- nptl/pthread_cond_wait.c | 16 ++++++++-------- nptl/tst-cond22.c | 4 ++-- sysdeps/nptl/bits/thread-shared-types.h | 2 +- 9 files changed, 25 insertions(+), 25 deletions(-) -- 2.25.1 diff --git a/nptl/nptl-printers.py b/nptl/nptl-printers.py index eddd62b234..99155e352b 100644 --- a/nptl/nptl-printers.py +++ b/nptl/nptl-printers.py @@ -312,7 +312,7 @@ class ConditionVariablePrinter(object): """ data = cond['__data'] - self.wrefs = data['__wrefs'] + self.crefs = data['__crefs'] self.grefs = data['__g_refs'] self.values = [] @@ -359,12 +359,12 @@ class ConditionVariablePrinter(object): def read_attributes(self): """Read the condvar's attributes.""" - if (self.wrefs & PTHREAD_COND_CLOCK_MONOTONIC_MASK) != 0: + if (self.crefs & PTHREAD_COND_CLOCK_MONOTONIC_MASK) != 0: self.values.append(('Clock ID', 'CLOCK_MONOTONIC')) else: self.values.append(('Clock ID', 'CLOCK_REALTIME')) - if (self.wrefs & PTHREAD_COND_SHARED_MASK) != 0: + if (self.crefs & PTHREAD_COND_SHARED_MASK) != 0: self.values.append(('Shared', 'Yes')) else: self.values.append(('Shared', 'No')) diff --git a/nptl/pthread_cond_broadcast.c b/nptl/pthread_cond_broadcast.c index 2ae16cbb8f..51a1249f08 100644 --- a/nptl/pthread_cond_broadcast.c +++ b/nptl/pthread_cond_broadcast.c @@ -45,7 +45,7 @@ ___pthread_cond_broadcast (pthread_cond_t *cond) unsigned int grefs1 = atomic_load_relaxed (cond->__data.__g_refs + 1); if ((grefs0 >> 1) == 0 && (grefs1 >> 1) == 0) return 0; - unsigned int flags = atomic_load_relaxed (&cond->__data.__wrefs); + unsigned int flags = atomic_load_relaxed (&cond->__data.__crefs); int private = __condvar_get_private (flags); __condvar_acquire_lock (cond, private); diff --git a/nptl/pthread_cond_common.c b/nptl/pthread_cond_common.c index 001dbd41e2..9af6a6fbec 100644 --- a/nptl/pthread_cond_common.c +++ b/nptl/pthread_cond_common.c @@ -20,7 +20,7 @@ #include #include -/* We need 3 least-significant bits on __wrefs for something else. */ +/* We need 3 least-significant bits on __crefs for something else. */ #define __PTHREAD_COND_MAX_GROUP_SIZE ((unsigned) 1 << 29) #if __HAVE_64B_ATOMICS == 1 @@ -318,7 +318,7 @@ __condvar_set_orig_size (pthread_cond_t *cond, unsigned int size) atomic_store_relaxed (&cond->__data.__g1_orig_size, (size << 2) | 2); } -/* Returns FUTEX_SHARED or FUTEX_PRIVATE based on the provided __wrefs +/* Returns FUTEX_SHARED or FUTEX_PRIVATE based on the provided __crefs value. */ static int __attribute__ ((unused)) __condvar_get_private (int flags) diff --git a/nptl/pthread_cond_destroy.c b/nptl/pthread_cond_destroy.c index e14ec89adf..ba971152e5 100644 --- a/nptl/pthread_cond_destroy.c +++ b/nptl/pthread_cond_destroy.c @@ -43,7 +43,7 @@ __pthread_cond_destroy (pthread_cond_t *cond) { LIBC_PROBE (cond_destroy, 1, cond); - unsigned int flags = atomic_load_relaxed (&cond->__data.__wrefs); + unsigned int flags = atomic_load_relaxed (&cond->__data.__crefs); int private = __condvar_get_private (flags); for (unsigned g = 0; g < 2; ++g) { @@ -63,11 +63,11 @@ __pthread_cond_destroy (pthread_cond_t *cond) /* Same as above, except to synchronize with canceled threads. This wake flag never gets cleared, so it's enough to set it once. */ - unsigned int wrefs = atomic_fetch_or_acquire (&cond->__data.__wrefs, 4) | 4; - while (wrefs >> 3 != 0) + unsigned int crefs = atomic_fetch_or_acquire (&cond->__data.__crefs, 4) | 4; + while (crefs >> 3 != 0) { - futex_wait_simple (&cond->__data.__wrefs, wrefs, private); - wrefs = atomic_load_acquire (&cond->__data.__wrefs); + futex_wait_simple (&cond->__data.__crefs, crefs, private); + crefs = atomic_load_acquire (&cond->__data.__crefs); } /* The memory the condvar occupies can now be reused. */ return 0; diff --git a/nptl/pthread_cond_init.c b/nptl/pthread_cond_init.c index 6aa002ed1f..61d2e56881 100644 --- a/nptl/pthread_cond_init.c +++ b/nptl/pthread_cond_init.c @@ -37,13 +37,13 @@ __pthread_cond_init (pthread_cond_t *cond, const pthread_condattr_t *cond_attr) /* Iff not equal to ~0l, this is a PTHREAD_PROCESS_PRIVATE condvar. */ if (icond_attr != NULL && (icond_attr->value & 1) != 0) - cond->__data.__wrefs |= __PTHREAD_COND_SHARED_MASK; + cond->__data.__crefs |= __PTHREAD_COND_SHARED_MASK; int clockid = (icond_attr != NULL ? ((icond_attr->value >> 1) & ((1 << COND_CLOCK_BITS) - 1)) : CLOCK_REALTIME); /* If 0, CLOCK_REALTIME is used; CLOCK_MONOTONIC otherwise. */ if (clockid != CLOCK_REALTIME) - cond->__data.__wrefs |= __PTHREAD_COND_CLOCK_MONOTONIC_MASK; + cond->__data.__crefs |= __PTHREAD_COND_CLOCK_MONOTONIC_MASK; LIBC_PROBE (cond_init, 2, cond, cond_attr); diff --git a/nptl/pthread_cond_signal.c b/nptl/pthread_cond_signal.c index 2043d8ac64..df018b1b61 100644 --- a/nptl/pthread_cond_signal.c +++ b/nptl/pthread_cond_signal.c @@ -49,7 +49,7 @@ ___pthread_cond_signal (pthread_cond_t *cond) unsigned int grefs1 = atomic_load_relaxed (cond->__data.__g_refs + 1); if ((grefs0 >> 1) == 0 && (grefs1 >> 1) == 0) return 0; - unsigned int flags = atomic_load_relaxed (&cond->__data.__wrefs); + unsigned int flags = atomic_load_relaxed (&cond->__data.__crefs); int private = __condvar_get_private (flags); __condvar_acquire_lock (cond, private); diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c index dde007475c..b833e543a5 100644 --- a/nptl/pthread_cond_wait.c +++ b/nptl/pthread_cond_wait.c @@ -157,10 +157,10 @@ __condvar_cleanup_waiting (void *arg) determine when all waiters have woken. Since we will do more work in this function, we are using an extra channel to communicate to pthread_cond_destroy that it is not allowed to finish yet: We - increment the refcount starting at the fourth bit on __wrefs. Relaxed + increment the refcount starting at the fourth bit on __crefs. Relaxed MO is enough. The synchronization happens because __condvar_dec_grefs uses release MO. */ - atomic_fetch_add_relaxed (&cond->__data.__wrefs, 8); + atomic_fetch_add_relaxed (&cond->__data.__crefs, 8); __condvar_dec_grefs (cond, g, cbuffer->private); __condvar_cancel_waiting (cond, cbuffer->wseq >> 1, g, cbuffer->private); @@ -176,8 +176,8 @@ __condvar_cleanup_waiting (void *arg) are the last waiter (prior value of __wrefs was 1 << 3), then wake any threads waiting in pthread_cond_destroy. Release MO to synchronize with these threads. Don't bother clearing the wake-up request flag. */ - if ((atomic_fetch_add_release (&cond->__data.__wrefs, -8) >> 2) == 3) - futex_wake (&cond->__data.__wrefs, INT_MAX, cbuffer->private); + if ((atomic_fetch_add_release (&cond->__data.__crefs, -8) >> 2) == 3) + futex_wake (&cond->__data.__crefs, INT_MAX, cbuffer->private); /* XXX If locking the mutex fails, should we just stop execution? This might be better than silently ignoring the error. */ @@ -281,13 +281,13 @@ __condvar_cleanup_waiting (void *arg) __g1_orig_size: Initial size of G1 * The two least-significant bits represent the condvar-internal lock. * Only accessed while having acquired the condvar-internal lock. - __wrefs: Flags and count of waiters who called pthread_cancel. + __crefs: Flags and count of waiters who called pthread_cancel. * Bit 2 is true if waiters should run futex_wake when they remove the last reference. pthread_cond_destroy uses this as futex word. * Bit 1 is the clock ID (0 == CLOCK_REALTIME, 1 == CLOCK_MONOTONIC). * Bit 0 is true iff this is a process-shared condvar. * Simple reference count used by __condvar_cleanup_waiting and pthread_cond_destroy. - (If the format of __wrefs is changed, update the pretty printers.) + (If the format of __crefs is changed, update the pretty printers.) For each of the two groups, we have: __g_refs: Futex waiter reference count. * LSB is true if waiters should run futex_wake when they remove the @@ -394,7 +394,7 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, make us see the closed flag on __g_signals that designates a concurrent attempt to reuse the group's slot. */ atomic_fetch_add_acquire (cond->__data.__g_refs + g, 2); - unsigned int flags = atomic_load_relaxed (&cond->__data.__wrefs); + unsigned int flags = atomic_load_relaxed (&cond->__data.__crefs); int private = __condvar_get_private (flags); /* Now that we are registered as a waiter, we can release the mutex. @@ -549,7 +549,7 @@ ___pthread_cond_timedwait64 (pthread_cond_t *cond, pthread_mutex_t *mutex, /* Relaxed MO is suffice because clock ID bit is only modified in condition creation. */ - unsigned int flags = atomic_load_relaxed (&cond->__data.__wrefs); + unsigned int flags = atomic_load_relaxed (&cond->__data.__crefs); clockid_t clockid = (flags & __PTHREAD_COND_CLOCK_MONOTONIC_MASK) ? CLOCK_MONOTONIC : CLOCK_REALTIME; return __pthread_cond_wait_common (cond, mutex, clockid, abstime); diff --git a/nptl/tst-cond22.c b/nptl/tst-cond22.c index 64f19ea0a5..d4fcaacc98 100644 --- a/nptl/tst-cond22.c +++ b/nptl/tst-cond22.c @@ -110,7 +110,7 @@ do_test (void) c.__data.__wseq, c.__data.__g1_start, c.__data.__g_signals[0], c.__data.__g_refs[0], c.__data.__g_size[0], c.__data.__g_signals[1], c.__data.__g_refs[1], c.__data.__g_size[1], - c.__data.__g1_orig_size, c.__data.__wrefs); + c.__data.__g1_orig_size, c.__data.__crefs); if (pthread_create (&th, NULL, tf, (void *) 1l) != 0) { @@ -153,7 +153,7 @@ do_test (void) c.__data.__wseq, c.__data.__g1_start, c.__data.__g_signals[0], c.__data.__g_refs[0], c.__data.__g_size[0], c.__data.__g_signals[1], c.__data.__g_refs[1], c.__data.__g_size[1], - c.__data.__g1_orig_size, c.__data.__wrefs); + c.__data.__g1_orig_size, c.__data.__crefs); return status; } diff --git a/sysdeps/nptl/bits/thread-shared-types.h b/sysdeps/nptl/bits/thread-shared-types.h index 44bf1e358d..494a43a675 100644 --- a/sysdeps/nptl/bits/thread-shared-types.h +++ b/sysdeps/nptl/bits/thread-shared-types.h @@ -112,7 +112,7 @@ struct __pthread_cond_s unsigned int __g_refs[2] __LOCK_ALIGNMENT; unsigned int __g_size[2]; unsigned int __g1_orig_size; - unsigned int __wrefs; + unsigned int __crefs; unsigned int __g_signals[2]; }; From patchwork Wed Sep 8 02:52:39 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Malte Skarupke X-Patchwork-Id: 44903 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 72E6D3858D35 for ; Wed, 8 Sep 2021 02:59:05 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 72E6D3858D35 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1631069945; bh=HQ2zqYkCUASjzgVAtVwAlcaikc1Pf7T/6aDnczsWJFc=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=qYs/+EwmLFHMp6tTpgUjGbZNrx2iAgEljuKqSSnuAj4nlli0VpDkfbNMQWWvGXoAn aCo5vp6xIWJ6hEtlKvuKRqLWz0D1bKrWrDFPTrDMwIwLPKRvXPl31xpqQFMxzJWx0D v6fe22KeSX7I++UoI3XUabVoziAPWgXiDyiTuevw= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mout.web.de (mout.web.de [212.227.17.12]) by sourceware.org (Postfix) with ESMTPS id 8EC5F38515E3 for ; Wed, 8 Sep 2021 02:54:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8EC5F38515E3 X-UI-Sender-Class: c548c8c5-30a9-4db5-a2e7-cb6cb037b8f9 Received: from auth2-smtp.messagingengine.com ([66.111.4.228]) by smtp.web.de (mrweb106 [213.165.67.124]) with ESMTPSA (Nemesis) id 1MMpCg-1mgZ250glw-00J18O; Wed, 08 Sep 2021 04:54:17 +0200 Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailauth.nyi.internal (Postfix) with ESMTP id C8D5627C0064; Tue, 7 Sep 2021 22:54:15 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute5.internal (MEProxy); Tue, 07 Sep 2021 22:54:15 -0400 X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrudefiedgieduucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpefhvffufffkofgjfhgggfestdekre dtredttdenucfhrhhomhepofgrlhhtvgcuufhkrghruhhpkhgvuceomhgrlhhtvghskhgr rhhuphhkvgesfigvsgdruggvqeenucggtffrrghtthgvrhhnpeevgfehffefffehleelie fftddvudffkedtkeduheeiueeugffgvdejgeduvdfgjeenucevlhhushhtvghrufhiiigv pedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehmrghlthgvshhkrghruhhpkhgvodhmvg hsmhhtphgruhhthhhpvghrshhonhgrlhhithihqddutddujedtfedvleeiqdduuddvgedv keeiledqmhgrlhhtvghskhgrrhhuphhkvgeppeifvggsrdguvgesfhgrshhtmhgrihhlrd hfmh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 7 Sep 2021 22:54:15 -0400 (EDT) To: libc-alpha@sourceware.org Subject: [PATCH v2 6/6] nptl: Cleaning up __g1_start and related code in pthread_cond_wait Date: Tue, 7 Sep 2021 22:52:39 -0400 Message-Id: <20210908025239.1326480-7-malteskarupke@web.de> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20210908025239.1326480-1-malteskarupke@web.de> References: <20210908025239.1326480-1-malteskarupke@web.de> MIME-Version: 1.0 X-Provags-ID: V03:K1:xqMt/2VaYSSA/XPMqYuvQOaXo0cvtGA51BrH/6In4f5v30vRQBu wdPBKd689JBMCho3/hgcgr8CWIrgyxPaVN8rGuGDOesi036ZOW9kNQdGiGVCfNCYQSMFK3H lMRvQBUh9m3UT7gY38ETsyqZEdIT1o74TQMGWHpttbTGEnB4k17lKchXjmn/ycmer0shy4e fH+JQn+ifGoAOtOXy5fsA== X-UI-Out-Filterresults: notjunk:1;V03:K0:sINWOSAD9k4=:wlQQ5WsDnE/12GlNpIYKfK +w0MazdAR++d0ctztd1PEak5LlNk3vB7ogIWz6MdmtXxX+ZdgoapgkdNX2KoljGDwP4wObl5e g//sgzXr0wmxUKdw//21H9GJPVuP6Ua2HHTDcZYZNf9T0uh1FHm8jQ2T+lTV8TolUrR6/i3tT wTxP4TSX1WrJXYZuy+A6uat4jDgQaIYES1jnoYW3YsUpaRJrEovmd/BFnpD2BWmJWG24jDf93 4kZ+9ECeCUrxiBj2iOfMVJoGLbWkcbyAoI8jEeSxDtHQfUJIsAqiTmRMRRP5Q7BTZ7ny5o4cz +HVpxOq0EugmpqlRF7ou4fDDiuKobU1u00YPxiaIHtXkvZVHUuya8FzbHtNpXoMjxLRuu4J0D uWqY3yW6X22d45JaeXSSXSGY9Rz6HlmxapoIgaHPxXwbCUhj3P6v/aqEzcKJLXztwpSp+NYsG FpqOupHUqUa3KQpzBdbHBN1qScAeJLgIqPSIPaVz/9Sp4zL0bdzDKcz6Ulw9998e5NwPsFbYv vPgHbQ3zRkcNTm9A08O7e2E54QRjgsKgBv+bmIfgES7wa6WZKWz6QvMJe7oLktt+8F4yMut34 ZPe2HKHo3GaSt1fh3cWMw/0N7d+yIgBB+FycINw6DZv2LsZ1CUEHgG+hZ7m2kzmlGky8nZXDC 3scIgd+IY92q5TDzX6N7JR1/7BCHYCWc2fkblQtDB6s0QZKDbsO7zImHlEiQAIGRfqaaUo0NB QZJJjJd51cjXihJMx6Zmg4+3+M1yJxVBqMu6QI9R2bZQi2XuuVXLbQu6f+CKFSiNbORkblKZB H1/BDMW09L2K8+c8oi09hkfcph5K4K1UVBTfgV9jjCukO/NQrpYCb2cf/xYxMUWOHUBegVZYg ZLOWC1aeZp7HZi2Iv9AZfBCQTKFm4P40HkzWtBCy/WoI6URZGZDP83ZPORnyq9dKh1nGqX2BF aS8RLyCiUIXnObiZdBqQ0xafyYhaoHQ/vudPbGRh/lwMfvIvbFQa7rCNgizpFwcGuhez1M8Bk o+VaxlM8aYQxppoo8XmIbLZV6Yu0z2pTV0wtdFJIh2K/+rpDEwRsqfEAsjvcFRUoO50x8RZtj zbB//Zu5RliGYQgwjgViylEp6rERpBM17JfxNU6OYBjWPfHGgybwVhCAQ== X-Spam-Status: No, score=-13.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Malte Skarupke via Libc-alpha From: Malte Skarupke Reply-To: Malte Skarupke Cc: Malte Skarupke , triegel@redhat.com, malteskarupke@fastmail.fm Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" After my previous changes, __g1_start can be simpler. It can not change while a thread is in pthread_cond_wait, because groups are only allowed to switch when all threads in g1 have left that function. So there is no need to check if it has changed in pthread_cond_wait. After __g1_start was no longer read in pthread_cond_wait, it was only read in code that holds the internal lock of the condition variable. So there was no longer a need for __g1_start to be atomic. Finally, the low bit of __g1_start was only used in the block that's was supposed to handle potential stealing of signals. Since I deleted that block, we can stop shifting the count in __g1_start. --- nptl/pthread_cond_common.c | 71 ++++++------------------- nptl/pthread_cond_wait.c | 45 +++------------- sysdeps/nptl/bits/thread-shared-types.h | 10 +--- sysdeps/nptl/pthread.h | 2 +- 4 files changed, 27 insertions(+), 101 deletions(-) -- 2.25.1 diff --git a/nptl/pthread_cond_common.c b/nptl/pthread_cond_common.c index 9af6a6fbec..2d8cc7ca10 100644 --- a/nptl/pthread_cond_common.c +++ b/nptl/pthread_cond_common.c @@ -43,29 +43,16 @@ __condvar_fetch_xor_wseq_release (pthread_cond_t *cond, unsigned int val) return atomic_fetch_xor_release (&cond->__data.__wseq, val); } -static uint64_t __attribute__ ((unused)) -__condvar_load_g1_start_relaxed (pthread_cond_t *cond) -{ - return atomic_load_relaxed (&cond->__data.__g1_start); -} - -static void __attribute__ ((unused)) -__condvar_add_g1_start_relaxed (pthread_cond_t *cond, unsigned int val) -{ - atomic_store_relaxed (&cond->__data.__g1_start, - atomic_load_relaxed (&cond->__data.__g1_start) + val); -} - #else -/* We use two 64b counters: __wseq and __g1_start. They are monotonically - increasing and single-writer-multiple-readers counters, so we can implement - load, fetch-and-add, and fetch-and-xor operations even when we just have - 32b atomics. Values we add or xor are less than or equal to 1<<31 (*), - so we only have to make overflow-and-addition atomic wrt. to concurrent - load operations and xor operations. To do that, we split each counter into - two 32b values of which we reserve the MSB of each to represent an - overflow from the lower-order half to the higher-order half. +/* __wseq is a 64b counter. It is a monotonically increasing + single-writer-multiple-readers counter, so we can implement load, + fetch-and-add, and fetch-and-xor operations even when we just have 32b + atomics. Values we add or xor are less than or equal to 1<<31, so we only + have to make overflow-and-addition atomic wrt. to concurrent load + operations and xor operations. To do that, we split the counter into two + 32b values of which we reserve the MSB of each to represent an overflow + from the lower-order half to the higher-order half. In the common case, the state is (higher-order / lower-order half, and . is basically concatenation of the bits): @@ -104,11 +91,7 @@ __condvar_add_g1_start_relaxed (pthread_cond_t *cond, unsigned int val) can almost always interpret a snapshot of each halves. Readers can be forced to read a new snapshot when the read is concurrent with an overflow. However, overflows will happen infrequently, so load operations are - practically lock-free. - - (*) The highest value we add is __PTHREAD_COND_MAX_GROUP_SIZE << 2 to - __g1_start (the two extra bits are for the lock in the two LSBs of - __g1_start). */ + practically lock-free. */ typedef struct { @@ -228,20 +211,6 @@ __condvar_fetch_xor_wseq_release (pthread_cond_t *cond, unsigned int val) return ((uint64_t) h << 31) + l2; } -static uint64_t __attribute__ ((unused)) -__condvar_load_g1_start_relaxed (pthread_cond_t *cond) -{ - return __condvar_load_64_relaxed - ((_condvar_lohi *) &cond->__data.__g1_start32); -} - -static void __attribute__ ((unused)) -__condvar_add_g1_start_relaxed (pthread_cond_t *cond, unsigned int val) -{ - ignore_value (__condvar_fetch_add_64_relaxed - ((_condvar_lohi *) &cond->__data.__g1_start32, val)); -} - #endif /* !__HAVE_64B_ATOMICS */ @@ -350,7 +319,7 @@ __condvar_quiesce_and_switch_g1 (pthread_cond_t *cond, uint64_t wseq, behavior. Note that this works correctly for a zero-initialized condvar too. */ unsigned int old_orig_size = __condvar_get_orig_size (cond); - uint64_t old_g1_start = __condvar_load_g1_start_relaxed (cond) >> 1; + uint64_t old_g1_start = cond->__data.__g1_start; if (((unsigned) (wseq - old_g1_start - old_orig_size) + cond->__data.__g_size[g1 ^ 1]) == 0) return false; @@ -380,10 +349,10 @@ __condvar_quiesce_and_switch_g1 (pthread_cond_t *cond, uint64_t wseq, /* Wait until there are no group references anymore. The fetch-or operation injects us into the modification order of __g_refs; release MO ensures that waiters incrementing __g_refs after our fetch-or see the previous - changes to __g_signals and to __g1_start that had to happen before we can - switch this G1 and alias with an older group (we have two groups, so - aliasing requires switching group roles twice). Note that nobody else - can have set the wake-request flag, so we do not have to act upon it. + change to __g_signals that had to happen before we can switch this G1 + and alias with an older group (we have two groups, so aliasing requires + switching group roles twice). Note that nobody else can have set the + wake-request flag, so we do not have to act upon it. Also note that it is harmless if older waiters or waiters from this G1 get a group reference after we have quiesced the group because it will @@ -421,15 +390,9 @@ __condvar_quiesce_and_switch_g1 (pthread_cond_t *cond, uint64_t wseq, after the waiters we waited for. */ atomic_fetch_and_acquire (cond->__data.__g_refs + g1, ~(unsigned int)1); - /* Update __g1_start, which finishes closing this group. The value we add - will never be negative because old_orig_size can only be zero when we - switch groups the first time after a condvar was initialized, in which - case G1 will be at index 1 and we will add a value of 1. See above for - why this takes place after waiting for quiescence of the group. - Relaxed MO is fine because the change comes with no additional - constraints that others would have to observe. */ - __condvar_add_g1_start_relaxed (cond, - (old_orig_size << 1) + (g1 == 1 ? 1 : - 1)); + /* Update __g1_start, which finishes closing this group. See above for + why this takes place after waiting for quiescence of the group. */ + cond->__data.__g1_start += old_orig_size; /* Now reopen the group, thus enabling waiters to again block using the futex controlled by __g_signals. Release MO so that observers that see diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c index b833e543a5..939d3e9b69 100644 --- a/nptl/pthread_cond_wait.c +++ b/nptl/pthread_cond_wait.c @@ -72,7 +72,7 @@ __condvar_cancel_waiting (pthread_cond_t *cond, uint64_t seq, unsigned int g, not hold a reference on the group. */ __condvar_acquire_lock (cond, private); - uint64_t g1_start = __condvar_load_g1_start_relaxed (cond) >> 1; + uint64_t g1_start = cond->__data.__g1_start; if (g1_start > seq) { /* Our group is closed, so someone provided enough signals for it. @@ -275,9 +275,8 @@ __condvar_cleanup_waiting (void *arg) * Waiters fetch-add while having acquire the mutex associated with the condvar. Signalers load it and fetch-xor it concurrently. __g1_start: Starting position of G1 (inclusive) - * LSB is index of current G2. - * Modified by signalers while having acquired the condvar-internal lock - and observed concurrently by waiters. + * Modified by signalers and observed by waiters, both only while having + acquired the condvar-internal lock. __g1_orig_size: Initial size of G1 * The two least-significant bits represent the condvar-internal lock. * Only accessed while having acquired the condvar-internal lock. @@ -314,16 +313,6 @@ __condvar_cleanup_waiting (void *arg) A PTHREAD_COND_INITIALIZER condvar has all fields set to zero, which yields a condvar that has G2 starting at position 0 and a G1 that is closed. - Because waiters do not claim ownership of a group right when obtaining a - position in __wseq but only reference count the group when using futexes - to block, it can happen that a group gets closed before a waiter can - increment the reference count. Therefore, waiters have to check whether - their group is already closed using __g1_start. They also have to perform - this check when spinning when trying to grab a signal from __g_signals. - Note that for these checks, using relaxed MO to load __g1_start is - sufficient because if a waiter can see a sufficiently large value, it could - have also consume a signal in the waiters group. - It is essential that the last field in pthread_cond_t is __g_signals[1]: The previous condvar used a pointer-sized field in pthread_cond_t, so a PTHREAD_COND_INITIALIZER from that condvar implementation might only @@ -415,8 +404,7 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, /* Now wait until a signal is available in our group or it is closed. Acquire MO so that if we observe a value of zero written after group switching in __condvar_quiesce_and_switch_g1, we synchronize with that - store and will see the prior update of __g1_start done while switching - groups too. */ + store. */ unsigned int signals = atomic_load_acquire (cond->__data.__g_signals + g); do @@ -436,11 +424,6 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, unsigned int spin = maxspin; while (signals == 0 && spin > 0) { - /* Check that we are not spinning on a group that's already - closed. */ - if (seq < (__condvar_load_g1_start_relaxed (cond) >> 1)) - goto done; - /* TODO Back off. */ /* Reload signals. See above for MO. */ @@ -457,19 +440,7 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, if (signals != 0) break; - /* No signals available after spinning, so prepare to block. - First check the closed flag on __g_signals that designates a - concurrent attempt to reuse the group's slot. We use acquire MO for - the __g_signals check to make sure we read the current value of - __g1_start (see above). */ - if (((atomic_load_acquire (cond->__data.__g_signals + g) & 1) != 0) - || (seq < (__condvar_load_g1_start_relaxed (cond) >> 1))) - { - /* Our group is closed. */ - goto done; - } - - // Now block. + // No signals available after spinning, so block. struct _pthread_cleanup_buffer buffer; struct _condvar_cleanup_buffer cbuffer; cbuffer.wseq = wseq; @@ -501,9 +472,9 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, } } - /* Try to grab a signal. Use acquire MO so that we see an up-to-date value - of __g1_start when spinning above. */ - while (!atomic_compare_exchange_weak_acquire (cond->__data.__g_signals + g, + /* Try to grab a signal. Relaxed MO is enough because the group can't be + closed while we're in this loop, so there are no writes we could miss. */ + while (!atomic_compare_exchange_weak_relaxed (cond->__data.__g_signals + g, &signals, signals - 2)); done: diff --git a/sysdeps/nptl/bits/thread-shared-types.h b/sysdeps/nptl/bits/thread-shared-types.h index 494a43a675..cec88694ca 100644 --- a/sysdeps/nptl/bits/thread-shared-types.h +++ b/sysdeps/nptl/bits/thread-shared-types.h @@ -100,15 +100,7 @@ struct __pthread_cond_s unsigned int __high; } __wseq32; }; - __extension__ union - { - __extension__ unsigned long long int __g1_start; - struct - { - unsigned int __low; - unsigned int __high; - } __g1_start32; - }; + unsigned long long int __g1_start; unsigned int __g_refs[2] __LOCK_ALIGNMENT; unsigned int __g_size[2]; unsigned int __g1_orig_size; diff --git a/sysdeps/nptl/pthread.h b/sysdeps/nptl/pthread.h index f1b7f2bdc6..324a2d7659 100644 --- a/sysdeps/nptl/pthread.h +++ b/sysdeps/nptl/pthread.h @@ -152,7 +152,7 @@ enum /* Conditional variable handling. */ -#define PTHREAD_COND_INITIALIZER { { {0}, {0}, {0, 0}, {0, 0}, 0, 0, {0, 0} } } +#define PTHREAD_COND_INITIALIZER { { {0}, 0, {0, 0}, {0, 0}, 0, 0, {0, 0} } } /* Cleanup buffers */