Re: [PATCH] nptl: Effectively skip CAS in spinlock loop

[Maxim Kuvyrkov <maxim.kuvyrkov@linaro.org>]

Hi Jangwoong,

I needed a minute to stare at your patch to understand where extra CAS execution comes from, and I meant adding a comment like the following to make code easier to understand.
+	      if (LLL_MUTEX_TRYLOCK (mutex) == 0)
                /* Do not be tempted to move this check to the loop condition.
                   See commit that added this comment for details.  */
+		break;

Feel free to merge your patch as-is, I may be overthinking this.

Regards,

--
Maxim Kuvyrkov
https://www.linaro.org

> On 6 Dec 2021, at 21:03, Jangwoong Kim via Libc-alpha <libc-alpha@sourceware.org> wrote:
> 
> Hi Maxim,
> 
> Did you mean editing the commit message?
> 
> If so, I will resend a patch right away.
> 
> Or, if you meant adding a comment to source code, I'm not clear on your request.
> 
> The reason do-while loop structure causes an extra CAS is "continue"
> expression does not skip the conditional expression do-while loop.
> 
> My patch replaces the do-while loop, so there isn't an extra CAS.
> 
> Thus, adding a comment why the previous code was wrong doesn't seem proper.
> 
> --
> Thank you.
> Jangwoong Kim
> 
> 2021년 12월 6일 (월) 오후 11:26, Maxim Kuvyrkov <maxim.kuvyrkov@linaro.org>님이 작성:
>> 
>> 
>>> On 5 Dec 2021, at 18:28, Jangwoong Kim via Libc-alpha <libc-alpha@sourceware.org> wrote:
>>> 
>>> The commit:
>>> "Add LLL_MUTEX_READ_LOCK [BZ #28537]"
>>> SHA1: d672a98a1af106bd68deb15576710cd61363f7a6
>>> 
>>> introduced LLL_MUTEX_READ_LOCK, to skip CAS in spinlock loop
>>> if atmoic load fails. But, "continue" inside of do-while loop
>>> does not skip the evaluation of escape expression, thus CAS
>>> is not skipped.
>>> 
>>> Replace do-while with while and break if LLL_MUTEX_READ_LOCK fails.
>>> ---
>>> nptl/pthread_mutex_lock.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
>>> index 47b88a6b5b..24936c8d56 100644
>>> --- a/nptl/pthread_mutex_lock.c
>>> +++ b/nptl/pthread_mutex_lock.c
>>> @@ -138,7 +138,7 @@ PTHREAD_MUTEX_LOCK (pthread_mutex_t *mutex)
>>>        int cnt = 0;
>>>        int max_cnt = MIN (max_adaptive_count (),
>>>                           mutex->__data.__spins * 2 + 10);
>>> -       do
>>> +       while (1)
>>>          {
>>>            if (cnt++ >= max_cnt)
>>>              {
>>> @@ -148,8 +148,9 @@ PTHREAD_MUTEX_LOCK (pthread_mutex_t *mutex)
>>>            atomic_spin_nop ();
>>>            if (LLL_MUTEX_READ_LOCK (mutex) != 0)
>>>              continue;
>>> +           if (LLL_MUTEX_TRYLOCK (mutex) == 0)
>>> +             break;
>>>          }
>>> -       while (LLL_MUTEX_TRYLOCK (mutex) != 0);
>> 
>> Giving this a second look, would you please add a comment before “break” to explain why do-while loop structure would cause an extra compare-and-swap?
>> 
>> --
>> Maxim Kuvyrkov
>> https://www.linaro.org
>> 
>>> 
>>>        mutex->__data.__spins += (cnt - mutex->__data.__spins) / 8;
>>>      }
>>> --
>>> 2.25.1
>>> 
>>