From: "Zeng, Star" <star.zeng@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
"Ni, Ray" <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>,
"Dong, Eric" <eric.dong@intel.com>,
"Kumar, Rahul1" <rahul1.kumar@intel.com>,
"Zeng, Star" <star.zeng@intel.com>
Subject: Re: [edk2-devel] [PATCH 2/2] UefiCpuPkg/MpInitLib: Use XADD to avoid lock acquire/release
Date: Thu, 4 Feb 2021 11:24:29 +0000 [thread overview]
Message-ID: <DM6PR11MB4058DAD357C1FBD5E9EC27E1E3B39@DM6PR11MB4058.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20210204025921.1428-3-ray.ni@intel.com>
Hi All,
Do you think it worth or not to also update MpFuncs.nasm in Edk2\UefiCpuPkg\PiSmmCpuDxeSmm?
Thanks,
Star
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Thursday, February 4, 2021 10:59 AM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>;
> Kumar, Rahul1 <rahul1.kumar@intel.com>
> Subject: [edk2-devel] [PATCH 2/2] 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@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Rahul1 Kumar <rahul1.kumar@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc | 4 ----
> .../Library/MpInitLib/Ia32/MpFuncs.nasm | 21 +++++--------------
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 3 +--
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 3 +--
> UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 4 ----
> UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 18 ++++------------
> 6 files changed, 11 insertions(+), 42 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> index 244c1e72b7..5f1f0351d2 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> @@ -12,16 +12,12 @@
> ; ;------------------------------------------------------------------------------- -
> VacantFlag equ 00h-NotVacantFlag equ 0ffh-
> CPU_SWITCH_STATE_IDLE equ 0 CPU_SWITCH_STATE_STORED
> equ 1 CPU_SWITCH_STATE_LOADED equ 2
> MP_CPU_EXCHANGE_INFO_OFFSET equ (SwitchToRealProcEnd -
> RendezvousFunnelProcStart) struc MP_CPU_EXCHANGE_INFO- .Lock:
> resd 1 .StackStart: resd 1 .StackSize: resd 1 .CFunction:
> resd 1diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> index 908c2eb447..b7267393db 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> @@ -122,23 +122,12 @@ SkipEnableExecuteDisable:
> ; AP init mov edi, esi- add edi,
> MP_CPU_EXCHANGE_INFO_OFFSET- mov eax, NotVacantFlag--
> TestLock:- xchg [edi], eax- cmp eax, NotVacantFlag- jz
> TestLock-- mov ecx, esi- add ecx,
> MP_CPU_EXCHANGE_INFO_OFFSET + MP_CPU_EXCHANGE_INFO.ApIndex-
> inc dword [ecx]- mov ebx, [ecx]--Releaselock:- mov eax,
> VacantFlag- xchg [edi], eax+ add edi,
> MP_CPU_EXCHANGE_INFO_OFFSET +
> MP_CPU_EXCHANGE_INFO.ApIndex+ mov ebx, 1+ lock xadd [edi],
> ebx ; EBX = ApIndex+++ inc ebx ; EBX is
> CpuNumber + ; program stack mov edi, esi add edi,
> MP_CPU_EXCHANGE_INFO_OFFSET + MP_CPU_EXCHANGE_INFO.StackSize
> mov eax, [edi]diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 8b1f7f84ba..32a3951742 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1,7 +1,7 @@
> /** @file CPU MP Initialize Library common functions. - Copyright (c) 2016 -
> 2020, Intel Corporation. All rights reserved.<BR>+ Copyright (c) 2016 - 2021,
> Intel Corporation. All rights reserved.<BR> Copyright (c) 2020, AMD Inc. All
> rights reserved.<BR> SPDX-License-Identifier: BSD-2-Clause-Patent@@ -
> 1012,7 +1012,6 @@ FillExchangeInfoData (
> IA32_CR4 Cr4; ExchangeInfo = CpuMpData-
> >MpCpuExchangeInfo;- ExchangeInfo->Lock = 0; ExchangeInfo-
> >StackStart = CpuMpData->Buffer; ExchangeInfo->StackSize =
> CpuMpData->CpuApStackSize; ExchangeInfo->BufferStart = CpuMpData-
> >WakeupBuffer;diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 02652eaae1..0bd60388b1 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -1,7 +1,7 @@
> /** @file Common header file for MP Initialize Library. - Copyright (c) 2016
> - 2020, Intel Corporation. All rights reserved.<BR>+ Copyright (c) 2016 - 2021,
> Intel Corporation. All rights reserved.<BR> Copyright (c) 2020, AMD Inc. All
> rights reserved.<BR> SPDX-License-Identifier: BSD-2-Clause-Patent@@ -
> 190,7 +190,6 @@ typedef struct _CPU_MP_DATA CPU_MP_DATA;
> // into this structure are used in assembly code in this module // typedef
> struct {- UINTN Lock; UINTN StackStart; UINTN
> StackSize; UINTN CFunction;diff --git
> a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> index 3974330991..32e9a262bc 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> @@ -12,16 +12,12 @@
> ; ;------------------------------------------------------------------------------- -
> VacantFlag equ 00h-NotVacantFlag equ 0ffh-
> CPU_SWITCH_STATE_IDLE equ 0 CPU_SWITCH_STATE_STORED
> equ 1 CPU_SWITCH_STATE_LOADED equ 2
> MP_CPU_EXCHANGE_INFO_OFFSET equ (SwitchToRealProcEnd -
> RendezvousFunnelProcStart) struc MP_CPU_EXCHANGE_INFO- .Lock:
> resq 1 .StackStart: resq 1 .StackSize: resq 1 .CFunction:
> resq 1diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index 423beb2cca..383b4974f8 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -158,21 +158,11 @@ LongModeStart:
> ; AP init mov edi, esi- add edi,
> MP_CPU_EXCHANGE_INFO_OFFSET + MP_CPU_EXCHANGE_INFO.Lock-
> mov rax, NotVacantFlag+ add edi,
> MP_CPU_EXCHANGE_INFO_OFFSET +
> MP_CPU_EXCHANGE_INFO.ApIndex+ mov ebx, 1+ lock xadd [edi],
> ebx ; EBX = ApIndex+++ inc ebx ; EBX is
> CpuNumber -TestLock:- xchg qword [edi], rax- cmp rax,
> NotVacantFlag- jz TestLock-- lea ecx, [esi +
> MP_CPU_EXCHANGE_INFO_OFFSET +
> MP_CPU_EXCHANGE_INFO.ApIndex]- inc dword [ecx]- mov ebx,
> [ecx]--Releaselock:- mov rax, VacantFlag- xchg qword [edi], rax ;
> program stack mov edi, esi add edi,
> MP_CPU_EXCHANGE_INFO_OFFSET +
> MP_CPU_EXCHANGE_INFO.StackSize--
> 2.27.0.windows.1
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#71132): https://edk2.groups.io/g/devel/message/71132
> Mute This Topic: https://groups.io/mt/80372087/1779220
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [star.zeng@intel.com] -
> =-=-=-=-=-=
>
next prev parent reply other threads:[~2021-02-04 11:24 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-04 2:59 [PATCH 0/2] UefiCpuPkg/MpInitLib: Use XADD to avoid lock acquire/release Ni, Ray
2021-02-04 2:59 ` [PATCH 1/2] UefiCpuPkg/MpInitLib: Use NASM struc to avoid hardcode offset Ni, Ray
2021-02-04 9:32 ` [edk2-devel] " Laszlo Ersek
2021-02-04 9:44 ` Laszlo Ersek
2021-02-04 14:27 ` Ni, Ray
2021-02-04 15:51 ` Laszlo Ersek
2021-02-04 2:59 ` [PATCH 2/2] UefiCpuPkg/MpInitLib: Use XADD to avoid lock acquire/release Ni, Ray
2021-02-04 9:51 ` [edk2-devel] " Laszlo Ersek
2021-02-04 13:43 ` Ni, Ray
2021-02-04 11:24 ` Zeng, Star [this message]
2021-02-04 11:58 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DM6PR11MB4058DAD357C1FBD5E9EC27E1E3B39@DM6PR11MB4058.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox