[v4,8/9] nptl: rename __condvar_quiesce_and_switch_g1

Message ID 20230509175558.10014-9-malteskarupke@fastmail.fm
State Superseded
Headers
Series Patch to fix glibc condition variable bug (BZ 25847) |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

develop--- via Libc-alpha May 9, 2023, 5:55 p.m. UTC
  From: Malte Skarupke <malteskarupke@fastmail.fm>

This function no longer waits for threads to leave g1, so rename it to
__condvar_switch_g1

Signed-off-by: Malte Skarupke <malteskarupke@fastmail.fm>
---
 nptl/pthread_cond_broadcast.c |  4 ++--
 nptl/pthread_cond_common.c    | 26 ++++++++++++--------------
 nptl/pthread_cond_signal.c    | 17 ++++++++---------
 nptl/pthread_cond_wait.c      |  9 ++++-----
 4 files changed, 26 insertions(+), 30 deletions(-)
  

Patch

diff --git a/nptl/pthread_cond_broadcast.c b/nptl/pthread_cond_broadcast.c
index 5ae141ac81..a07435589a 100644
--- a/nptl/pthread_cond_broadcast.c
+++ b/nptl/pthread_cond_broadcast.c
@@ -60,7 +60,7 @@  ___pthread_cond_broadcast (pthread_cond_t *cond)
 				cond->__data.__g_size[g1] << 1);
       cond->__data.__g_size[g1] = 0;
 
-      /* We need to wake G1 waiters before we quiesce G1 below.  */
+      /* We need to wake G1 waiters before we switch G1 below.  */
       /* TODO Only set it if there are indeed futex waiters.  We could
 	 also try to move this out of the critical section in cases when
 	 G2 is empty (and we don't need to quiesce).  */
@@ -69,7 +69,7 @@  ___pthread_cond_broadcast (pthread_cond_t *cond)
 
   /* G1 is complete.  Step (2) is next unless there are no waiters in G2, in
      which case we can stop.  */
