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

[Jangwoong Kim <6812skiii@gmail.com>]

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