Received-SPF: pass (google.com: domain of linux-kernel+bounces-141505-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240325140943.815051-1-ubizjak@gmail.com> <20240325140943.815051-2-ubizjak@gmail.com>
 <Zhfkinpp92Ja6Orl@gmail.com> <CAFULd4ZhJonzsP1GexJOy1PTx4PCTZoU1ukQrpmaNusumU2FTA@mail.gmail.com>
In-Reply-To: <CAFULd4ZhJonzsP1GexJOy1PTx4PCTZoU1ukQrpmaNusumU2FTA@mail.gmail.com>
From: Uros Bizjak <ubizjak@gmail.com>
Date: Thu, 11 Apr 2024 21:08:14 +0200
Message-ID: <CAFULd4bLBuPUK-Efdw65h3=fk2819zFnNkvUBh_OdNBbNFJBQg@mail.gmail.com>
Subject: Re: [PATCH 2/2] locking/pvqspinlock: Use try_cmpxchg() in qspinlock_paravirt.h
To: Ingo Molnar <mingo@kernel.org>
Cc: linux-kernel@vger.kernel.org, Peter Zijlstra <peterz@infradead.org>, 
	Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>, Waiman Long <longman@redhat.com>, 
	Boqun Feng <boqun.feng@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 11, 2024 at 3:35=E2=80=AFPM Uros Bizjak <ubizjak@gmail.com> wro=
te:
>
> On Thu, Apr 11, 2024 at 3:24=E2=80=AFPM Ingo Molnar <mingo@kernel.org> wr=
ote:
> >
> >
> > * Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> > > -     locked =3D cmpxchg_release(&lock->locked, _Q_LOCKED_VAL, 0);
> > > -     if (likely(locked =3D=3D _Q_LOCKED_VAL))
> > > +     if (try_cmpxchg_release(&lock->locked, &locked, 0);
> > >               return;                                   ^------------=
 ???
> >
> > This doesn't appear to be a particularly well-tested patch. ;-)
>
> Ouch, embarrassing... oh it is a generic function, conditionally compiled=
 with
>
> #ifndef __pv_queued_spin_lock
> #endif
>
> and x86 defines its own function ...

Looking at the assembly of the fixed function, it looks that improved
generic function is better than x86_64 special asm:

This is the new generic function:

0000000000000750 <__pv_queued_spin_unlock>:
 750:    f3 0f 1e fa              endbr64
 754:    b8 01 00 00 00           mov    $0x1,%eax
 759:    31 d2                    xor    %edx,%edx
 75b:    f0 0f b0 17              lock cmpxchg %dl,(%rdi)
 75f:    75 05                    jne    766 <__pv_queued_spin_unlock+0x16>
 761:    e9 00 00 00 00           jmp    766 <__pv_queued_spin_unlock+0x16>
            762: R_X86_64_PLT32    __x86_return_thunk-0x4
 766:    0f b6 f0                 movzbl %al,%esi
 769:    e9 02 ff ff ff           jmp    670 <__pv_queued_spin_unlock_slowp=
ath>

and  the x86_64 asm version:

0000000000000050 <__raw_callee_save___pv_queued_spin_unlock>:
  50:    f3 0f 1e fa              endbr64
  54:    52                       push   %rdx
  55:    b8 01 00 00 00           mov    $0x1,%eax
  5a:    31 d2                    xor    %edx,%edx
  5c:    f0 0f b0 17              lock cmpxchg %dl,(%rdi)
  60:    3c 01                    cmp    $0x1,%al
  62:    75 06                    jne    6a <.slowpath>
  64:    5a                       pop    %rdx
  65:    e9 00 00 00 00           jmp    6a <.slowpath>

I didn't investigate slowpath, but the generic fast-path part is
certainly better than x86_64 special asm.

Uros.