-  if (__condvar_quiesce_and_switch_g1 (cond, wseq, &g1, private))
+  if (__condvar_switch_g1 (cond, wseq, &g1, private))
     {
       /* Step (3): Send signals to all waiters in the old G2 / new G1.  */
       atomic_fetch_add_relaxed (cond->__data.__g_signals + g1,
diff --git a/nptl/pthread_cond_common.c b/nptl/pthread_cond_common.c
index f976a533a1..3baac4dabc 100644
--- a/nptl/pthread_cond_common.c
+++ b/nptl/pthread_cond_common.c
@@ -189,16 +189,15 @@  __condvar_get_private (int flags)
     return FUTEX_SHARED;
 }
 
-/* This closes G1 (whose index is in G1INDEX), waits for all futex waiters to
-   leave G1, converts G1 into a fresh G2, and then switches group roles so that
-   the former G2 becomes the new G1 ending at the current __wseq value when we
-   eventually make the switch (WSEQ is just an observation of __wseq by the
-   signaler).
+/* This closes G1 (whose index is in G1INDEX), converts G1 into a fresh G2,
+   and then switches group roles so that the former G2 becomes the new G1
+   ending at the current __wseq value when we eventually make the switch
+   (WSEQ is just an observation of __wseq by the signaler).
    If G2 is empty, it will not switch groups because then it would create an
    empty G1 which would require switching groups again on the next signal.
    Returns false iff groups were not switched because G2 was empty.  */
 static bool __attribute__ ((unused))
-__condvar_quiesce_and_switch_g1 (pthread_cond_t *cond, uint64_t wseq,
+__condvar_switch_g1 (pthread_cond_t *cond, uint64_t wseq,
     unsigned int *g1index, int private)
 {
   unsigned int g1 = *g1index;
@@ -214,8 +213,7 @@  __condvar_quiesce_and_switch_g1 (pthread_cond_t *cond, uint64_t wseq,
 	  + cond->__data.__g_size[g1 ^ 1]) == 0)
 	return false;
 
-  /* Now try to close and quiesce G1.  We have to consider the following kinds
-     of waiters:
+  /* We have to consider the following kinds of waiters:
      * Waiters from less recent groups than G1 are not affected because
        nothing will change for them apart from __g1_start getting larger.
      * New waiters arriving concurrently with the group switching will all go
@@ -223,12 +221,12 @@  __condvar_quiesce_and_switch_g1 (pthread_cond_t *cond, uint64_t wseq,
        are not affected.
      * Waiters in G1 have already received a signal and been woken.  */
 
-  /* 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.
-     Relaxed MO is fine because the change comes with no additional
-     constraints that others would have to observe.  */
+  /* Update __g1_start, which closes 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. 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));
 
diff --git a/nptl/pthread_cond_signal.c b/nptl/pthread_cond_signal.c
index 14800ba00b..a9bc10dcca 100644
--- a/nptl/pthread_cond_signal.c
+++ b/nptl/pthread_cond_signal.c
@@ -69,18 +69,17 @@  ___pthread_cond_signal (pthread_cond_t *cond)
   bool do_futex_wake = false;
 
   /* If G1 is still receiving signals, we put the signal there.  If not, we
-     check if G2 has waiters, and if so, quiesce and switch G1 to the former
-     G2; if this results in a new G1 with waiters (G2 might have cancellations
-     already, see __condvar_quiesce_and_switch_g1), we put the signal in the
-     new G1.  */
+     check if G2 has waiters, and if so, switch G1 to the former G2; if this
+     results in a new G1 with waiters (G2 might have cancellations already,
+     see __condvar_switch_g1), we put the signal in the new G1. */
   if ((cond->__data.__g_size[g1] != 0)
-      || __condvar_quiesce_and_switch_g1 (cond, wseq, &g1, private))
+      || __condvar_switch_g1 (cond, wseq, &g1, private))
     {
       /* Add a signal.  Relaxed MO is fine because signaling does not need to
-	 establish a happens-before relation (see above).  We do not mask the
-	 release-MO store when initializing a group in
-	 __condvar_quiesce_and_switch_g1 because we use an atomic
-	 read-modify-write and thus extend that store's release sequence.  */
+         establish a happens-before relation (see above).  We do not mask the
+         release-MO store when initializing a group in __condvar_switch_g1
+         because we use an atomic read-modify-write and thus extend that
+         store's release sequence.  */
       atomic_fetch_add_relaxed (cond->__data.__g_signals + g1, 2);
       cond->__data.__g_size[g1]--;
       /* TODO Only set it if there are indeed futex waiters.  */
diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c
index bf05ac6b22..346880c5a7 100644
--- a/nptl/pthread_cond_wait.c
+++ b/nptl/pthread_cond_wait.c
@@ -354,8 +354,7 @@  __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex,
      because we do not need to establish any happens-before relation with
      signalers (see __pthread_cond_signal); modification order alone
      establishes a total order of waiters/signals.  We do need acquire MO
-     to synchronize with group reinitialization in
-     __condvar_quiesce_and_switch_g1.  */
+     to synchronize with group reinitialization in __condvar_switch_g1.  */
   uint64_t wseq = __condvar_fetch_add_wseq_acquire (cond, 2);
   /* Find our group's index.  We always go into what was G2 when we acquired
      our position.  */
@@ -387,9 +386,9 @@  __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 (signals == lowseq) 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.  */
+         switching in __condvar_switch_g1, we synchronize with that store and
+         will see the prior update of __g1_start done while switching groups
+         too.  */
       unsigned int signals = atomic_load_acquire (cond->__data.__g_signals + g);
       uint64_t g1_start = __condvar_load_g1_start_relaxed (cond);
       unsigned int lowseq = (g1_start & 1) == g ? signals : g1_start & ~1U;