From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, ray.ni@intel.com
Cc: Eric Dong <eric.dong@intel.com>, Rahul1 Kumar <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH 2/2] UefiCpuPkg/MpInitLib: Use XADD to avoid lock acquire/release
Date: Thu, 4 Feb 2021 10:51:08 +0100 [thread overview]
Message-ID: <2afe6a89-5469-7576-08e3-5ee60eb9e754@redhat.com> (raw)
In-Reply-To: <20210204025921.1428-3-ray.ni@intel.com>
On 02/04/21 03:59, Ni, Ray wrote:
> 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 1
> diff --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++
(1) For more clarity, I would suggest
lock xadd dword [edi], ebx
(even though "ebx" already specifies the width, yes)
Applies to the X64 version too.
> + inc ebx ; EBX is CpuNumber
>
> + ; program stack
(2) This change belongs in a separate patch. The X64 version already has
this comment, and making both versions makes sense. But touching
anything "stack-related" in the current patch is very confusing to me.
Thanks
Laszlo
> 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 1
> diff --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
>
next prev parent reply other threads:[~2021-02-04 9:51 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 ` Laszlo Ersek [this message]
2021-02-04 13:43 ` [edk2-devel] " Ni, Ray
2021-02-04 11:24 ` Zeng, Star
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=2afe6a89-5469-7576-08e3-5ee60eb9e754@redhat.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