This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH][BZ #20973] Robust mutexes: Fix lost wake-up.
On Mon, 2016-12-19 at 20:47 +0100, Florian Weimer wrote:
> On 12/16/2016 11:13 PM, Torvald Riegel wrote:
> > On Fri, 2016-12-16 at 15:11 +0100, Florian Weimer wrote:
> >> On 12/15/2016 11:29 PM, Torvald Riegel wrote:
> >>> diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
> >>> index bdfa529..01ac75e 100644
> >>> --- a/nptl/pthread_mutex_lock.c
> >>> +++ b/nptl/pthread_mutex_lock.c
> >>> @@ -182,6 +182,11 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
> >>> &mutex->__data.__list.__next);
> >>>
> >>> oldval = mutex->__data.__lock;
> >>> + /* This is set to FUTEX_WAITERS iff we might have shared the
> >>
> >> “iff” doesn't seem to be correct here because it's not an exact
> >> equivalence, “if” is sufficient.
> >
> > No, I think the iff is correct. We do only set it if we may have shared
> > the flag.
>
> Then please change it to “This is set to FUTEX_WAITERS iff we have
> shared” (i.e. drop the “might”). Based on the source code, I'm still
> not sure if this is an exact equivalence.
I still think the original comment is correct. When we start, we will
not have shared, so assume_... starts as false. After we have set F_W,
we *might* have shared, so we set assume_... to true. You're right that
we might have just succeeded to acquire the lock, and then we might not
have shared, but then we're not using assume_... anymore.
> The part which confuses me is the unconditional assignment
> assume_other_futex_waiters = FUTEX_WAITERS further below. But I think
> lll_robust_lock returns 0 if we did not share FUTEX_WAITERS, and the
> code never retries with the assigned assume_other_futex_waiters value,
> ensuring the equivalence. I think it would be clearer if you switched
> from a do-while loop to a loop with an exit condition in the middle,
> right after the call to lll_robust_lock.
>
> Putting the FUTEX_WAITERS into the ID passed to lll_robust_lock is a
> violation of its precondition documented in sysdeps/nptl/lowlevellock.h,
> so please update the comment.
I've already committed the patch after what sounded like approval.
As I've said before, I plan to follow this up with a cleanup of the
whole robust mutex code. In that cleanup patch we can fine-tune the
documentation.