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

[Torvald Riegel <triegel at redhat dot com>]

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

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

>      /* How often we retry using elision if there is chance for the
> transaction
>         to finish execution (e.g., it wasn't aborted due to the lock
> being
>         already acquired.  */
> diff --git a/nptl/sysdeps/unix/sysv/linux/x86/elision-conf.h
> b/nptl/sysdeps/unix/sysv/linux/x86/elision-conf.h
> index 02cd2a6..1e519e9 100644
> --- a/nptl/sysdeps/unix/sysv/linux/x86/elision-conf.h
> +++ 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?