[edk2-devel] [PATCH v3 1/4] UefiCpuPkg/MpInitLib: Use XADD to avoid lock acquire/release
Ni, Ray
ray.ni at intel.com
Thu Feb 25 04:04:48 UTC 2021
Laszlo,
Do you think that Mike's R-b to the first patch can be an ack to the V3 patch set?
Can you please review and provide comments?
Thanks,
Ray
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney at intel.com>
> Sent: Wednesday, February 24, 2021 2:12 AM
> To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>; Kinney, Michael D
> <michael.d.kinney at intel.com>
> Cc: Dong, Eric <eric.dong at intel.com>; Laszlo Ersek <lersek at redhat.com>;
> Kumar, Rahul1 <rahul1.kumar at intel.com>
> Subject: RE: [edk2-devel] [PATCH v3 1/4] UefiCpuPkg/MpInitLib: Use XADD
> to avoid lock acquire/release
>
> Reviewed-by: Michael D Kinney <michael.d.kinney at intel.com>
>
> > -----Original Message-----
> > From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Ni,
> Ray
> > Sent: Tuesday, February 9, 2021 6:17 AM
> > To: devel at edk2.groups.io
> > Cc: Dong, Eric <eric.dong at intel.com>; Laszlo Ersek <lersek at redhat.com>;
> Kumar, Rahul1 <rahul1.kumar at intel.com>
> > Subject: [edk2-devel] [PATCH v3 1/4] UefiCpuPkg/MpInitLib: Use XADD to
> avoid lock acquire/release
> >
> > When AP firstly wakes up, MpFuncs.nasm contains below logic to assign
> > an unique ApIndex to each AP according to who comes first:
> > ---ASM---
> > TestLock:
> > xchg [edi], eax
> > cmp eax, NotVacantFlag
> > jz TestLock
> >
> > mov ecx, esi
> > add ecx, ApIndexLocation
> > inc dword [ecx]
> > mov ebx, [ecx]
> >
> > Releaselock:
> > mov eax, VacantFlag
> > xchg [edi], eax
> > ---ASM END---
> >
> > "lock inc" cannot be used to increase ApIndex because not only the
> > global ApIndex should be increased, but also the result should be
> > stored to a local general purpose register EBX.
> >
> > This patch learns from the NASM implementation of
> > InternalSyncIncrement() to use "XADD" instruction which can increase
> > the global ApIndex and store the original ApIndex to EBX in one
> > instruction.
> >
> > With this patch, OVMF when running in a 255 threads QEMU spends about
> > one second to wakeup all APs. Original implementation needs more than
> > 10 seconds.
> >
> > Signed-off-by: Ray Ni <ray.ni at intel.com>
> > Cc: Eric Dong <eric.dong at intel.com>
> > Cc: Laszlo Ersek <lersek at redhat.com>
> > Cc: Rahul Kumar <rahul1.kumar at intel.com>
> > ---
> > .../Library/MpInitLib/Ia32/MpFuncs.nasm | 20 ++++++-------------
> > UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 18 ++++++-----------
> > 2 files changed, 12 insertions(+), 26 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> > index 7e81d24aa6..2eaddc93bc 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> > +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> > @@ -1,5 +1,5 @@
> > ;------------------------------------------------------------------------------ ;
> >
> > -; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
> >
> > +; Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
> >
> > ; SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > ;
> >
> > ; Module Name:
> >
> > @@ -125,19 +125,11 @@ SkipEnableExecuteDisable:
> > add edi, LockLocation
> >
> > mov eax, NotVacantFlag
> >
> >
> >
> > -TestLock:
> >
> > - xchg [edi], eax
> >
> > - cmp eax, NotVacantFlag
> >
> > - jz TestLock
> >
> > -
> >
> > - mov ecx, esi
> >
> > - add ecx, ApIndexLocation
> >
> > - inc dword [ecx]
> >
> > - mov ebx, [ecx]
> >
> > -
> >
> > -Releaselock:
> >
> > - mov eax, VacantFlag
> >
> > - xchg [edi], eax
> >
> > + mov edi, esi
> >
> > + add edi, ApIndexLocation
> >
> > + mov ebx, 1
> >
> > + lock xadd dword [edi], ebx ; EBX = ApIndex++
> >
> > + inc ebx ; EBX is CpuNumber
> >
> >
> >
> > mov edi, esi
> >
> > add edi, StackSizeLocation
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> > index aecfd07bc0..5b588f2dcb 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> > +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> > @@ -1,5 +1,5 @@
> > ;------------------------------------------------------------------------------ ;
> >
> > -; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
> >
> > +; Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
> >
> > ; SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > ;
> >
> > ; Module Name:
> >
> > @@ -161,18 +161,12 @@ LongModeStart:
> > add edi, LockLocation
> >
> > mov rax, NotVacantFlag
> >
> >
> >
> > -TestLock:
> >
> > - xchg qword [edi], rax
> >
> > - cmp rax, NotVacantFlag
> >
> > - jz TestLock
> >
> > -
> >
> > - lea ecx, [esi + ApIndexLocation]
> >
> > - inc dword [ecx]
> >
> > - mov ebx, [ecx]
> >
> > + mov edi, esi
> >
> > + add edi, ApIndexLocation
> >
> > + mov ebx, 1
> >
> > + lock xadd dword [edi], ebx ; EBX = ApIndex++
> >
> > + inc ebx ; EBX is CpuNumber
> >
> >
> >
> > -Releaselock:
> >
> > - mov rax, VacantFlag
> >
> > - xchg qword [edi], rax
> >
> > ; program stack
> >
> > mov edi, esi
> >
> > add edi, StackSizeLocation
> >
> > --
> > 2.27.0.windows.1
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#71517):
> https://edk2.groups.io/g/devel/message/71517
> > Mute This Topic: https://groups.io/mt/80504936/1643496
> > Group Owner: devel+owner at edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [michael.d.kinney at intel.com]
> > -=-=-=-=-=-=
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72179): https://edk2.groups.io/g/devel/message/72179
Mute This Topic: https://groups.io/mt/80504936/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list