[1/3] Tunables: Add tunables of spin count for adaptive spin mutex

This patch does not have any functionality change, we only provide a spin
count tunes for pthread adaptive spin mutex. The tunable
glibc.mutex.spin_count tunes can be used by system adminstrator to squeeze
system performance according to different hardware capability and workload
model.

This is the preparation work for the next patch, in which the way of
adaptive spin would be changed from an expensive cmpxchg to read while
spinning.

   * elf/dl-tunables.list: Add glibc.mutex.spin_count entry.
   * manual/tunables.texi: Add glibc.mutex.spin_count description.
   * nptl/Makefile: Add mutex-conf.c for compilation.
   * nptl/mutex-conf.h: New file.
   * nptl/mutex-conf.c: New file.

Suggested-by: Andi Kleen <andi.kleen@intel.com>
Signed-off-by: Kemi Wang <kemi.wang@intel.com>
---
 ChangeLog            | 10 ++++++-
 elf/dl-tunables.list | 10 +++++++
 manual/tunables.texi | 17 ++++++++++++
 nptl/Makefile        |  3 +-
 nptl/mutex-conf.c    | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 nptl/mutex-conf.h    | 31 +++++++++++++++++++++
 6 files changed, 147 insertions(+), 2 deletions(-)
 create mode 100644 nptl/mutex-conf.c
 create mode 100644 nptl/mutex-conf.h

On 30/03/2018 04:14, Kemi Wang wrote:
> This patch does not have any functionality change, we only provide a spin
> count tunes for pthread adaptive spin mutex. The tunable
> glibc.mutex.spin_count tunes can be used by system adminstrator to squeeze
> system performance according to different hardware capability and workload
> model.
> 
> This is the preparation work for the next patch, in which the way of
> adaptive spin would be changed from an expensive cmpxchg to read while
> spinning.
> 
>    * elf/dl-tunables.list: Add glibc.mutex.spin_count entry.
>    * manual/tunables.texi: Add glibc.mutex.spin_count description.
>    * nptl/Makefile: Add mutex-conf.c for compilation.
>    * nptl/mutex-conf.h: New file.
>    * nptl/mutex-conf.c: New file.
> 
> Suggested-by: Andi Kleen <andi.kleen@intel.com>
> Signed-off-by: Kemi Wang <kemi.wang@intel.com>

> ---
>  ChangeLog            | 10 ++++++-
>  elf/dl-tunables.list | 10 +++++++
>  manual/tunables.texi | 17 ++++++++++++
>  nptl/Makefile        |  3 +-
>  nptl/mutex-conf.c    | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  nptl/mutex-conf.h    | 31 +++++++++++++++++++++
>  6 files changed, 147 insertions(+), 2 deletions(-)
>  create mode 100644 nptl/mutex-conf.c
>  create mode 100644 nptl/mutex-conf.h
> 
> diff --git a/ChangeLog b/ChangeLog
> index 1f98425..472657c 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,13 @@
> -2018-03-29  Florian Weimer  <fweimer@redhat.com>
> +2018-03-30 Kemi Wang <kemi.wang@intel.com>
> +
> +	* elf/dl-tunables.list: Add glibc.mutex.spin_count entry.
> +	* manual/tunables.texi: Add glibc.mutex.spin_count description.
> +	* nptl/Makefile: Add mutex-conf.c for compilation.
> +	* nptl/mutex-conf.h: New file.
> +	* nptl/mutex-conf.c: New file.
> +	* nptl/Makefile: Add new file compilation.
>  
> +2018-03-29  Florian Weimer  <fweimer@redhat.com>
>  	* sysdeps/unix/sysv/linux/i386/tst-bz21269.c (do_test): Also
>  	capture SIGBUS.
>  
> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
> index 1f8ecb8..0c27c14 100644
> --- a/elf/dl-tunables.list
> +++ b/elf/dl-tunables.list
> @@ -121,4 +121,14 @@ glibc {
>        default: 3
>      }
>    }
> +
> +  mutex {
> +	  spin_count {
> +		  type: INT_32
> +		  minval: 0
> +		  maxval: 30000
> +		  env_alias: LD_SPIN_COUNT
> +		  default: 1000
> +	  }
> +  }

