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

[Torvald Riegel <triegel@redhat.com>]

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.