Re: [PATCH v2 3/3] Mutex: Optimize adaptive spin algorithm

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

On 2018年05月02日 16:19, Florian Weimer wrote:
> On 04/25/2018 04:56 AM, Kemi Wang wrote:
>> @@ -124,21 +125,24 @@ __pthread_mutex_lock (pthread_mutex_t *mutex)
>>         if (LLL_MUTEX_TRYLOCK (mutex) != 0)
>>       {
>>         int cnt = 0;
> …
>> +      int max_cnt = MIN (__mutex_aconf.spin_count,
>> +            mutex->__data.__spins * 2 + 100);
>> +
>> +      /* MO read while spinning */
>> +      do
>> +        {
>> +         atomic_spin_nop ();
>> +        }
>> +      while (atomic_load_relaxed (&mutex->__data.__lock) != 0 &&
>> +            ++cnt < max_cnt);
>> +        /* Try to acquire the lock if lock is available or the spin count
>> +         * is run out, call into kernel to block if fails
>> +         */
>> +      if (LLL_MUTEX_TRYLOCK (mutex) != 0)
>> +        LLL_MUTEX_LOCK (mutex);
>>   
> …
>> +      mutex->__data.__spins += (cnt - mutex->__data.__spins) / 8;
>> +    }
> 
> The indentation is off.  Comments should end with a ”.  ” (dot and two spaces).  Multi-line comments do not start with “*” on subsequent lines.  We don't use braces when we can avoid them.  Operators such as “&&” should be on the following line when breaking up lines.
> 

Will fold these changes in next version. 
I am not familiar with glibc coding style, apologize for that.

> Why is the LLL_MUTEX_TRYLOCK call still needed?  Shouldn't be an unconditional call to LLL_MUTEX_LOCK be sufficient?
> 

The purpose of calling LLL_MUTEX_TRYLOCK here is to try to acquire the lock at user 
space without block when we observed the lock is available. Thus, in case of multiple 
spinners contending for the lock,  only one spinner can acquire the lock successfully
and others fall into block.

I am not sure an unconditional call to LLL_MUTEX_LOCK as you mentioned here can satisfy 
this purpose.

> But the real question is if the old way of doing CAS in a loop is beneficial on other, non-Intel architectures.  You either need get broad consensus from the large SMP architectures (various aarch64 implementations, IBM POWER and Z), or somehow make this opt-in at the source level.
> 

That would be a platform-specific change and have obvious performance improvement for x86 architecture.
And according to Adhemerval, this change could also have some improvement for arrch64 architecture.
If you or someone else still have some concern of performance regression on other architecture, making
this opt-in could eliminate people's worries. 

"
I checked the change on a 64 cores aarch64 machine, but
differently than previous patch this one seems to show improvements:

nr_threads      base            head(SPIN_COUNT=10)  head(SPIN_COUNT=1000)
1               27566206        28776779 (4.206770)  28778073 (4.211078)
2               8498813         9129102 (6.904173)   7042975 (-20.670782)
7               5019434         5832195 (13.935765)  5098511 (1.550982)
14              4379155         6507212 (32.703053)  5200018 (15.785772)
28              4397464         4584480 (4.079329)   4456767 (1.330628)
56              4020956         3534899 (-13.750237) 4096197 (1.836850)
"

Also, I would appreciate if someone could test this patch on other architectures using
the benchmark provided in the second patch. 

> Ideally, we would also get an ack from AMD for the x86 change, but they seem not particularly active on libc-alpha, so that may not be realistic.
> 
> Thanks,
> Florian