From patchwork Sat Jan 16 20:49:48 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Malte Skarupke X-Patchwork-Id: 41734 X-Patchwork-Delegate: carlos@redhat.com 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 A01F3388A432; Sat, 16 Jan 2021 20:50:12 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A01F3388A432 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1610830212; bh=0v3eEPLRWwVw9hWkkBdMcD8y9I1VXiDoxFpXwTUVmv8=; 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=ojUMHaX9H4I8dOO1plK6tXZY10lzLMHM0ikKKq3aIm+0GFYDpQJHW0X0bvfgnUkhh Zxdr3Eh4Vl/wy3YhaijS9aFlOpnZf5HfD993TGCUg1hFbqwSweZpMRy5HKCXo0vFka 8ci4Yz35RwTfVnZmk8k2oMD8abp0TgLsHpcWFAWE= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mout.web.de (mout.web.de [217.72.192.78]) by sourceware.org (Postfix) with ESMTPS id 612963836C38 for ; Sat, 16 Jan 2021 20:50:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 612963836C38 X-UI-Sender-Class: c548c8c5-30a9-4db5-a2e7-cb6cb037b8f9 Received: from auth1-smtp.messagingengine.com ([66.111.4.227]) by smtp.web.de (mrweb105 [213.165.67.124]) with ESMTPSA (Nemesis) id 1M3m9J-1l03RE1jQB-0012jL; Sat, 16 Jan 2021 21:49:59 +0100 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailauth.nyi.internal (Postfix) with ESMTP id 97FB827C005A; Sat, 16 Jan 2021 15:49:57 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute3.internal (MEProxy); Sat, 16 Jan 2021 15:49:57 -0500 X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrtdeggddugeehucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpefhvffufffkofgjfhestddtredtre dttdenucfhrhhomhepofgrlhhtvgcuufhkrghruhhpkhgvuceomhgrlhhtvghskhgrrhhu phhkvgesfigvsgdruggvqeenucggtffrrghtthgvrhhnpeduffeufedtudehhfffieejfe ehfeefledvffffffehhfefffelffeiudelteehfeenucfkphepieejrddvheegrdduieeh rdelnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmh grlhhtvghskhgrrhhuphhkvgdomhgvshhmthhprghuthhhphgvrhhsohhnrghlihhthidq uddtudejtdefvdeliedqudduvdegvdekieelqdhmrghlthgvshhkrghruhhpkhgvpeepfi gvsgdruggvsehfrghsthhmrghilhdrfhhm X-ME-Proxy: Received: from localhost.localdomain (unknown [67.254.165.9]) by mail.messagingengine.com (Postfix) with ESMTPA id 4887E108005F; Sat, 16 Jan 2021 15:49:57 -0500 (EST) To: libc-alpha@sourceware.org Subject: [PATCH 3/5] nptl: Optimization by not incrementing wrefs in pthread_cond_wait Date: Sat, 16 Jan 2021 15:49:48 -0500 Message-Id: <20210116204950.16434-3-malteskarupke@web.de> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20210116204950.16434-1-malteskarupke@web.de> References: <20210116204950.16434-1-malteskarupke@web.de> X-Provags-ID: V03:K1:PePhyC8Et9Qvd2YOx8/iaET6VZOvtJxHHwaI+S40W+dCVXpxpIN yyFRshoXqGFZAsbSSr+SDBtN059ZRQaLpI/9TE8xcVutDosG+PY8Y8pRXfEYRUl75NafoNJ uCbu2hd98Ex03m0sIZra/x0tvjhA5Qa8HyTH8wz2HQnihbhqkvZ9zfO52f7gIyw0sA4ql5v NMlfQwxHlCyAJkCNDpXUg== X-UI-Out-Filterresults: notjunk:1;V03:K0:oVqjzljuRuc=:mHsl+Uf0Z3O1gVg3jbUc4m hcZNis9LTY3gjrLVQEgllUwCMgh34vE5zbAlU8DTyLdPaGrst4vmhbJapC5xEcwrLljQ7qTSS y33e7i/CsALaqkfcRQ8ZvqwTN5Cwu6lNdKWBH4NR/jwkkchlx+sk2i/HM6P6K1WYxiPSEIAAs hISXArqcygYsv0t1FEzM5AHXNbETXcRKO9ZtNvXvan9I5LH7q0f8htJvhBXLikDYbmfX9//wT mH2DZbPNxCNBzrmco94gTJqWv/op9ycKEN7PaXSW3bWhKRP8JAmGdsUeLLdcKDTTWKV4Er7FP ekattZnQDuEi4Ypqeh1djjF9UTum2Ym7eG2BeX6EsIlJj+HqtaDYPWVqZ0G2N6144EIZ5yYIk ErMgrKe18r152AR6qemLiDnfJSTTyf9m19Tv6FhRlgkFE/YD5uVMhLOkRJu5+7rjHqDUdUog2 +/TeeYE+GOF3+HA7aI0fwHToBcS+AvdyNiTOczo/GA7jIU8BpfFMagv7T09umulhi0Jo7zPHc EhTfO/LYtUjKoICvRRwtmjOoAlnjI7VcfOLODZQdL1yXbdSvGJEQS8xSjrFnqVCdu2N2QXzQB xbv9XDjtibb6HK/j4QoLCGAwUJAU/4GLe7RAPJLPQw+TZq72EK6CVRMFJ72eXl6wUQH++HDmV z5saglqKcqVGf5CbpmTAvXSRMjeOYVqPHLMTy4EE/tSHXD2/gxuBMBs8PH0rTJqCNl/sX8+Ns pINpTy/hCjppzR5SfnKupPMoktXXbUKx9Mwh91aGVSQCDN9e3EbeeD11nuFrxi7v+pd+h29QH ZgZlSsPG5sAZzn93bKBXBi+z1/JfcD7/XAl30tsv6S+818lZyX7l19JmJqwNeNtP1JB/X1/tu VwLCbQFq+FyEDyKpleRw== X-Spam-Status: No, score=-13.9 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_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: 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@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. --- nptl/nptl-printers.py | 5 +++- nptl/nptl_lock_constants.pysym | 2 +- nptl/pthread_cond_broadcast.c | 8 +++--- nptl/pthread_cond_destroy.c | 29 ++++++++++++++++------ nptl/pthread_cond_signal.c | 8 +++--- nptl/pthread_cond_wait.c | 45 ++++++++++++++++------------------ 6 files changed, 57 insertions(+), 40 deletions(-) -- 2.17.1 diff --git a/nptl/nptl-printers.py b/nptl/nptl-printers.py index e794034d83..1a404befe5 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 8d887aab93..e10432ce7c 100644 --- a/nptl/pthread_cond_broadcast.c +++ b/nptl/pthread_cond_broadcast.c @@ -40,10 +40,12 @@ __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) + 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_destroy.c b/nptl/pthread_cond_destroy.c index 31034905d1..1c27385f89 100644 --- a/nptl/pthread_cond_destroy.c +++ b/nptl/pthread_cond_destroy.c @@ -37,22 +37,35 @@ 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) | 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 4281ad4d3b..0cd534cc40 100644 --- a/nptl/pthread_cond_signal.c +++ b/nptl/pthread_cond_signal.c @@ -39,10 +39,12 @@ __pthread_cond_signal (pthread_cond_t *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) + 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_wait.c b/nptl/pthread_cond_wait.c index 7b825f81df..0ee0247874 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 @@ -81,7 +68,7 @@ __condvar_cancel_waiting (pthread_cond_t *cond, uint64_t seq, unsigned int g, { bool consumed_signal = false; - /* No deadlock with group switching is possible here because we have do + /* No deadlock with group switching is possible here because we do not hold a reference on the group. */ __condvar_acquire_lock (cond, private); @@ -172,6 +159,14 @@ __condvar_cleanup_waiting (void *arg) pthread_cond_t *cond = cbuffer->cond; unsigned g = cbuffer->wseq & 1; + /* Normally we are not allowed to touch cond any more 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 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 +178,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 +287,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. @@ -409,9 +410,7 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, synchronize with the dummy read-modify-write in __condvar_quiesce_and_switch_g1 if we read from that. */ atomic_fetch_add_acquire (cond->__data.__g_refs + g, 2); - /* 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); + 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. @@ -425,7 +424,6 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, if (__glibc_unlikely (err != 0)) { __condvar_cancel_waiting (cond, seq, g, private); - __condvar_confirm_wakeup (cond, private); __condvar_dec_grefs (cond, g, private); return err; } @@ -530,7 +528,6 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex, /* 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_confirm_wakeup (cond, private); __condvar_dec_grefs (cond, g, private); /* Woken up; now re-acquire the mutex. If this doesn't fail, return RESULT,