Re: [PATCH 09/14] Add a new pthread_mutexattr_setelision_np interface.

["Carlos O'Donell" <carlos at redhat dot com>]

On 06/28/2013 04:53 PM, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Add a new pthread_mutexattr_setelision_np() export function
> that allows enabling or disabling elision for a mutex.

Let me state a couple of things up front.

(a) Roland objects to this patch.

Roland is away on vacation this next week, but I did take the
opportunity to pull him aside and discuss with him any opposition
he has to the various interfaces that we are proposing.

Roland objects to this patch principally because the API should
be about behaviour guarantees and performance characteristics
that the application can describe. The fact is that elision is an
implementation detail of the underlying lock type. The library
should be deciding amongst the implementation details given
information it knows about the lock.

What API would allow us to describe the lock in such a way that
glibc could make the decision to use or not use elision?

Comments?

(b) I will review this patch anyway.

I will review this patch anyway because I feel that the review
of the patch is instructive and because this patch could get
picked up by a downstream that desires to add these interfaces
and maintain the as an experimental interface.

> This now implies no behaviour changes that are not undefined or
> are "may" in POSIX to my knowledge. The previous trylock
> changes have been eliminated.
> 
> It is purely a tuning hint.

(1) Please make sure you say it's purely a tuning hint in the manual

(2) The interface needs some cleanup, see below.

> nptl/:
> 2013-06-27  Andi Kleen  <ak@linux.intel.com>
> 
> 	* Makefile: Add pthread_mutexattr_setelision_np.
> 	* Versions: Add pthread_mutexattr_setelision_np for 2.18.
> 	* pthread_mutexattr_setelision_np.c (pthread_mutexattr_setelision_np):
> 	  New file to set elision attributes for a mutex attr.

"New file."

> 	* sysdeps/unix/sysv/linux/x86/pthread_mutexattr_setelision_np.c: New file
>           as wrapper to define the ENABLE_ELISION macro.

"New file."

