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.kin...@intel.com> > Sent: Wednesday, February 24, 2021 2:12 AM > To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; Kinney, Michael D > <michael.d.kin...@intel.com> > Cc: Dong, Eric <eric.d...@intel.com>; Laszlo Ersek <ler...@redhat.com>; > Kumar, Rahul1 <rahul1.ku...@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.kin...@intel.com> > > > -----Original Message----- > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, > Ray > > Sent: Tuesday, February 9, 2021 6:17 AM > > To: devel@edk2.groups.io > > Cc: Dong, Eric <eric.d...@intel.com>; Laszlo Ersek <ler...@redhat.com>; > Kumar, Rahul1 <rahul1.ku...@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...@intel.com> > > Cc: Eric Dong <eric.d...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Cc: Rahul Kumar <rahul1.ku...@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+ow...@edk2.groups.io > > Unsubscribe: https://edk2.groups.io/g/devel/unsub > [michael.d.kin...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-