* [PATCH] nptl: Effectively skip CAS in spinlock loop
@ 2021-12-05 15:28 Jangwoong Kim
2021-12-06 14:11 ` Maxim Kuvyrkov
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jangwoong Kim @ 2021-12-05 15:28 UTC (permalink / raw)
To: libc-alpha; +Cc: Jangwoong Kim
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);
mutex->__data.__spins += (cnt - mutex->__data.__spins) / 8;
}
--
2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nptl: Effectively skip CAS in spinlock loop
2021-12-05 15:28 [PATCH] nptl: Effectively skip CAS in spinlock loop Jangwoong Kim
@ 2021-12-06 14:11 ` Maxim Kuvyrkov
2021-12-06 14:26 ` Maxim Kuvyrkov
2021-12-13 14:42 ` Andreas Schwab
2 siblings, 0 replies; 6+ messages in thread
From: Maxim Kuvyrkov @ 2021-12-06 14:11 UTC (permalink / raw)
To: Jangwoong Kim; +Cc: libc-alpha
> 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(-)
Good catch! Looks good to me.
--
Maxim Kuvyrkov
https://www.linaro.org
>
> 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);
>
> mutex->__data.__spins += (cnt - mutex->__data.__spins) / 8;
> }
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nptl: Effectively skip CAS in spinlock loop
2021-12-05 15:28 [PATCH] nptl: Effectively skip CAS in spinlock loop Jangwoong Kim
2021-12-06 14:11 ` Maxim Kuvyrkov
@ 2021-12-06 14:26 ` Maxim Kuvyrkov
2021-12-06 18:03 ` Jangwoong Kim
2021-12-13 14:42 ` Andreas Schwab
2 siblings, 1 reply; 6+ messages in thread
From: Maxim Kuvyrkov @ 2021-12-06 14:26 UTC (permalink / raw)
To: Jangwoong Kim; +Cc: libc-alpha
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nptl: Effectively skip CAS in spinlock loop
2021-12-06 14:26 ` Maxim Kuvyrkov
@ 2021-12-06 18:03 ` Jangwoong Kim
2021-12-13 13:42 ` Maxim Kuvyrkov
0 siblings, 1 reply; 6+ messages in thread
From: Jangwoong Kim @ 2021-12-06 18:03 UTC (permalink / raw)
To: Maxim Kuvyrkov; +Cc: libc-alpha
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
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nptl: Effectively skip CAS in spinlock loop
2021-12-06 18:03 ` Jangwoong Kim
@ 2021-12-13 13:42 ` Maxim Kuvyrkov
0 siblings, 0 replies; 6+ messages in thread
From: Maxim Kuvyrkov @ 2021-12-13 13:42 UTC (permalink / raw)
To: Jangwoong Kim; +Cc: libc-alpha
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
>>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nptl: Effectively skip CAS in spinlock loop
2021-12-05 15:28 [PATCH] nptl: Effectively skip CAS in spinlock loop Jangwoong Kim
2021-12-06 14:11 ` Maxim Kuvyrkov
2021-12-06 14:26 ` Maxim Kuvyrkov
@ 2021-12-13 14:42 ` Andreas Schwab
2 siblings, 0 replies; 6+ messages in thread
From: Andreas Schwab @ 2021-12-13 14:42 UTC (permalink / raw)
To: Jangwoong Kim via Libc-alpha; +Cc: Jangwoong Kim
On Dez 06 2021, Jangwoong Kim via Libc-alpha wrote:
> @@ -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;
Please combine the two conditions to make it a less confusing control flow.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-13 14:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-05 15:28 [PATCH] nptl: Effectively skip CAS in spinlock loop Jangwoong Kim
2021-12-06 14:11 ` Maxim Kuvyrkov
2021-12-06 14:26 ` Maxim Kuvyrkov
2021-12-06 18:03 ` Jangwoong Kim
2021-12-13 13:42 ` Maxim Kuvyrkov
2021-12-13 14:42 ` Andreas Schwab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).