> 	* nptl/sysdeps/pthread/pthread.h (pthread_mutexattr_settype):
> 	  New prototype.
> 	  (__PTHREAD_SET_ELISION): New define for the test suite.
> 
> /:
> 2013-06-27  Andi Kleen  <ak@linux.intel.com>
> 
> 	* sysdeps/unix/sysv/linux/i386/nptl/libpthread.abilist: Add
>           pthread_mutexattr_setelision_np and related functions.
>         * sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/nptl/libpthread.abilist: dito.
>         * sysdeps/unix/sysv/linux/powerpc/powerpc64/nptl/libpthread.abilist: dito.
>         * sysdeps/unix/sysv/linux/s390/s390-32/nptl/libpthread.abilist: dito.
>         * sysdeps/unix/sysv/linux/s390/s390-64/nptl/libpthread.abilist: dito.
>         * sysdeps/unix/sysv/linux/sh/nptl/libpthread.abilist: dito.
>         * sysdeps/unix/sysv/linux/sparc/sparc32/nptl/libpthread.abilist: dito.
>         * sysdeps/unix/sysv/linux/sparc/sparc64/nptl/libpthread.abilist: dito.
>         * sysdeps/unix/sysv/linux/x86_64/64/nptl/libpthread.abilist: dito.
>         * sysdeps/unix/sysv/linux/x86_64/x32/nptl/libpthread.abilist: dito.
> ---
>  nptl/Makefile                                      |  3 +-
>  nptl/Versions                                      |  1 +
>  nptl/pthread_mutexattr_setelision_np.c             | 39 ++++++++++++++++++++++
>  nptl/sysdeps/pthread/pthread.h                     | 10 ++++++
>  .../linux/x86/pthread_mutexattr_setelision_np.c    | 20 +++++++++++
>  .../unix/sysv/linux/i386/nptl/libpthread.abilist   |  1 +
>  .../powerpc/powerpc32/fpu/nptl/libpthread.abilist  |  1 +
>  .../powerpc/powerpc64/nptl/libpthread.abilist      |  1 +
>  .../linux/s390/s390-32/nptl/libpthread.abilist     |  1 +
>  .../linux/s390/s390-64/nptl/libpthread.abilist     |  1 +
>  sysdeps/unix/sysv/linux/sh/nptl/libpthread.abilist |  1 +
>  .../linux/sparc/sparc32/nptl/libpthread.abilist    |  1 +
>  .../linux/sparc/sparc64/nptl/libpthread.abilist    |  1 +
>  .../sysv/linux/x86_64/64/nptl/libpthread.abilist   |  1 +
>  .../sysv/linux/x86_64/x32/nptl/libpthread.abilist  |  1 +
>  15 files changed, 82 insertions(+), 1 deletion(-)
>  create mode 100644 nptl/pthread_mutexattr_setelision_np.c
>  create mode 100644 nptl/sysdeps/unix/sysv/linux/x86/pthread_mutexattr_setelision_np.c
> 
> diff --git a/nptl/Makefile b/nptl/Makefile
> index cd601e5..883c9a8 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -126,7 +126,8 @@ libpthread-routines = nptl-init vars events version \
>  		      pthread_mutex_getprioceiling \
>  		      pthread_mutex_setprioceiling \
>  		      pthread_setname pthread_getname \
> -		      pthread_setattr_default_np pthread_getattr_default_np
> +		      pthread_setattr_default_np pthread_getattr_default_np \
> +		      pthread_mutexattr_setelision_np
>  #		      pthread_setuid pthread_seteuid pthread_setreuid \
>  #		      pthread_setresuid \
>  #		      pthread_setgid pthread_setegid pthread_setregid \
> diff --git a/nptl/Versions b/nptl/Versions
> index 7a3da25..1ca3943 100644
> --- a/nptl/Versions
> +++ b/nptl/Versions
> @@ -259,6 +259,7 @@ libpthread {
>      pthread_mutexattr_settype;
>      pthread_mutexattr_setkind_np;
>      __pthread_mutexattr_settype;
> +    pthread_mutexattr_setelision_np;

This doesn't look like the nptl/Version file I have on master,
so I figure you have some additional changes that should be
reverted.

Sorted alphabetically.

http://sourceware.org/glibc/wiki/Consensus:
~~~
Anyone can commit a change fixing the ordering of the Versions files. These files should be sorted for each version (alphabetically, first internal symbols used by other libraries, then public symbols). 
~~~

>    };
>  
>    GLIBC_PRIVATE {
> diff --git a/nptl/pthread_mutexattr_setelision_np.c b/nptl/pthread_mutexattr_setelision_np.c
> new file mode 100644
> index 0000000..de04190
> --- /dev/null
> +++ b/nptl/pthread_mutexattr_setelision_np.c
> @@ -0,0 +1,39 @@
> +/* Copyright (C) 2013 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <pthreadP.h>
> +
> +#ifndef ENABLE_ELISION
> +# define ENABLE_ELISION 0
> +#endif
> +
> +int
> +pthread_mutexattr_setelision_np (pthread_mutexattr_t *attr, int flag)
> +{

This is not sufficiently robust.

One should aim for a similar API to pthread_mutexattr_[sg]etrobust.

I would suggest the following:

(a) Define two new external flags for the user to use to set the
    elision state for the mutex.

    e.g. PTHREAD_MUTEX_ELISION_NP and PTHREAD_MUTEX_NO_ELISION_NP.

    You can just use an enum for them.
e.g.
/* Elision state flags.  */
enum
{
  PTHREAD_MUTEX_ELISION_NP,
  PTHREAD_MUTEX_NO_ELISION_NP,
};

Note that you potentially have 3 values:
- Value passed by user as PTHREAD_MUTEX_ELISION_NP
  - Should not be a bit value to help us preserve flag space for
    the future interface uses. That way we can add flags to the
    interface for future HTM needs.
- Bit in the pthread_mutexattr_t
  - May or may not be the same as the internal kind bits.
- Bit in the mutex kind.

(b) Reject any values that aren't these values.

    Return EINVAL for anything not in the above set.
    Otherwise it should jsut work.

> +  struct pthread_mutexattr *iattr = (struct pthread_mutexattr *) attr;
> +
> +  iattr->mutexkind &= ~PTHREAD_MUTEX_ELISION_FLAGS_NP;
> +  if (ENABLE_ELISION)
> +    {
> +      if (flag)

No implicit boolean coercion. Check for equality with the new types.

> +	iattr->mutexkind |= PTHREAD_MUTEX_ELISION_NP;
> +      else
> +	iattr->mutexkind |= PTHREAD_MUTEX_NO_ELISION_NP;
> +    }
> +  return 0;
> +}
> +
> diff --git a/nptl/sysdeps/pthread/pthread.h b/nptl/sysdeps/pthread/pthread.h
> index df4b725..c34d657 100644
> --- a/nptl/sysdeps/pthread/pthread.h
> +++ b/nptl/sysdeps/pthread/pthread.h
> @@ -853,6 +853,16 @@ extern int pthread_mutexattr_settype (pthread_mutexattr_t *__attr, int __kind)
>       __THROW __nonnull ((1));
>  #endif
>  
> +#if defined __USE_GNU
> +
> +/* Enable or disable lock elision for *ATTR. A FLAG value not zero enables
> +   elision, otherwise elision gets disabled.  */

Adjust comment for new types.

> +extern int pthread_mutexattr_setelision_np (pthread_mutexattr_t *__attr,
> +		                            int __flag);
> +
> +#define __PTHREAD_SET_ELISION 1

You don't need this? 

If you add the interfaces, then you add their support to the testsuite.

At which point you have no need to distinguish if the interface is there
or not.

We don't want to unnecessarily increase the size of a public header.

> +#endif
> +
>  /* Return in *PROTOCOL the mutex protocol attribute in *ATTR.  */
>  extern int pthread_mutexattr_getprotocol (const pthread_mutexattr_t *
>  					  __restrict __attr,
> diff --git a/nptl/sysdeps/unix/sysv/linux/x86/pthread_mutexattr_setelision_np.c b/nptl/sysdeps/unix/sysv/linux/x86/pthread_mutexattr_setelision_np.c
> new file mode 100644
> index 0000000..9c453fc
> --- /dev/null
> +++ b/nptl/sysdeps/unix/sysv/linux/x86/pthread_mutexattr_setelision_np.c
> @@ -0,0 +1,20 @@
> +/* Copyright (C) 2013 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <elision-conf.h>
> +#define ENABLE_ELISION (__elision_available != 0)
> +#include "nptl/pthread_mutexattr_setelision_np.c"

OK.

Cheers,
Carlos.