Re: [PATCH][BZ #14417] Unlock mutex before going back to waitingfor PI mutexes

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Sat, 29 Sep 2012 20:32:32 +0200, Torvald wrote:
> > +       /* We need to go back to futex_wait.  If we're using
> > requeue_pi, then
> > +          release the mutex we had acquired and go back.  */
> 
> I think the point here is not just that we're using requeue_pi, but
> that we did that AND 16(%esp) is true iff the syscall did not return
> EAGAIN. Then I'd prefer to either have a comment why we need to do
> this (in comparison to the non-PI-aware base algorithm), or refer to
> the x86_64 code for additional docs.

The whole code block marked by '22' up to (and not including) the label
'1' is for generic jumping back to '8'.  The PI specific section is only
the code that adjusts the mutex value and then jumps back, i.e. the code
after (and not including):

> > +22:    movl    16(%esp), %edx
> > +       test    %edx, %edx
> > +       jz      8b 

> Did you test with __ASSUME_REQUEUE_PI defined?  Shouldn't this jump be
> after the EAGAIN checks?  Or is this because you only expect ENOSYS as
> only other error code you need to handle here?  A comment perhaps?

I didn't test with __ASSUME_REQUEUE_PI.  The jump is correct however,
because the behaviour with -EAGAIN (or with any other errno except
ENOSYS) is as if we did a usual futex_wait, so simply having the %r8b
flag being 0 is sufficient.  We do that when we say:

>	cmpl $0, %eax
>	sete %r8b

The conditional jump is only useful when we want to fall back to
futex_wait for ENOSYS.

> We don't race for the futex (what does that mean?).  I think we should
> say "if the futex_wait was interrupted by a change to the futex
> variable" or something like that.

I meant what you said, so I'll just update the comment.

> > +          an EAGAIN.  Act as if you did a regular FUTEX_WAIT and
> > returned
> > +          successfully from it.  Let the sequence numbers hold you
> > back if
> > +          you were not supposed to be woken up.  That way, you
> > don't forget
> > +          to take the mutex lock on your way out.
> 
> IMHO, I find that hard to understand.  Perhaps be a bit more verbose?
> Just pointing out the differences to the non-PI base algorithm might
> also be useful.

OK, will give this blurb another shot.

> I would be surprised if a single branch after a syscall would be a
> measurable performance degradation.  Not sure what the policy for this
> is, but I'd prefer an abort or such if we get a return value we didn't
> expect.  Also, if we get frequent spurious wake-ups, then this branch
> is the smallest part of the overhead.

I got the impression in the past that the nptl code had to be as
trim as possible, which is why I left it out on purpose.  If the abort
is OK in this for others, I'll put it in.

Regards,
Siddhesh