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

[Florian Weimer]

On 05/02/2018 01:06 PM, kemi wrote:
> 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.

Could you add this as a comment, please?

> 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.

Ahh, makes sense.  Perhaps put this information into the commit message.

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

Sure, or if there is just a single good choice for the tunable, just use 
that and remove the tunable again.  I guess one aspect here is to 
experiment with different values and see if there's a clear winner.

>>> +# 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.

We can still macroize the code at that point.  But no strong preference 
here.

>>> +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.

The code would go into nptl/nptl-init.c.  It's just an idea, but I think 
it should be possible to make it work.

Thanks,
Florian