Indentation seems off here, rest the file uses double space while your
patch uses tabs.

Also I not sure if it worth to add environment variable for this tunable,
I would rather avoid adding newer ones (and naming seems off, since afaik
LD_* meaning some parameters that affects the loader).

>  }
> diff --git a/manual/tunables.texi b/manual/tunables.texi
> index be33c9f..9c6a9f1 100644
> --- a/manual/tunables.texi
> +++ b/manual/tunables.texi
> @@ -281,6 +281,23 @@ of try lock attempts.
>  The default value of this tunable is @samp{3}.
>  @end deftp
>  
> +@node Pthread Mutex Tunables
> +@section Pthread Mutex Tunables
> +@cindex pthread mutex tunables
> +
> +@deftp {Tunable namespace} glibc.mutex
> +Behavior of ptherad mutex can be tuned to acquire performance improvement
> +according to specific hardware capablity and workload character by setting
> +the following tunables in the @code{mutex} namespace.
> +@end deftp

There is a typo (s/ptherad/pthread) and I think 'acquire' is not the best
word here, I would use 'increase' (I am not a native speaker, so someone
might suggest a better wording here).

> +
> +@deftp Tunable glibc.mutex.spin_count
> +The @code{glibc.mutex.spin_count} tunable set the maximum times the thread
> +should spin on the lock before going to sleep.
> +
> +The default value of this tunable is @samp{1000}.
> +@end deftp

I think we need to expand it to specify:

  * spinning is only used for the mutex initialized with PTHREAD_MUTEX_ADAPTIVE_NP
    GNU extension(current wording is ambiguous).

  * It affects both pthread_mutex_lock and pthread_mutex_timedlock.

  * The spinning is done by first issuing an atomic operation similar to trylock
    followed by a arch-specific no operation (not sure if we need to expand
    how spinning is backoff value is incremented).

  * Not sure 'sleep' is right terminology here, since for Linux 'futex' can
    return right away when maximum spin_count it reached.

> +
>  @node Hardware Capability Tunables
>  @section Hardware Capability Tunables
>  @cindex hardware capability tunables
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 94be92c..5edacea 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -139,7 +139,8 @@ libpthread-routines = nptl-init vars events version pt-interp \
>  		      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 \
> +		      mutex-conf
>  #		      pthread_setuid pthread_seteuid pthread_setreuid \
>  #		      pthread_setresuid \
>  #		      pthread_setgid pthread_setegid pthread_setregid \

I think a better name for the file would be pthread_mutex_conf.c.

> diff --git a/nptl/mutex-conf.c b/nptl/mutex-conf.c
> new file mode 100644
> index 0000000..f4ffd6d
> --- /dev/null
> +++ b/nptl/mutex-conf.c
> @@ -0,0 +1,78 @@
> +/* mutex-conf.c: Pthread mutex tunable parameters.
> +   Copyright (C) 2013-2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +

Copyright should start at 2018 for newer implementations.

> +   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 "config.h"
> +#include <pthreadP.h>
> +#include <init-arch.h>
> +#include <mutex-conf.h>
> +#include <unistd.h>
> +
> +#if HAVE_TUNABLES
> +# define TUNABLE_NAMESPACE mutex
> +#endif
> +#include <elf/dl-tunables.h>
> +
> +
> +struct mutex_config __mutex_aconf =
> +  {
> +	  /* The maximum times a thread spin on the lock before
> +	   * going to sleep */
> +	  .spin_count = 1000,
> +  };
> +

Indentation seems off here and in other places in this file.

> +#if HAVE_TUNABLES
> +#define TUNABLE_CALLBACK_FNDECL(__name, __type)			\

Indentation for nested preprocessor is to just one space each level:

#if HAVE_TUNABLES
# define ... 

