Re: [RESEND PATCH v9] Mutex: Add pthread mutex tunables

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

On 2018/11/28 上午4:14, Adhemerval Zanella wrote:
> 
> 
> On 21/11/2018 23:45, 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 ().
> 
> With current algorithm nptl/pthread_mutex_{timed}lock.c, I think we can use 
> the full short range (32767) since assuming maximum spin_nop usage with 'cnt'
> being 'max_cnt - 1' (the fast atomic path) it will converge to 32719.
> 

yes, it's fine to limited the maximum value of spin count to 32767.
BTW, the maximum possible value of spin_nop 'cnt' would be 'max_cnt + 1'.

>>
>> 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.
>>
>>  
>> +#if HAVE_TUNABLES
>> +struct mutex_config
>> +{
>> +  int spin_count;
>> +};
>> +
>> +extern struct mutex_config __mutex_aconf;
>> +#endif
>> +
>> +/* Adaptive mutex definitions.  */
>> +#ifndef MAX_ADAPTIVE_COUNT
>> +# if HAVE_TUNABLES
>> +#  define MAX_ADAPTIVE_COUNT __mutex_aconf.spin_count
>> +# else
>> +#  define MAX_ADAPTIVE_COUNT 100
>> +# endif
>> +#endif
>> +
>>  /* Internal variables.  */
>>  
> 
> We try to avoid the logic to check if a define is already define to set a
> default value.  If the idea is on future patches to define an arch-specific
> value, the preferred strategy is to define the generic and an arch-specific
> value in their own headers.
> 

Agree!

>>  
>> diff --git a/nptl/pthread_mutex_conf.c b/nptl/pthread_mutex_conf.c
>> new file mode 100644
>> index 0000000..0da4afe
>> --- /dev/null
>> +++ b/nptl/pthread_mutex_conf.c
>> @@ -0,0 +1,27 @@
>> +/* pthread_mutex_conf.c: Pthread mutex tunable parameters.
> 
> It is not a rule, but there is no need to duplicate the file name on its
> description.
> 

OK. Will remove the file name

>> +   Copyright (C) 2018 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/>.  */
>> +
>> +#if HAVE_TUNABLES
>> +#include <pthreadP.h>
>> +struct mutex_config __mutex_aconf =
>> +{
>> +  /* The maximum number of times a thread should spin on the lock before
>> +  calling into kernel to block.  */
>> +  .spin_count = 100,
>> +};
>> +#endif
> 
> If the idea is to keep both non-tunable version and tunable version in sync
> when default value change, I would suggest to use an extra define to hold
> the '100' value.
> 
> Another option, which I think it better, is to avoid make MAX_ADAPTIVE_COUNT
> have different meaning depending if tunables is enabled or not and just
> create a inline function:
> 
> nptl/pthreadP.h:
> 
> #define MAX_DEFAULT_ADAPTIVE_COUNT 100
> 
> static inline short max_adaptive_count (void)
> {
> #if HAVE_TUNABLES
>   return __mutex_aconf.spin_count;
> #else
>   return MAX_DEFAULT_ADAPTIVE_COUNT;
> #endif
> }
> 
> And adjust nptl/pthread_mutex_conf.c, nptl/pthread_mutex_lock.c, and
> nptl/pthread_mutex_timedlock.c accordingly.
> 

Sure

> Also, if the idea is to provide a different generic value depending
> of the architecture it is a matter to move the MAX_DEFAULT_ADAPTIVE_COUNT
> to a generic header and override the header by the architecture.
> 

thanks!
The v10 will be like this:
the DEFAULT_ADAPTIVE_COUNT would be architecture-specific, defined in a generic
file sysdeps/generic/adaptive_spin_count.h, in future, different architectures
(or even different platforms) will have its individual version.
E.g x86 may use /sysdeps/unix/sysv/linux/x86/adaptive_spin_count.h to define this
default value.

I am not familiar with toolchains, is it a right way to define a generic
file and a architecture-specific file like this?

If HAVE_TUNABLES is defined as true, DEFAULT_ADAPTIVE_COUNT will be ignored.

>> +#if HAVE_TUNABLES
>> +# define TUNABLE_NAMESPACE pthread
>> +# include <stdbool.h>
>> +# include <stdint.h>
>> +# include <pthreadP.h>
>> +# include <pthread_mutex_conf.h>
>> +# include <unistd.h>  /* Get STDOUT_FILENO for _dl_printf.  */
>> +# include <elf/dl-tunables.h>
> 
> Mostly of the includes seems superfluous, I think only pthread_mutex_conf.h
> and elf/dl-tunables.h are really required.
> 

maybe not.

For pthreadP.h, I defines struct mutex_config in pthreadP.h, so we have to include header pthreadP.h here
Or, if you don't want header pthreadP.h included, one alternative way is to move this structure definition back
to pthread_mutex_conf.h. But, we will have to include header pthread_mutex_conf.h in pthreadP.h (required by ) 
I think both are OK for me, your idea?

For stdint.h, I tried to remove it, the system complains
In file included from ../elf/dl-tunables.h:34:0,
                       from pthread_mutex_conf.c:23:

../elf/dl-tunable-types.h:35:3: error: unknown type name ‘int64_t’
   int64_t min;
   ^

Then, I added the header stdint.h back, then, the system complains
../elf/dl-tunables.h:42:3: error: unknown type name ‘bool’
  bool initialized;   /* Flag to indicate that the tunable is
  ^
Then, I added the header stdbool.h back, and continue

For unistd.h, it's OK to remove on 64-bits system, but it will fail to compile in 32-bits system.
The comment added after this header explains it: "Get STDOUT_FILENO for _dl_printf"

So, all of the header files listed above are required in current implementation.
Hmm, pthreadP.h should not be needed if moving struct mutex_config back to pthread_mutex_conf.h.

Anyway, I will post v10 for all of those changes.
Thanks for your review and suggestion.

>> +
>> +extern struct mutex_config __mutex_aconf attribute_hidden;
>> +
>> +static void
>> +TUNABLE_CALLBACK (set_mutex_spin_count) (tunable_val_t *valp)
>> +{
>> +  __mutex_aconf.spin_count = (int32_t) (valp)->numval;
>> +}
>> +
>> +static void pthread_tunables_init (void)
>> +{
>> +  TUNABLE_GET (mutex_spin_count, int32_t,
>> +               TUNABLE_CALLBACK (set_mutex_spin_count));
>> +}
>> +#endif
>> +
>> +#endif
>> diff --git a/sysdeps/nptl/dl-tunables.list b/sysdeps/nptl/dl-tunables.list
>> new file mode 100644
>> index 0000000..1cb52e8
>> --- /dev/null
>> +++ b/sysdeps/nptl/dl-tunables.list
>> @@ -0,0 +1,51 @@
>> +# Copyright (C) 2018 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/>.
>> +
>> +# Allowed attributes for tunables:
>> +#
>> +# type: Defaults to STRING
>> +# minval: Optional minimum acceptable value
>> +# maxval: Optional maximum acceptable value
>> +# env_alias: An alias environment variable
>> +# security_level: Specify security level of the tunable.  Valid values are:
>> +#
>> +# 	     SXID_ERASE: (default) Don't read for AT_SECURE binaries and
>> +# 	     		 removed so that child processes can't read it.
>> +# 	     SXID_IGNORE: Don't read for AT_SECURE binaries, but retained for
>> +# 	     		  non-AT_SECURE subprocesses.
>> +# 	     NONE: Read all the time.
> 
> Do we need to duplicate the same comments from 'elf/dl-tunables.list'?
> 

Will remove them in v10.