[5/5] nptl: Rename __wrefs to __flags because its meaning has changed

Message ID 20210116204950.16434-5-malteskarupke@web.de
State Superseded
Delegated to: Carlos O'Donell
Headers
Series [1/5] nptl: Fix pthread_cond_signal missing a sleeper (#BZ 25847) |

Commit Message

Malte Skarupke Jan. 16, 2021, 8:49 p.m. UTC
  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. So the name "wrefs" is no longer accurate. It is still used
as a reference count in an edge case, in the interaction between
pthread_cancel and pthread_cond_destroy, but that edge case shouldn't
be what this variable is named after.

The name __flags seems more appropriate since in the most common case
this variable is just used to store the flags of the condition
variable.
---
 nptl/nptl-printers.py                   |  6 +++---
 nptl/pthread_cond_broadcast.c           |  2 +-
 nptl/pthread_cond_common.c              |  4 ++--
 nptl/pthread_cond_destroy.c             |  8 ++++----
 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, 24 insertions(+), 24 deletions(-)

--
2.17.1
  

Comments

Torvald Riegel Jan. 18, 2021, 11:47 p.m. UTC | #1
On Sat, 2021-01-16 at 15:49 -0500, Malte Skarupke wrote:
> 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. So the name "wrefs" is no longer accurate. It is still used
> as a reference count in an edge case, in the interaction between
> pthread_cancel and pthread_cond_destroy, but that edge case shouldn't
> be what this variable is named after.

I don't think that this change is good.  Cancellation is not
necessarily an "edge case" because it will happen whenever timeouts are
involved.  More importantly, the wake-up flag is an integral part of
the refcount.  The other flags in there are just in there because
available space was scarce (ABI...).  So if you want to rename it, I'd
make it "cancellation_refs" or "crefs" or something like that, not just
"flags".
  

Patch

diff --git a/nptl/nptl-printers.py b/nptl/nptl-printers.py
index 1a404befe5..0e37d8b4dd 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.flags = data['__flags']
         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.flags & 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.flags & 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 e10432ce7c..cbb249b35b 100644
--- a/nptl/pthread_cond_broadcast.c
+++ b/nptl/pthread_cond_broadcast.c
@@ -44,7 +44,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.__flags);
   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 3251c7f0ec..22270e8805 100644
--- a/nptl/pthread_cond_common.c
+++ b/nptl/pthread_cond_common.c
@@ -20,7 +20,7 @@ 
 #include <stdint.h>
 #include <pthread.h>

-/* We need 3 least-significant bits on __wrefs for something else.  */
+/* We need 3 least-significant bits on __flags 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 __flags
    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 1c27385f89..3dfe4a48db 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.__flags);
   int private = __condvar_get_private (flags);
   for (unsigned g = 0; g < 2; ++g)
     {
@@ -62,11 +62,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;
+  unsigned int wrefs = atomic_fetch_or_acquire (&cond->__data.__flags, 4) | 4;
   while (wrefs >> 3 != 0)
     {
-      futex_wait_simple (&cond->__data.__wrefs, wrefs, private);
-      wrefs = atomic_load_acquire (&cond->__data.__wrefs);
+      futex_wait_simple (&cond->__data.__flags, wrefs, private);
+      wrefs = atomic_load_acquire (&cond->__data.__flags);
     }
   /* 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 595b1b3528..3031d52a42 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.__flags |= __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.__flags |= __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 0cd534cc40..979c9d72d5 100644
--- a/nptl/pthread_cond_signal.c
+++ b/nptl/pthread_cond_signal.c
@@ -43,7 +43,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.__flags);
   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 0ee0247874..0993728e5d 100644
--- a/nptl/pthread_cond_wait.c
+++ b/nptl/pthread_cond_wait.c
@@ -164,9 +164,9 @@  __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 fourth bit on __wrefs. Relaxed MO is enough. The synchronization
+     the fourth bit on __flags. 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.__flags, 8);
   __condvar_dec_grefs (cond, g, cbuffer->private);

   __condvar_cancel_waiting (cond, cbuffer->wseq >> 1, g, cbuffer->private);
@@ -182,8 +182,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.__flags, -8) >> 2) == 3)
+    futex_wake (&cond->__data.__flags, 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,13 +287,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.
+   __flags: 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 __flags 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
@@ -410,7 +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);
-  unsigned int flags = atomic_load_relaxed (&cond->__data.__wrefs);
+  unsigned int flags = atomic_load_relaxed (&cond->__data.__flags);
   int private = __condvar_get_private (flags);

   /* Now that we are registered as a waiter, we can release the mutex.
@@ -558,7 +558,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.__flags);
   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..e1338ebf94 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.__flags);

   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.__flags);

   return status;
 }
diff --git a/sysdeps/nptl/bits/thread-shared-types.h b/sysdeps/nptl/bits/thread-shared-types.h
index fbbdd0bb36..84cedfcaa0 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 __flags;
   unsigned int __g_signals[2];
 };