> +static inline void						\
> +__always_inline							\
> +do_set_mutex_ ## __name (__type value)			\
> +{								\
> +  __mutex_aconf.__name = value;				\
> +}								\
> +void								\
> +TUNABLE_CALLBACK (set_mutex_ ## __name) (tunable_val_t *valp) \
> +{								\
> +  __type value = (__type) (valp)->numval;			\
> +  do_set_mutex_ ## __name (value);				\
> +}
> +
> +TUNABLE_CALLBACK_FNDECL (spin_count, int32_t);
> +#endif
> +
> +static void
> +mutex_tunables_init (int argc __attribute__ ((unused)),
> +			      char **argv  __attribute__ ((unused)),
> +					      char **environ)
> +{
> +#if HAVE_TUNABLES
> +
> +	TUNABLE_GET (spin_count, int32_t,
> +			TUNABLE_CALLBACK (set_mutex_spin_count));
> +#endif
> +}
> +
> +#ifdef SHARED
> +# define INIT_SECTION ".init_array"
> +#else
> +# define INIT_SECTION ".preinit_array"
> +#endif
> +
> +void (*const __pthread_mutex_tunables_init_array []) (int, char **, char **)
> +  __attribute__ ((section (INIT_SECTION), aligned (sizeof (void *)))) =
> +{
> +  &mutex_tunables_init
> +};> diff --git a/nptl/mutex-conf.h b/nptl/mutex-conf.h
> new file mode 100644
> index 0000000..babefe3
> --- /dev/null
> +++ b/nptl/mutex-conf.h
> @@ -0,0 +1,31 @@
> +/* mutex-conf.h: Pthread mutex tunable parameters.
> +   Copyright (C) 2013-2018 Free Software Foundation, Inc.

Copyright should start at 2018 for newer implementations.

> +   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/>.  */
> +#ifndef _MUTEX_CONF_H
> +#define _MUTEX_CONF_H 1
> +
> +#include <pthread.h>
> +#include <time.h>
> +
> +struct mutex_config
> +{
> +  int spin_count;
> +};
> +
> +extern struct mutex_config __mutex_aconf attribute_hidden;
> +
> +#endif
>

Hi, Adhemerval
   Thanks for your review. Could you please help to review the other two patches in this series
if available? Also, please keep guys in the cc list in case someone may not subscribe glibc 
mail list. Thanks:)

See my reply below.

On 2018年03月30日 15:14, Kemi Wang wrote:
> On 30/03/2018 04:14, Kemi Wang wrote:
>> This patch does not have any functionality change, we only provide a spin
>> count tunes for pthread adaptive spin mutex. The tunable
>> glibc.mutex.spin_count tunes can be used by system adminstrator to squeeze
>> system performance according to different hardware capability and workload
>> model.
>>
>> This is the preparation work for the next patch, in which the way of
>> adaptive spin would be changed from an expensive cmpxchg to read while
>> spinning.
>>
>>    * elf/dl-tunables.list: Add glibc.mutex.spin_count entry.
>>    * manual/tunables.texi: Add glibc.mutex.spin_count description.
>>    * nptl/Makefile: Add mutex-conf.c for compilation.
>>    * nptl/mutex-conf.h: New file.
>>    * nptl/mutex-conf.c: New file.
>>
>> Suggested-by: Andi Kleen <andi.kleen@intel.com>
>> Signed-off-by: Kemi Wang <kemi.wang@intel.com>
> 
>> ---
>>  ChangeLog            | 10 ++++++-
>>  elf/dl-tunables.list | 10 +++++++
>>  manual/tunables.texi | 17 ++++++++++++
>>  nptl/Makefile        |  3 +-
>>  nptl/mutex-conf.c    | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  nptl/mutex-conf.h    | 31 +++++++++++++++++++++
>>  6 files changed, 147 insertions(+), 2 deletions(-)
>>  create mode 100644 nptl/mutex-conf.c
>>  create mode 100644 nptl/mutex-conf.h
>>
>> diff --git a/ChangeLog b/ChangeLog
>> index 1f98425..472657c 100644
>> --- a/ChangeLog
>> +++ b/ChangeLog
>> @@ -1,5 +1,13 @@
>> -2018-03-29  Florian Weimer  <fweimer@redhat.com>
>> +2018-03-30 Kemi Wang <kemi.wang@intel.com>
>> +
>> +	* elf/dl-tunables.list: Add glibc.mutex.spin_count entry.
>> +	* manual/tunables.texi: Add glibc.mutex.spin_count description.
>> +	* nptl/Makefile: Add mutex-conf.c for compilation.
>> +	* nptl/mutex-conf.h: New file.
>> +	* nptl/mutex-conf.c: New file.
>> +	* nptl/Makefile: Add new file compilation.
>>  
>> +2018-03-29  Florian Weimer  <fweimer@redhat.com>
>>  	* sysdeps/unix/sysv/linux/i386/tst-bz21269.c (do_test): Also
>>  	capture SIGBUS.
>>  
>> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
>> index 1f8ecb8..0c27c14 100644
>> --- a/elf/dl-tunables.list
>> +++ b/elf/dl-tunables.list
>> @@ -121,4 +121,14 @@ glibc {
>>        default: 3
>>      }
>>    }
>> +
>> +  mutex {
>> +	  spin_count {
>> +		  type: INT_32
>> +		  minval: 0
>> +		  maxval: 30000
>> +		  env_alias: LD_SPIN_COUNT
>> +		  default: 1000
>> +	  }
>> +  }
> 
> Indentation seems off here, rest the file uses double space while your
> patch uses tabs.
> 

Thanks for catching it.

> Also I not sure if it worth to add environment variable for this tunable,
> I would rather avoid adding newer ones (and naming seems off, since afaik
> LD_* meaning some parameters that affects the loader).
> 

AFAIK, environment variable is what I can think of to give people the possibility of 
tunes in shell for that, maybe you have better idea?

Yes, we probably use other name like MUTEX_SPIN_COUNT to avoid confusion, agree?

>>  }
>> diff --git a/manual/tunables.texi b/manual/tunables.texi
>> index be33c9f..9c6a9f1 100644
>> --- a/manual/tunables.texi
>> +++ b/manual/tunables.texi
>> @@ -281,6 +281,23 @@ of try lock attempts.
>>  The default value of this tunable is @samp{3}.
>>  @end deftp
>>  
>> +@node Pthread Mutex Tunables
>> +@section Pthread Mutex Tunables
>> +@cindex pthread mutex tunables
>> +
>> +@deftp {Tunable namespace} glibc.mutex
>> +Behavior of ptherad mutex can be tuned to acquire performance improvement
>> +according to specific hardware capablity and workload character by setting
>> +the following tunables in the @code{mutex} namespace.
>> +@end deftp
> 
> There is a typo (s/ptherad/pthread) 

thanks for catching it.

> and I think 'acquire' is not the best
> word here, I would use 'increase' (I am not a native speaker, so someone
> might suggest a better wording here).
> 

All right, I will ask for some native speakers to help check it.

>> +
>> +@deftp Tunable glibc.mutex.spin_count
>> +The @code{glibc.mutex.spin_count} tunable set the maximum times the thread
>> +should spin on the lock before going to sleep.
>> +
>> +The default value of this tunable is @samp{1000}.
>> +@end deftp
> 
> I think we need to expand it to specify:
> 
>   * spinning is only used for the mutex initialized with PTHREAD_MUTEX_ADAPTIVE_NP
>     GNU extension(current wording is ambiguous).
> >   * It affects both pthread_mutex_lock and pthread_mutex_timedlock.
> 
>   * The spinning is done by first issuing an atomic operation similar to trylock
>     followed by a arch-specific no operation (not sure if we need to expand
>     how spinning is backoff value is incremented).
> 	
      How about this?
      The spinning is done in case of either the maximum spin count 
      is reached or lock is acquired during spinning.

>   * Not sure 'sleep' is right terminology here, since for Linux 'futex' can
>     return right away when maximum spin_count it reached.
>
    'block' here may be more precise, agree?
    e.g. use "before calling into the kernel to block" or something like that

>> +
>>  @node Hardware Capability Tunables
>>  @section Hardware Capability Tunables
>>  @cindex hardware capability tunables
>> diff --git a/nptl/Makefile b/nptl/Makefile
>> index 94be92c..5edacea 100644
>> --- a/nptl/Makefile
>> +++ b/nptl/Makefile
>> @@ -139,7 +139,8 @@ libpthread-routines = nptl-init vars events version pt-interp \
>>  		      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 \
>> +		      mutex-conf
>>  #		      pthread_setuid pthread_seteuid pthread_setreuid \
>>  #		      pthread_setresuid \
>>  #		      pthread_setgid pthread_setegid pthread_setregid \
> 
> I think a better name for the file would be pthread_mutex_conf.c.
> 

