[RFC] nptl: On read lock timeout for a rwlock, wake up pending readers
Checks
| Context |
Check |
Description |
| redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
| linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
warning
|
Skipped because it is an RFC
|
| linaro-tcwg-bot/tcwg_glibc_build--master-arm |
warning
|
Skipped because it is an RFC
|
Commit Message
I've been looking at a nptl/tst-rwlock9 failure. So far, I've not been
able to reproduce this outside of our build system.
LLMs however identified that unregistration upon timeout during rdlock
does not wake up other readers, so that they can complete their
registration. I wrote the patch below (based on
__pthread_rwlock_rdunlock) to cover this gap.
Do you think this fixes an actual bug?
Thanks,
Florian
---
nptl/pthread_rwlock_common.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
base-commit: adb18e934774ab454b244077d3123206d2e893c6
Comments
On 9/21/26 11:01 AM, Florian Weimer wrote:
> I've been looking at a nptl/tst-rwlock9 failure. So far, I've not been
> able to reproduce this outside of our build system.
>
> LLMs however identified that unregistration upon timeout during rdlock
> does not wake up other readers, so that they can complete their
> registration. I wrote the patch below (based on
> __pthread_rwlock_rdunlock) to cover this gap.
>
> Do you think this fixes an actual bug?
I haven't seen anyone answer this question, so I'll step up and make some comments.
What you have provided is not sufficient to show the failure.
That is to say that for us to claim this fixes it we need to talk about the total
global order that causes this issue.
It takes effort to write such a summary up, but it is required as part of the analysis
to show that such an ordering exists.
I have had success in guiding an LLM to explore the total global ordering, and verifying
that such an ordering is possible. Such orderings are hard for humans to explore because
of the amount of state space usually required.
In this case to verify this I would have to do the above, reviewing the existing algorithm
and the requirements it is supposed to provide against the change you're making.
I don't know if this fixes an actual bug.
I think the next step though is to write a test case that actually exercise the rwlock
as an open-box test modifying the algorithm to get into this state and then releasing
a thread to create the issue.
I wrote similar open-box testes for condvar when working on the lost signal part and
it requires tests-internal, and some helper functions, but it wasn't overly onerous
once you had macros in place.
In summary:
- Suggest trying to write an open-box test.
- Suggest trying to guide an LLM to explore the global ordering that causes the problem
and provide the justification for a human reviewer to review and validate.
> Thanks,
> Florian
> ---
> nptl/pthread_rwlock_common.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/nptl/pthread_rwlock_common.c b/nptl/pthread_rwlock_common.c
> index 9b0b528298..295a5fbfc1 100644
> --- a/nptl/pthread_rwlock_common.c
> +++ b/nptl/pthread_rwlock_common.c
> @@ -467,13 +467,27 @@ __pthread_rwlock_rdlock_full64 (pthread_rwlock_t *rwlock, clockid_t clockid,
> r = atomic_load_relaxed (&rwlock->__data.__readers);
> while ((r & PTHREAD_RWLOCK_WRPHASE) != 0)
> {
> + /* If we are the last reader, we also need to
> + unblock any readers that are waiting for a writer
> + to go first (PTHREAD_RWLOCK_RWAITING) so that
> + they can register while the writer is active.
> + See __pthread_rwlock_rdunlock above. */
> + unsigned int rnew = r - (1 << PTHREAD_RWLOCK_READER_SHIFT);
> + if ((rnew >> PTHREAD_RWLOCK_READER_SHIFT) == 0)
> + rnew &= ~(unsigned int) PTHREAD_RWLOCK_RWAITING;
> +
> /* We don't need to make anything else visible to
> others besides unregistering, so relaxed MO is
> sufficient. */
> if (atomic_compare_exchange_weak_relaxed
> - (&rwlock->__data.__readers, &r,
> - r - (1 << PTHREAD_RWLOCK_READER_SHIFT)))
> - return err;
> + (&rwlock->__data.__readers, &r, rnew))
> + {
> + if ((r & PTHREAD_RWLOCK_RWAITING)
> + != (rnew & PTHREAD_RWLOCK_RWAITING))
> + futex_wake (&rwlock->__data.__readers, INT_MAX,
> + private);
> + return err;
> + }
> /* TODO Back-off. */
> }
> /* Use the acquire MO fence to mirror the steps taken in the
>
> base-commit: adb18e934774ab454b244077d3123206d2e893c6
>
@@ -467,13 +467,27 @@ __pthread_rwlock_rdlock_full64 (pthread_rwlock_t *rwlock, clockid_t clockid,
r = atomic_load_relaxed (&rwlock->__data.__readers);
while ((r & PTHREAD_RWLOCK_WRPHASE) != 0)
{
+ /* If we are the last reader, we also need to
+ unblock any readers that are waiting for a writer
+ to go first (PTHREAD_RWLOCK_RWAITING) so that
+ they can register while the writer is active.
+ See __pthread_rwlock_rdunlock above. */
+ unsigned int rnew = r - (1 << PTHREAD_RWLOCK_READER_SHIFT);
+ if ((rnew >> PTHREAD_RWLOCK_READER_SHIFT) == 0)
+ rnew &= ~(unsigned int) PTHREAD_RWLOCK_RWAITING;
+
/* We don't need to make anything else visible to
others besides unregistering, so relaxed MO is
sufficient. */
if (atomic_compare_exchange_weak_relaxed
- (&rwlock->__data.__readers, &r,
- r - (1 << PTHREAD_RWLOCK_READER_SHIFT)))
- return err;
+ (&rwlock->__data.__readers, &r, rnew))
+ {
+ if ((r & PTHREAD_RWLOCK_RWAITING)
+ != (rnew & PTHREAD_RWLOCK_RWAITING))
+ futex_wake (&rwlock->__data.__readers, INT_MAX,
+ private);
+ return err;
+ }
/* TODO Back-off. */
}
/* Use the acquire MO fence to mirror the steps taken in the