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

[Florian Weimer <fweimer@redhat.com>]

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

No apology needed, it takes some time to get use to.

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

It's what we use for the default case.  It expands to lll_lock, so it 
should try atomic_compare_and_exchange_bool_acq first and only perform a 
futex syscall in case there is contention.  So I do think that 
LLL_MUTEX_TRYLOCK is redundant here.  Perhaps manually review the 
disassembly to make sure?

> 
>> 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)
> "

Ah, nice, I had missed that.  I suppose this means we can risk enabling 
it by default.

Thanks,
Florian