Agree, will send V2 to fix it.

>> diff --git a/nptl/mutex-conf.c b/nptl/mutex-conf.c
>> new file mode 100644
>> index 0000000..f4ffd6d
>> --- /dev/null
>> +++ b/nptl/mutex-conf.c
>> @@ -0,0 +1,78 @@
>> +/* mutex-conf.c: Pthread mutex tunable parameters.
>> +   Copyright (C) 2013-2018 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
> 
> Copyright should start at 2018 for newer implementations.
> 

I am not aware of that before, do you mean "Copyright (C) 2018-2023"？

>> +   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 "config.h"
>> +#include <pthreadP.h>
>> +#include <init-arch.h>
>> +#include <mutex-conf.h>
>> +#include <unistd.h>
>> +
>> +#if HAVE_TUNABLES
>> +# define TUNABLE_NAMESPACE mutex
>> +#endif
>> +#include <elf/dl-tunables.h>
>> +
>> +
>> +struct mutex_config __mutex_aconf =
>> +  {
>> +	  /* The maximum times a thread spin on the lock before
>> +	   * going to sleep */
>> +	  .spin_count = 1000,
>> +  };
>> +
> 
> Indentation seems off here and in other places in this file.
> 

Will fix it in V2, thanks for pointing it out.

>> +#if HAVE_TUNABLES
>> +#define TUNABLE_CALLBACK_FNDECL(__name, __type)			\
> 
> Indentation for nested preprocessor is to just one space each level:
> 
> #if HAVE_TUNABLES
> # define ... 
> 
>> +static inline void						\
>> +__always_inline							\
>> +do_set_mutex_ ## __name (__type value)			\
>> +{								\
>> +  __mutex_aconf.__name = value;				\
>> +}								\
>> +void								\
>> +TUNABLE_CALLBACK (set_mutex_ ## __name) (tunable_val_t *valp) \
>> +{								\
>> +  __type value = (__type) (valp)->numval;			\
>> +  do_set_mutex_ ## __name (value);				\
>> +}
>> +
>> +TUNABLE_CALLBACK_FNDECL (spin_count, int32_t);
>> +#endif
>> +
>> +static void
>> +mutex_tunables_init (int argc __attribute__ ((unused)),
>> +			      char **argv  __attribute__ ((unused)),
>> +					      char **environ)
>> +{
>> +#if HAVE_TUNABLES
>> +
>> +	TUNABLE_GET (spin_count, int32_t,
>> +			TUNABLE_CALLBACK (set_mutex_spin_count));
>> +#endif
>> +}
>> +
>> +#ifdef SHARED
>> +# define INIT_SECTION ".init_array"
>> +#else
>> +# define INIT_SECTION ".preinit_array"
>> +#endif
>> +
>> +void (*const __pthread_mutex_tunables_init_array []) (int, char **, char **)
>> +  __attribute__ ((section (INIT_SECTION), aligned (sizeof (void *)))) =
>> +{
>> +  &mutex_tunables_init
>> +};> diff --git a/nptl/mutex-conf.h b/nptl/mutex-conf.h
>> new file mode 100644
>> index 0000000..babefe3
>> --- /dev/null
>> +++ b/nptl/mutex-conf.h
>> @@ -0,0 +1,31 @@
>> +/* mutex-conf.h: Pthread mutex tunable parameters.
>> +   Copyright (C) 2013-2018 Free Software Foundation, Inc.
> 
> Copyright should start at 2018 for newer implementations.
> 

Sure.

>> +   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/>.  */
>> +#ifndef _MUTEX_CONF_H
>> +#define _MUTEX_CONF_H 1
>> +
>> +#include <pthread.h>
>> +#include <time.h>
>> +
>> +struct mutex_config
>> +{
>> +  int spin_count;
>> +};
>> +
>> +extern struct mutex_config __mutex_aconf attribute_hidden;
>> +
>> +#endif
>>
>

On 04/04/2018 07:25, kemi wrote:
> Hi, Adhemerval
>    Thanks for your review. Could you please help to review the other two patches in this series
> if available? Also, please keep guys in the cc list in case someone may not subscribe glibc 
> mail list. Thanks:)

