Re: [PATCH 2/6] Fall back to locking if elision runs out of retries.

[Dominik Vogt <vogt at linux dot vnet dot ibm dot com>]

On Wed, Sep 11, 2013 at 05:42:47PM +0200, Torvald Riegel wrote:
> On Mon, 2013-09-02 at 09:59 +0200, Dominik Vogt wrote:
> > With the old code, elision would never be disabled if every
> > transaction aborts
> > with a temporary failure.  This patch introduces a new configuration
> > value that
> > is used whenever pthread_mutex_lock runs out of retries.
> 
> I agree that this case can exist, but it's not quite as bad as you make
> it sound like.  In particular, if the temporary / retryable aborts are
> due to conflict with other threads acquiring the same lock, then in
> practice we will disable, but via skip_lock_busy.

I do not think this is the case in general.  The code path that
handles _ABORT_LOCK_BUSY is only executed in case of a race when
one thread looks at *adapt_count and decides to try elision, but
before it can start a transaction another thread using elision on
the same lock is aborted and switches to using a real lock.  This
needs to happen between

  if *adapt_count <= 0

and

  if (*futex == 0)

Otherwise the transaction will simply abort because of a
read/write conflict on *futex, but it won't cause an
_ABORT_LOCK_BUSY.

> However, if we have
> retryable aborts due to other threads' actions that don't acquire the
> same lock, then you are right that we'll never disable.

> Nonetheless, I'm not comfortable with this patch.  First, see the issues
> below.

> Second, I think we need to take a step back and come up with a
> full design and discussion of the adaptation first; right now we're
> adding one hack after another, and this can't be good.

Yes, that _is_ important.  But please also keep in mind that my
task at hand is to port lock elision to z, and thus my current
focus on the discussion is to decide about the portions of code
where z works differently than Tsx (e.g. handling of explicit
abors).

> Dominik, it
> would be great if you could work on this, and I'd like to see Andi to
> help with that as much as possible.

Sure.

> > diff --git a/nptl/sysdeps/unix/sysv/linux/x86/elision-conf.c
> > b/nptl/sysdeps/unix/sysv/linux/x86/elision-conf.c
> > index 2fed32b..c6bd49f 100644
> > --- a/nptl/sysdeps/unix/sysv/linux/x86/elision-conf.c
> > +++ b/nptl/sysdeps/unix/sysv/linux/x86/elision-conf.c
> > @@ -35,6 +35,9 @@ struct elision_config __elision_aconf =
> >         to reasons other than other threads' memory accesses.
> > Expressed in
> >         number of lock acquisition attempts.  */
> >      .skip_lock_internal_abort = 3,
> > +    /* How often to not attempt to use elision if a lock used up all
> > retries
> > +       without success.  Expressed in number of lock acquisition
> > attempts.  */
> > +    .skip_lock_out_of_retries = 3,
> 
> IMHO, this needs another name.  What kind of retries?

It's not clear to me what you have in mind.  If you're fine with
the name ".skip_lock_internal_abort"; what additional information
would you like to see in that name?  That it's about transaction
retries, or what else?

[snip]
> > +++ b/nptl/sysdeps/unix/sysv/linux/x86/elision-conf.h
> > @@ -28,6 +28,7 @@ struct elision_config
> >  {
> >    int skip_lock_busy;
> >    int skip_lock_internal_abort;
> > +  int skip_lock_out_of_retries;
> >    int retry_try_xbegin;
> >    int skip_trylock_internal_abort;
> >  };
> > diff --git a/nptl/sysdeps/unix/sysv/linux/x86/elision-lock.c
> > b/nptl/sysdeps/unix/sysv/linux/x86/elision-lock.c
> > index 45370ed..04f7c28 100644
> > --- a/nptl/sysdeps/unix/sysv/linux/x86/elision-lock.c
> > +++ b/nptl/sysdeps/unix/sysv/linux/x86/elision-lock.c
> > @@ -82,6 +82,10 @@ __lll_lock_elision (int *futex, short *adapt_count,
> > EXTRAARG int private)
> >               break;
> >             }
> >         }
> > +      /* Same logic as above, but for for a number of tepmorary
> > failures in a
> > +        row.  */
> > +      if (aconf.skip_lock_out_of_retries > 0 &&
> > aconf.retry_try_xbegin > 0)
> > +       *adapt_count = aconf.skip_lock_out_of_retries;
> >      }
> >    else
> >      { 
> 
> Won't the break a few lines above that change fall through to this
> change, so that this doesn't just set the skip count for retryable
> aborts?

Yup, I'll fix that in the next set of patches.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany