Re: [PATCH 10/14] Add a new pthread_rwlockattr_setelision_np function for rwlocks

["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 exported pthread_rwlockattr_setelision_np() function that allows
> enabling or disabling elision for a rwlock.
> 
> 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.
> 
> This is now purely a tuning hint.

Roland's same objection applies to this interface.

My exact same review also applies to this interface.

I'm not going to give the same review over again,
but anything that is different I'll make a comment.

One nit below which applies to the other interface also.

> nptl/:
> 2013-06-27  Andi Kleen  <ak@linux.intel.com>
> 
> 	* Makefile (pthread_rwlockattr_setelision_np): Add new file.
> 	* Versions (pthread_rwlockattr_setelision_np): Add new symbol.
> 	* pthread_rwlockattr_setelision_np.c (pthread_rwlockattr_setelision_np):
> 	  Add new file.
> 	* pthread_rwlock_init (pthread_rwlock_init): Transfer elision state.
> 	* sysdeps/pthread/pthread.h (pthread_rwlockattr_setkind_np):
> 	  Add new prototype.
> 	* sysdeps/unix/sysv/linux/x86/pthread_rwlockattr_setelision_np.c:
> 	  Add wrapper to provide ENABLE_ELISION.
> 
> /:
> 2013-06-27  Andi Kleen  <ak@linux.intel.com>
> 
> 	* sysdeps/unix/sysv/linux/i386/nptl/libpthread.abilist: Add
> 	  pthread_rwlockattr_setelision_np.
> 	* 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                                      |  1 +
>  nptl/Versions                                      |  1 +
>  nptl/pthread_rwlock_init.c                         |  4 +++
>  nptl/pthread_rwlockattr_setelision_np.c            | 39 ++++++++++++++++++++++
>  nptl/sysdeps/pthread/pthread.h                     | 10 ++++++
>  .../linux/x86/pthread_rwlockattr_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 +
>  16 files changed, 85 insertions(+)
>  create mode 100644 nptl/pthread_rwlockattr_setelision_np.c
>  create mode 100644 nptl/sysdeps/unix/sysv/linux/x86/pthread_rwlockattr_setelision_np.c
> 
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 883c9a8..d076b68 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -66,6 +66,7 @@ libpthread-routines = nptl-init vars events version \
>  		      pthread_rwlockattr_setpshared \
>  		      pthread_rwlockattr_getkind_np \
>  		      pthread_rwlockattr_setkind_np \
> +		      pthread_rwlockattr_setelision_np \
>  		      pthread_cond_init pthread_cond_destroy \
>  		      pthread_cond_wait pthread_cond_timedwait \
>  		      pthread_cond_signal pthread_cond_broadcast \
> diff --git a/nptl/Versions b/nptl/Versions
> index 1ca3943..7a161b6 100644
> --- a/nptl/Versions
> +++ b/nptl/Versions
> @@ -260,6 +260,7 @@ libpthread {
>      pthread_mutexattr_setkind_np;
>      __pthread_mutexattr_settype;
>      pthread_mutexattr_setelision_np;
> +    pthread_rwlockattr_setelision_np;
>    };
>  
>    GLIBC_PRIVATE {
> diff --git a/nptl/pthread_rwlock_init.c b/nptl/pthread_rwlock_init.c
> index 29bef71..ba5c3f3 100644
> --- a/nptl/pthread_rwlock_init.c
> +++ b/nptl/pthread_rwlock_init.c
> @@ -68,6 +68,10 @@ __pthread_rwlock_init (rwlock, attr)
>  					      header.private_futex));
>  #endif
>  
> +#ifdef __PTHREAD_RWLOCK_ELISION
> +  rwlock->__data.__rw_elision = iattr->rw_elision;
> +#endif
> +
>    return 0;
>  }
>  strong_alias (__pthread_rwlock_init, pthread_rwlock_init)
> diff --git a/nptl/pthread_rwlockattr_setelision_np.c b/nptl/pthread_rwlockattr_setelision_np.c
> new file mode 100644
> index 0000000..852f48b
> --- /dev/null
> +++ b/nptl/pthread_rwlockattr_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_rwlockattr_setelision_np (pthread_rwlockattr_t *attr, int flag)
> +{
> +  struct pthread_rwlockattr *iattr;
> +
> +  iattr = (struct pthread_rwlockattr *) attr;
> +
> +  if (ENABLE_ELISION)

We should do exactly the same thing with or without elision.

We should set the value and just ignore it.

This way you can set the value and get the value and the
two value match.

This is only a hint, and you shouldn't expect to use 
these interfaces as a method of determining if the hint 
was applied by the implementation.

I'm only just now dealing with the fallout of a similar
problem in an interface we created 9 years ago, where
users were using it to determine something else about
the underlying implementation. It's not pretty.

> +    {
> +      if (flag)
> +        iattr->rw_elision = 1;
> +      else
> +        iattr->rw_elision = -1;
> +    }
> +  return 0;
> +}
> diff --git a/nptl/sysdeps/pthread/pthread.h b/nptl/sysdeps/pthread/pthread.h
> index c34d657..fa21502 100644
> --- a/nptl/sysdeps/pthread/pthread.h
> +++ b/nptl/sysdeps/pthread/pthread.h
> @@ -989,6 +989,16 @@ extern int pthread_rwlockattr_setkind_np (pthread_rwlockattr_t *__attr,
>  					  int __pref) __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.  */
> +extern int pthread_rwlockattr_setelision_np (pthread_rwlockattr_t *__attr,
> +		                             int __flag);
> +
> +#define __PTHREAD_RWLOCK_SET_ELISION 1
> +#endif
> +
>  
>  /* Functions for handling conditional variables.  */
>  
> diff --git a/nptl/sysdeps/unix/sysv/linux/x86/pthread_rwlockattr_setelision_np.c b/nptl/sysdeps/unix/sysv/linux/x86/pthread_rwlockattr_setelision_np.c
> new file mode 100644
> index 0000000..ee71b43
> --- /dev/null
> +++ b/nptl/sysdeps/unix/sysv/linux/x86/pthread_rwlockattr_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_rwlockattr_setelision_np.c"

Cheers,
Carlos.