Yeah, I am checking the patch along with the referenced benchmark on a aarch64
machine as well.  

> 
> See my reply below.
> 
> On 2018年03月30日 15:14, Kemi Wang wrote:
>> On 30/03/2018 04:14, Kemi Wang wrote:
>>> This patch does not have any functionality change, we only provide a spin
>>> count tunes for pthread adaptive spin mutex. The tunable
>>> glibc.mutex.spin_count tunes can be used by system adminstrator to squeeze
>>> system performance according to different hardware capability and workload
>>> model.
>>>
>>> This is the preparation work for the next patch, in which the way of
>>> adaptive spin would be changed from an expensive cmpxchg to read while
>>> spinning.
>>>
>>>    * elf/dl-tunables.list: Add glibc.mutex.spin_count entry.
>>>    * manual/tunables.texi: Add glibc.mutex.spin_count description.
>>>    * nptl/Makefile: Add mutex-conf.c for compilation.
>>>    * nptl/mutex-conf.h: New file.
>>>    * nptl/mutex-conf.c: New file.
>>>
>>> Suggested-by: Andi Kleen <andi.kleen@intel.com>
>>> Signed-off-by: Kemi Wang <kemi.wang@intel.com>
>>
>>> ---
>>>  ChangeLog            | 10 ++++++-
>>>  elf/dl-tunables.list | 10 +++++++
>>>  manual/tunables.texi | 17 ++++++++++++
>>>  nptl/Makefile        |  3 +-
>>>  nptl/mutex-conf.c    | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  nptl/mutex-conf.h    | 31 +++++++++++++++++++++
>>>  6 files changed, 147 insertions(+), 2 deletions(-)
>>>  create mode 100644 nptl/mutex-conf.c
>>>  create mode 100644 nptl/mutex-conf.h
>>>
>>> diff --git a/ChangeLog b/ChangeLog
>>> index 1f98425..472657c 100644
>>> --- a/ChangeLog
>>> +++ b/ChangeLog
>>> @@ -1,5 +1,13 @@
>>> -2018-03-29  Florian Weimer  <fweimer@redhat.com>
>>> +2018-03-30 Kemi Wang <kemi.wang@intel.com>
>>> +
>>> +	* elf/dl-tunables.list: Add glibc.mutex.spin_count entry.
>>> +	* manual/tunables.texi: Add glibc.mutex.spin_count description.
>>> +	* nptl/Makefile: Add mutex-conf.c for compilation.
>>> +	* nptl/mutex-conf.h: New file.
>>> +	* nptl/mutex-conf.c: New file.
>>> +	* nptl/Makefile: Add new file compilation.
>>>  
>>> +2018-03-29  Florian Weimer  <fweimer@redhat.com>
>>>  	* sysdeps/unix/sysv/linux/i386/tst-bz21269.c (do_test): Also
>>>  	capture SIGBUS.
>>>  
>>> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
>>> index 1f8ecb8..0c27c14 100644
>>> --- a/elf/dl-tunables.list
>>> +++ b/elf/dl-tunables.list
>>> @@ -121,4 +121,14 @@ glibc {
>>>        default: 3
>>>      }
>>>    }
>>> +
>>> +  mutex {
>>> +	  spin_count {
>>> +		  type: INT_32
>>> +		  minval: 0
>>> +		  maxval: 30000
>>> +		  env_alias: LD_SPIN_COUNT
>>> +		  default: 1000
>>> +	  }
>>> +  }
>>
>> Indentation seems off here, rest the file uses double space while your
>> patch uses tabs.
>>
> 
> Thanks for catching it.
> 
>> Also I not sure if it worth to add environment variable for this tunable,
>> I would rather avoid adding newer ones (and naming seems off, since afaik
>> LD_* meaning some parameters that affects the loader).
>>
> 
> AFAIK, environment variable is what I can think of to give people the possibility of 
> tunes in shell for that, maybe you have better idea?
> 
> Yes, we probably use other name like MUTEX_SPIN_COUNT to avoid confusion, agree?

The tunables framework already provides a environment variable to this [1],
the 'env_alias' is mainly to provide compatibility and to use the same logic
internally.

So I think it is better to use the default tunable env var.

