From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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) From: Malte Skarupke To: libc-alpha@sourceware.org Cc: malteskarupke@fastmail.fm, triegel@redhat.com, Malte Skarupke 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== Content-Transfer-Encoding: quoted-printable 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-List-Received-Date: Sat, 16 Jan 2021 20:50:09 -0000 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. =2D-- 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(-) diff --git a/nptl/nptl-printers.py b/nptl/nptl-printers.py index e794034d83..1a404befe5 100644 =2D-- a/nptl/nptl-printers.py +++ b/nptl/nptl-printers.py @@ -313,6 +313,7 @@ class ConditionVariablePrinter(object): data =3D cond['__data'] self.wrefs =3D data['__wrefs'] + self.grefs =3D data['__g_refs'] self.values =3D [] self.read_values() @@ -350,8 +351,10 @@ class ConditionVariablePrinter(object): are waiting for it. """ + num_readers_g0 =3D self.grefs[0] >> PTHREAD_COND_GREFS_SHIFT + num_readers_g1 =3D self.grefs[1] >> PTHREAD_COND_GREFS_SHIFT self.values.append(('Threads known to still execute a wait functi= on', - 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.pys= ym index ade4398e0c..2141cfa1f0 100644 =2D-- 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 =2D-- 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 =3D atomic_load_relaxed (&cond->__data.__wrefs); - if (wrefs >> 3 =3D=3D 0) + unsigned int grefs0 =3D atomic_load_relaxed (cond->__data.__g_refs); + unsigned int grefs1 =3D atomic_load_relaxed (cond->__data.__g_refs + 1)= ; + if ((grefs0 >> 1) =3D=3D 0 && (grefs1 >> 1) =3D=3D 0) return 0; - int private =3D __condvar_get_private (wrefs); + unsigned int flags =3D atomic_load_relaxed (&cond->__data.__wrefs); + int private =3D __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 =2D-- 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 cond= var have been woken. We wait until they have confirmed to have woken up b= y - 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 confir= ming - that they finished. */ - unsigned int wrefs =3D atomic_fetch_or_acquire (&cond->__data.__wrefs, = 4); - int private =3D __condvar_get_private (wrefs); + unsigned int flags =3D atomic_load_relaxed (&cond->__data.__wrefs); + int private =3D __condvar_get_private (flags); + for (unsigned g =3D 0; g < 2; ++g) + { + while (true) + { + /* Set the wake request flag. We could also spin, but destruction tha= t is + concurrent with still-active waiters is probably neither common nor + performance critical. Acquire MO to synchronize with waiters confi= rming + that they finished. */ + unsigned r =3D atomic_fetch_or_acquire (cond->__data.__g_refs + g, 1) = | 1; + if (r =3D=3D 1) + break; + futex_wait_simple (cond->__data.__g_refs + g, r, private); + } + } + + /* Same as above, except to synchronize with canceled threads. This wa= ke + flag never gets cleared, so it's enough to set it once. */ + unsigned int wrefs =3D atomic_fetch_or_acquire (&cond->__data.__wrefs, = 4) | 4; while (wrefs >> 3 !=3D 0) { futex_wait_simple (&cond->__data.__wrefs, wrefs, private); - /* See above. */ wrefs =3D 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 =2D-- 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 =3D atomic_load_relaxed (&cond->__data.__wrefs); - if (wrefs >> 3 =3D=3D 0) + unsigned int grefs0 =3D atomic_load_relaxed (cond->__data.__g_refs); + unsigned int grefs1 =3D atomic_load_relaxed (cond->__data.__g_refs + 1)= ; + if ((grefs0 >> 1) =3D=3D 0 && (grefs1 >> 1) =3D=3D 0) return 0; - int private =3D __condvar_get_private (wrefs); + unsigned int flags =3D atomic_load_relaxed (&cond->__data.__wrefs); + int private =3D __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 =2D-- 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) a= nd we - are the last waiter (prior value of __wrefs was 1 << 3), then wake a= ny - 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) =3D=3D = 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 =3D 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 =3D cbuffer->cond; unsigned g =3D 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 increm= ent + 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) a= nd we + are the last waiter (prior value of __wrefs was 1 << 3), then wake a= ny + 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) =3D=3D = 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 th= e last reference. pthread_cond_destroy uses this as futex word. * Bit 1 is the clock ID (0 =3D=3D CLOCK_REALTIME, 1 =3D=3D CLOCK_MON= OTONIC). * Bit 0 is true iff this is a process-shared condvar. - * Simple reference count used by both waiters and pthread_cond_destr= oy. - (If the format of __wrefs is changed, update nptl_lock_constants.pys= ym - and the pretty printers.) + * Simple reference count used by __condvar_cleanup_waiting and pthre= ad_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 h= ave acquired the condvar-internal lock. + (If the format of __g_refs is changed, update nptl_lock_constants.py= sym + 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, pthr= ead_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 becau= se - we only need to synchronize when decrementing the reference count. = */ - unsigned int flags =3D atomic_fetch_add_relaxed (&cond->__data.__wrefs,= 8); + unsigned int flags =3D atomic_load_relaxed (&cond->__data.__wrefs); int private =3D __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, pthr= ead_mutex_t *mutex, if (__glibc_unlikely (err !=3D 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, pthr= ead_mutex_t *mutex, /* Confirm that we have been woken. We do that before acquiring the mu= tex 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 RE= SULT, =2D- 2.17.1