Re: [PATCH][BZ #20973] Robust mutexes: Fix lost wake-up.

[Florian Weimer <fweimer at redhat dot com>]

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.

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.

Thanks,
Florian