[1] https://www.gnu.org/software/libc/manual/html_node/Tunables.html

> 
>>>  }
>>> diff --git a/manual/tunables.texi b/manual/tunables.texi
>>> index be33c9f..9c6a9f1 100644
>>> --- a/manual/tunables.texi
>>> +++ b/manual/tunables.texi
>>> @@ -281,6 +281,23 @@ of try lock attempts.
>>>  The default value of this tunable is @samp{3}.
>>>  @end deftp
>>>  
>>> +@node Pthread Mutex Tunables
>>> +@section Pthread Mutex Tunables
>>> +@cindex pthread mutex tunables
>>> +
>>> +@deftp {Tunable namespace} glibc.mutex
>>> +Behavior of ptherad mutex can be tuned to acquire performance improvement
>>> +according to specific hardware capablity and workload character by setting
>>> +the following tunables in the @code{mutex} namespace.
>>> +@end deftp
>>
>> There is a typo (s/ptherad/pthread) 
> 
> thanks for catching it.
> 
>> and I think 'acquire' is not the best
>> word here, I would use 'increase' (I am not a native speaker, so someone
>> might suggest a better wording here).
>>
> 
> All right, I will ask for some native speakers to help check it.
> 
>>> +
>>> +@deftp Tunable glibc.mutex.spin_count
>>> +The @code{glibc.mutex.spin_count} tunable set the maximum times the thread
>>> +should spin on the lock before going to sleep.
>>> +
>>> +The default value of this tunable is @samp{1000}.
>>> +@end deftp
>>
>> I think we need to expand it to specify:
>>
>>   * spinning is only used for the mutex initialized with PTHREAD_MUTEX_ADAPTIVE_NP
>>     GNU extension(current wording is ambiguous).
>>>   * It affects both pthread_mutex_lock and pthread_mutex_timedlock.
>>
>>   * The spinning is done by first issuing an atomic operation similar to trylock
>>     followed by a arch-specific no operation (not sure if we need to expand
>>     how spinning is backoff value is incremented).
>> 	
>       How about this?
>       The spinning is done in case of either the maximum spin count 
>       is reached or lock is acquired during spinning.

It is better for this specific part.

