180 lines
8.8 KiB
Diff
180 lines
8.8 KiB
Diff
commit 91bb902f58264a2fd50fbce8f39a9a290dd23706
|
|
Author: Malte Skarupke <malteskarupke@fastmail.fm>
|
|
Date: Wed Dec 4 08:05:40 2024 -0500
|
|
|
|
nptl: Use all of g1_start and g_signals
|
|
|
|
The LSB of g_signals was unused. The LSB of g1_start was used to indicate
|
|
which group is G2. This was used to always go to sleep in pthread_cond_wait
|
|
if a waiter is in G2. A comment earlier in the file says that this is not
|
|
correct to do:
|
|
|
|
"Waiters cannot determine whether they are currently in G2 or G1 -- but they
|
|
do not have to because all they are interested in is whether there are
|
|
available signals"
|
|
|
|
I either would have had to update the comment, or get rid of the check. I
|
|
chose to get rid of the check. In fact I don't quite know why it was there.
|
|
There will never be available signals for group G2, so we didn't need the
|
|
special case. Even if there were, this would just be a spurious wake. This
|
|
might have caught some cases where the count has wrapped around, but it
|
|
wouldn't reliably do that, (and even if it did, why would you want to force a
|
|
sleep in that case?) and we don't support that many concurrent waiters
|
|
anyway. Getting rid of it allows us to use one more bit, making us more
|
|
robust to wraparound.
|
|
|
|
Signed-off-by: Malte Skarupke <malteskarupke@fastmail.fm>
|
|
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
|
|
|
|
diff --git a/nptl/pthread_cond_broadcast.c b/nptl/pthread_cond_broadcast.c
|
|
index 3dff819952718892..6fd6cfe9d002c5d5 100644
|
|
--- a/nptl/pthread_cond_broadcast.c
|
|
+++ b/nptl/pthread_cond_broadcast.c
|
|
@@ -58,7 +58,7 @@ ___pthread_cond_broadcast (pthread_cond_t *cond)
|
|
{
|
|
/* Add as many signals as the remaining size of the group. */
|
|
atomic_fetch_add_relaxed (cond->__data.__g_signals + g1,
|
|
- cond->__data.__g_size[g1] << 1);
|
|
+ cond->__data.__g_size[g1]);
|
|
cond->__data.__g_size[g1] = 0;
|
|
|
|
/* We need to wake G1 waiters before we switch G1 below. */
|
|
@@ -74,7 +74,7 @@ ___pthread_cond_broadcast (pthread_cond_t *cond)
|
|
{
|
|
/* Step (3): Send signals to all waiters in the old G2 / new G1. */
|
|
atomic_fetch_add_relaxed (cond->__data.__g_signals + g1,
|
|
- cond->__data.__g_size[g1] << 1);
|
|
+ cond->__data.__g_size[g1]);
|
|
cond->__data.__g_size[g1] = 0;
|
|
/* TODO Only set it if there are indeed futex waiters. */
|
|
do_futex_wake = true;
|
|
diff --git a/nptl/pthread_cond_common.c b/nptl/pthread_cond_common.c
|
|
index 7b2b1e4605f163e7..485aca4076a372d7 100644
|
|
--- a/nptl/pthread_cond_common.c
|
|
+++ b/nptl/pthread_cond_common.c
|
|
@@ -348,9 +348,9 @@ __condvar_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;
|
|
- if (((unsigned) (wseq - old_g1_start - old_orig_size)
|
|
- + cond->__data.__g_size[g1 ^ 1]) == 0)
|
|
+ uint64_t old_g1_start = __condvar_load_g1_start_relaxed (cond);
|
|
+ uint64_t new_g1_start = old_g1_start + old_orig_size;
|
|
+ if (((unsigned) (wseq - new_g1_start) + cond->__data.__g_size[g1 ^ 1]) == 0)
|
|
return false;
|
|
|
|
/* We have to consider the following kinds of waiters:
|
|
@@ -361,16 +361,10 @@ __condvar_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 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));
|
|
-
|
|
- unsigned int lowseq = ((old_g1_start + old_orig_size) << 1) & ~1U;
|
|
+ /* Update __g1_start, which closes this 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);
|
|
|
|
/* At this point, the old G1 is now a valid new G2 (but not in use yet).
|
|
No old waiter can neither grab a signal nor acquire a reference without
|
|
@@ -382,13 +376,13 @@ __condvar_switch_g1 (pthread_cond_t *cond, uint64_t wseq,
|
|
g1 ^= 1;
|
|
*g1index ^= 1;
|
|
|
|
- /* Now advance the new G1 g_signals to the new lowseq, giving it
|
|
+ /* Now advance the new G1 g_signals to the new g1_start, giving it
|
|
an effective signal count of 0 to start. */
|
|
- atomic_store_release (cond->__data.__g_signals + g1, lowseq);
|
|
+ atomic_store_release (cond->__data.__g_signals + g1, (unsigned)new_g1_start);
|
|
|
|
/* These values are just observed by signalers, and thus protected by the
|
|
lock. */
|
|
- unsigned int orig_size = wseq - (old_g1_start + old_orig_size);
|
|
+ unsigned int orig_size = wseq - new_g1_start;
|
|
__condvar_set_orig_size (cond, orig_size);
|
|
/* Use and addition to not loose track of cancellations in what was
|
|
previously G2. */
|
|
diff --git a/nptl/pthread_cond_signal.c b/nptl/pthread_cond_signal.c
|
|
index 4f7639e386fc207a..9a5bac92fe8fc246 100644
|
|
--- a/nptl/pthread_cond_signal.c
|
|
+++ b/nptl/pthread_cond_signal.c
|
|
@@ -81,7 +81,7 @@ ___pthread_cond_signal (pthread_cond_t *cond)
|
|
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);
|
|
+ atomic_fetch_add_relaxed (cond->__data.__g_signals + g1, 1);
|
|
cond->__data.__g_size[g1]--;
|
|
/* TODO Only set it if there are indeed futex waiters. */
|
|
do_futex_wake = true;
|
|
diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c
|
|
index 173bd134164eed44..944b241ec26b9b32 100644
|
|
--- a/nptl/pthread_cond_wait.c
|
|
+++ b/nptl/pthread_cond_wait.c
|
|
@@ -85,7 +85,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 = __condvar_load_g1_start_relaxed (cond);
|
|
if (g1_start > seq)
|
|
{
|
|
/* Our group is closed, so someone provided enough signals for it.
|
|
@@ -260,7 +260,6 @@ __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.
|
|
__g1_orig_size: Initial size of G1
|
|
@@ -281,11 +280,9 @@ __condvar_cleanup_waiting (void *arg)
|
|
* Reference count used by waiters concurrently with signalers that have
|
|
acquired the condvar-internal lock.
|
|
__g_signals: The number of signals that can still be consumed, relative to
|
|
- the current g1_start. (i.e. bits 31 to 1 of __g_signals are bits
|
|
- 31 to 1 of g1_start with the signal count added)
|
|
+ the current g1_start. (i.e. g1_start with the signal count added)
|
|
* Used as a futex word by waiters. Used concurrently by waiters and
|
|
signalers.
|
|
- * LSB is currently reserved and 0.
|
|
__g_size: Waiters remaining in this group (i.e., which have not been
|
|
signaled yet.
|
|
* Accessed by signalers and waiters that cancel waiting (both do so only
|
|
@@ -392,9 +389,8 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex,
|
|
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;
|
|
|
|
- if (seq < (g1_start >> 1))
|
|
+ if (seq < g1_start)
|
|
{
|
|
/* If the group is closed already,
|
|
then this waiter originally had enough extra signals to
|
|
@@ -407,13 +403,13 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex,
|
|
by now, perhaps in the process of switching back to an older
|
|
G2, but in either case we're allowed to consume the available
|
|
signal and should not block anymore. */
|
|
- if ((int)(signals - lowseq) >= 2)
|
|
+ if ((int)(signals - (unsigned int)g1_start) > 0)
|
|
{
|
|
/* Try to grab a signal. See above for MO. (if we do another loop
|
|
iteration we need to see the correct value of g1_start) */
|
|
if (atomic_compare_exchange_weak_acquire (
|
|
cond->__data.__g_signals + g,
|
|
- &signals, signals - 2))
|
|
+ &signals, signals - 1))
|
|
break;
|
|
else
|
|
continue;
|