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

[Torvald Riegel <triegel at redhat dot com>]

On Mon, 2012-10-01 at 11:21 +0530, Siddhesh Poyarekar wrote:
> 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 

I'm not saying the code is wrong.  What I'm saying is that your comment
could be improved.  It leaves out what 16(%esp) represent (precisely!)
and readers have to trace it down to find out.  Second, you don't
summarize how the algorithm works; the easiest way to do that would be
to summarize the difference to the non-PI-aware algorithm, or link to
such a summary when you want to put it elsewhere.

We spent enough time on this in the last days, as you'll know.  Let's
try to take some extra time to improve the comments so that the people
who will work on this next don't have to spend that much time.

> > 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.

Look, I figured that out, obviously.  But that is not the point.  We can
make this easier to understand for whoever reads the code.
As I wrote, and as you wrote too, this relies on your assumptions on
which errors the syscall might return, and the scheme how they are to be
handled.  That isn't clear until the reader follows most of the possible
branches.  So why not just point out what you intend to do right away?