> 
>>   * Not sure 'sleep' is right terminology here, since for Linux 'futex' can
>>     return right away when maximum spin_count it reached.
>>
>     'block' here may be more precise, agree?
>     e.g. use "before calling into the kernel to block" or something like that
> 
>>> +
>>>  @node Hardware Capability Tunables
>>>  @section Hardware Capability Tunables
>>>  @cindex hardware capability tunables
>>> diff --git a/nptl/Makefile b/nptl/Makefile
>>> index 94be92c..5edacea 100644
>>> --- a/nptl/Makefile
>>> +++ b/nptl/Makefile
>>> @@ -139,7 +139,8 @@ libpthread-routines = nptl-init vars events version pt-interp \
>>>  		      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 \
>>> +		      mutex-conf
>>>  #		      pthread_setuid pthread_seteuid pthread_setreuid \
>>>  #		      pthread_setresuid \
>>>  #		      pthread_setgid pthread_setegid pthread_setregid \
>>
>> I think a better name for the file would be pthread_mutex_conf.c.
>>
> 
> Agree, will send V2 to fix it.
> 
>>> diff --git a/nptl/mutex-conf.c b/nptl/mutex-conf.c
>>> new file mode 100644
>>> index 0000000..f4ffd6d
>>> --- /dev/null
>>> +++ b/nptl/mutex-conf.c
>>> @@ -0,0 +1,78 @@
>>> +/* mutex-conf.c: Pthread mutex tunable parameters.
>>> +   Copyright (C) 2013-2018 Free Software Foundation, Inc.
>>> +   This file is part of the GNU C Library.
>>> +
>>
>> Copyright should start at 2018 for newer implementations.
>>
> 
> I am not aware of that before, do you mean "Copyright (C) 2018-2023"？

No, just "Copyright (C) 2018".

> 
>>> +   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 "config.h"
>>> +#include <pthreadP.h>
>>> +#include <init-arch.h>
>>> +#include <mutex-conf.h>
>>> +#include <unistd.h>
>>> +
>>> +#if HAVE_TUNABLES
>>> +# define TUNABLE_NAMESPACE mutex
>>> +#endif
>>> +#include <elf/dl-tunables.h>
>>> +
>>> +
>>> +struct mutex_config __mutex_aconf =
>>> +  {
>>> +	  /* The maximum times a thread spin on the lock before
>>> +	   * going to sleep */
>>> +	  .spin_count = 1000,
>>> +  };
>>> +
>>
>> Indentation seems off here and in other places in this file.
>>
> 
> Will fix it in V2, thanks for pointing it out.
> 
>>> +#if HAVE_TUNABLES
>>> +#define TUNABLE_CALLBACK_FNDECL(__name, __type)			\
>>
>> Indentation for nested preprocessor is to just one space each level:
>>
>> #if HAVE_TUNABLES
>> # define ... 
>>
>>> +static inline void						\
>>> +__always_inline							\
>>> +do_set_mutex_ ## __name (__type value)			\
>>> +{								\
>>> +  __mutex_aconf.__name = value;				\
>>> +}								\
>>> +void								\
>>> +TUNABLE_CALLBACK (set_mutex_ ## __name) (tunable_val_t *valp) \
>>> +{								\
>>> +  __type value = (__type) (valp)->numval;			\
>>> +  do_set_mutex_ ## __name (value);				\
>>> +}
>>> +
>>> +TUNABLE_CALLBACK_FNDECL (spin_count, int32_t);
>>> +#endif
>>> +
>>> +static void
>>> +mutex_tunables_init (int argc __attribute__ ((unused)),
>>> +			      char **argv  __attribute__ ((unused)),
>>> +					      char **environ)
>>> +{
>>> +#if HAVE_TUNABLES
>>> +
>>> +	TUNABLE_GET (spin_count, int32_t,
>>> +			TUNABLE_CALLBACK (set_mutex_spin_count));
>>> +#endif
>>> +}
>>> +
>>> +#ifdef SHARED
>>> +# define INIT_SECTION ".init_array"
>>> +#else
>>> +# define INIT_SECTION ".preinit_array"
>>> +#endif
>>> +
>>> +void (*const __pthread_mutex_tunables_init_array []) (int, char **, char **)
>>> +  __attribute__ ((section (INIT_SECTION), aligned (sizeof (void *)))) =
>>> +{
>>> +  &mutex_tunables_init
>>> +};> diff --git a/nptl/mutex-conf.h b/nptl/mutex-conf.h
>>> new file mode 100644
>>> index 0000000..babefe3
>>> --- /dev/null
>>> +++ b/nptl/mutex-conf.h
>>> @@ -0,0 +1,31 @@
>>> +/* mutex-conf.h: Pthread mutex tunable parameters.
>>> +   Copyright (C) 2013-2018 Free Software Foundation, Inc.
>>
>> Copyright should start at 2018 for newer implementations.
>>
> 
> Sure.
> 
>>> +   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/>.  */
>>> +#ifndef _MUTEX_CONF_H
>>> +#define _MUTEX_CONF_H 1
>>> +
>>> +#include <pthread.h>
>>> +#include <time.h>
>>> +
>>> +struct mutex_config
>>> +{
>>> +  int spin_count;
>>> +};
>>> +
>>> +extern struct mutex_config __mutex_aconf attribute_hidden;
>>> +
>>> +#endif
>>>
>>

On 04/04/2018 12:16 PM, Adhemerval Zanella wrote:
>>> Also I not sure if it worth to add environment variable for this tunable,
>>> I would rather avoid adding newer ones (and naming seems off, since afaik
>>> LD_* meaning some parameters that affects the loader).
>>>
>>
>> AFAIK, environment variable is what I can think of to give people the possibility of 
>> tunes in shell for that, maybe you have better idea?
>>
>> Yes, we probably use other name like MUTEX_SPIN_COUNT to avoid confusion, agree?
> 
> The tunables framework already provides a environment variable to this [1],
> the 'env_alias' is mainly to provide compatibility and to use the same logic
> internally.
> 
> So I think it is better to use the default tunable env var.
> 
> [1] https://www.gnu.org/software/libc/manual/html_node/Tunables.html

Fully agree. We should never add any new environment variable, they should all be
automatically used from the top-level tunable env var.