Re: [PATCH v7 1/2] Mutex: Add pthread mutex tunables

[kemi <kemi dot wang at intel dot com>]

On 2018年07月07日 01:53, Carlos O'Donell wrote:
> On 07/06/2018 03:50 AM, 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.pthread.mutex_spin_count tunes can be used by system administrator to
>> squeeze system performance according to different hardware capabilities and
>> workload characteristics.
>>
>> The maximum value of spin count is limited to 30000 to avoid the overflow
>> of mutex->__data.__spins variable with the possible type of short in
>> pthread_mutex_lock ().
>>
>> The default value of spin count is set to 100 with the reference to the
>> previous number of times of spinning via trylock. This value would be
>> architecture-specific and can be tuned with kinds of benchmarks to fit most
>> cases in future.
>>
>> 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.
>>
>>    * sysdeps/nptl/dl-tunables.list: Add glibc.pthread.mutex_spin_count entry.
>>    * manual/tunables.texi: Add glibc.pthread.mutex_spin_count description.
>>    * sysdeps/nptl/pthread_mutex_conf.h: New file.
>>    * nptl/pthread_mutex_conf.c: New file.
>>    * nptl/Makefile: Add pthread_mutex_conf.c for compilation.
>>    * nptl/nptl-init.c: Put pthread mutex tunable initialization in pthread
>>      initialization.
>>    * nptl/pthreadP.h: Move MAX_ADAPTIVE_COUNT macro definition to
>>      pthread_mutex_conf.h
>>
>> I would extend my appreciation sincerely to H.J.Lu for his help to refine
>> this patch series.
> 
> Have you, to the best of your ability answered all of the questions posed
> by the previous reviewers i.e. Adhemerval Zanella, Rical Jasan, Florian Weimer,
> Szabolcs Nagy, H.J. Lu?
> 

I did.
> This version looks good to me if you met the requirements of the above
> reviewers, and if you fixup the manual as noted below.
> 

Sure.

> With the above changes I'm OK with *this* patch going in (does not include the
> second patch in the series).
> 

OK, I will submit v8 for this patch individually.
For the second patch, please see my reply.
https://sourceware.org/ml/libc-alpha/2018-07/msg00228.html

> This patch is generic and basically exposes the spin count as a tunable and
> that's useful for all sorts of experimentation with workloads.
> 
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> 

Thanks

>> ChangeLog:
>>     V6->V7:
>>     a) Patch is refined by H.J.Lu
>>
>>     V5->V6:
>>     a) Missing "pthread mutex tunables" entry in the menu of tunables.texi,
>>     add it.
>>
>>     V4->V5
>>     a) Put mutex tunable (glibc.mutex.spin_count) initialization as part of
>>     overall pthread initialization, that would avoid the extra relocation,
>>     as suggested by Florian Weimer. Thanks for pointing it out!
>>     b) Move the READ_ONLY_SPIN macro definition from the third patch to
>>     this patch
>>
>>     V3->V4
>>     a) Add comments in elf/dl-tunables.list
>>
>>     V2->V3
>>     a) Polish the description of glibc.mutex.spin_count tunable with the
>>     help from Rical Jasan.
>>     b) Get rid of the TUNABLE_CALLBACK_FNDECL macros in
>>     pthread_mutex_conf.c, as suggested by Florian Weimer.
>>     c) Adjust the default value of spin count to 100 with the reference of
>>     the previous spinning way via trylock.
>>
>>     V1->V2
>>     a) Renamed nptl/mutex-conf.h -> nptl/pthread_mutex_conf.h
>>     b) Renamed nptl/mutex-conf.c -> nptl/pthread_mutex_conf.c
>>     c) Change the Makefile to compile pthread_mutex_conf.c
>>     d) Modify the copyright "2013-2018" -> "2018" for new added files
>>     e) Fix the indentation issue (tab -> double space) in
>>     elf/dl-tunables.list
>>     f) Remove the env alias LD_SPIN_COUNT in elf/dl-tunables.list
>>     g) Fix the typo errors and refresh glibc.mutex.spin_count tunable
>>     description in manual/tunables.texi.
>>     h) Fix the indentation issue in nptl/pthread_mutex_conf.c
>>     i) Fix the indentation issue for nested preprocessor (add one space for
>>     each level)
>>
>> Suggested-by: Andi Kleen <andi.kleen@intel.com>
>> Signed-off-by: Kemi.wang <kemi.wang@intel.com>
>> ---
>>  manual/tunables.texi              | 23 ++++++++++++++++++
>>  nptl/Makefile                     |  3 ++-
>>  nptl/nptl-init.c                  |  5 ++++
>>  nptl/pthreadP.h                   |  7 +-----
>>  nptl/pthread_mutex_conf.c         | 44 +++++++++++++++++++++++++++++++++
>>  sysdeps/nptl/dl-tunables.list     | 51 +++++++++++++++++++++++++++++++++++++++
>>  sysdeps/nptl/pthread_mutex_conf.h | 39 ++++++++++++++++++++++++++++++
>>  7 files changed, 165 insertions(+), 7 deletions(-)
>>  create mode 100644 nptl/pthread_mutex_conf.c
>>  create mode 100644 sysdeps/nptl/dl-tunables.list
>>  create mode 100644 sysdeps/nptl/pthread_mutex_conf.h
>>
>> diff --git a/manual/tunables.texi b/manual/tunables.texi
>> index be33c9f..8cdfde1 100644
>> --- a/manual/tunables.texi
>> +++ b/manual/tunables.texi
>> @@ -32,6 +32,7 @@ their own namespace.
>>  * Tunable names::  The structure of a tunable name
>>  * Memory Allocation Tunables::  Tunables in the memory allocation subsystem
>>  * Elision Tunables::  Tunables in elision subsystem
>> +* Pthread Mutex Tunables:: Tunables in pthread subsystem
> 
> POSIX Thread Tunables:: Tunables in the POSIX thread subsystem
> 

OK.

>>  * Hardware Capability Tunables::  Tunables that modify the hardware
>>  				  capabilities seen by @theglibc{}
>>  @end menu
>> @@ -281,6 +282,28 @@ of try lock attempts.
>>  The default value of this tunable is @samp{3}.
>>  @end deftp
>>  
>> +@node Pthread Mutex Tunables
> 
> POSIX Thread Tunables
> 

OK

>> +@section Pthread Mutex Tunables
> 
> Mutex Tunables
> 

Why don't we keep the same name in @node and @section?

>> +@cindex pthread mutex tunables
> 
> thread mutex tunables
> 

OK. But seems Rical has a better idea?