Re: [PATCH v2 1/3] Tunables: Add tunables of spin count for pthread adaptive spin mutex

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

Hi, Florian
    Thanks for your time to review.

On 2018年05月02日 16:04, Florian Weimer wrote:
> On 04/25/2018 04:56 AM, Kemi Wang wrote:
> 
>> +  mutex {
>> +    spin_count {
>> +      type: INT_32
>> +      minval: 0
>> +      maxval: 30000
>> +      default: 1000
>> +    }
> 
> How did you come up with the default and maximum values?  Larger maximum values might be useful for testing boundary conditions.
> 

For the maximum value of spin count:
Please notice that mutex->__data.__spins += (cnt - mutex->__data.__spins) / 8, and the variable *cnt* could reach 
the value of spin count due to spinning timeout. In such case, mutex->__data.__spins is increased and could be close to *cnt*
(close to the value of spin count). Keeping the value of spin count less than MAX_SHORT can avoid the overflow of
 mutex->__data.__spins variable with the possible type of short.


For the default value of spin count:
I referred to the previous number of 100 times for trylock in the loop. When this mode is changed to read only while spinning.
I suppose the value could be larger because of lower overhead and latency of read compared with cmpxchg.

Perhaps we should make the default value of spin count differently according to architecture. 

>> +# define TUNABLE_CALLBACK_FNDECL(__name, __type)            \
>> +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);
> 
> I'm not sure if the macro is helpful in this context.
> 

It is a matter of taste.
But, perhaps we have other mutex tunables in future.

>> +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
>> +};
> 
> Can't you perform the initialization as part of overall pthread initialization?  This would avoid the extra relocation.
> 

Thanks for your suggestion. I am not sure how to do it now and will take a look at it.

> Thanks,
> Florian