public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ni, Ray" <ray.ni@intel.com>
To: devel@edk2.groups.io
Cc: Laszlo Ersek <lersek@redhat.com>, Eric Dong <eric.dong@intel.com>,
	Rahul1 Kumar <rahul1.kumar@intel.com>
Subject: [PATCH v2 3/3] UefiCpuPkg/MpInitLib: Use XADD to avoid lock acquire/release
Date: Fri,  5 Feb 2021 15:58:10 +0800	[thread overview]
Message-ID: <20210205075810.981-4-ray.ni@intel.com> (raw)
In-Reply-To: <20210205075810.981-1-ray.ni@intel.com>

When AP firstly wakes up, MpFuncs.nasm contains below logic to assign
an unique ApIndex to each AP according to who comes first:
---NASM---
    mov        edi, esi
    add        edi, MP_CPU_EXCHANGE_INFO_FIELD (Lock)
    mov        eax, NotVacantFlag

TestLock:
    xchg       [edi], eax
    cmp        eax, NotVacantFlag
    jz         TestLock

    mov        ecx, esi
    add        ecx, MP_CPU_EXCHANGE_INFO_FIELD (ApIndex)
    inc        dword [ecx]
    mov        ebx, [ecx]

Releaselock:
    mov        eax, VacantFlag
    xchg       [edi], eax
---NASM END---

"LOCK INC" cannot be used to increase MP_CPU_EXCHANGE_INFO.ApIndex
because not only the MP_CPU_EXCHANGE_INFO.ApIndex should be
increased, but also the result should be stored to a thread 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>
---
 .../Library/MpInitLib/Ia32/MpFuncs.nasm       | 20 ++++---------------
 UefiCpuPkg/Library/MpInitLib/MpEqu.inc        |  4 ----
 UefiCpuPkg/Library/MpInitLib/MpLib.c          |  1 -
 UefiCpuPkg/Library/MpInitLib/MpLib.h          |  3 +--
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 18 ++++-------------
 5 files changed, 9 insertions(+), 37 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index 2f1b102717..7bd2415670 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -122,22 +122,10 @@ SkipEnableExecuteDisable:
 
     ; AP init
     mov        edi, esi
-    add        edi, MP_CPU_EXCHANGE_INFO_FIELD (Lock)
-    mov        eax, NotVacantFlag
-
-TestLock:
-    xchg       [edi], eax
-    cmp        eax, NotVacantFlag
-    jz         TestLock
-
-    mov        ecx, esi
-    add        ecx, MP_CPU_EXCHANGE_INFO_FIELD (ApIndex)
-    inc        dword [ecx]
-    mov        ebx, [ecx]
-
-Releaselock:
-    mov        eax, VacantFlag
-    xchg       [edi], eax
+    add        edi, MP_CPU_EXCHANGE_INFO_FIELD (ApIndex)
+    mov        ebx, 1
+    lock xadd  dword [edi], ebx                 ; EBX = ApIndex++
+    inc        ebx                              ; EBX is CpuNumber
 
     mov        edi, esi
     add        edi, MP_CPU_EXCHANGE_INFO_FIELD (StackSize)
diff --git a/UefiCpuPkg/Library/MpInitLib/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
index 46c2b5c116..2e9368a374 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
+++ b/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
@@ -13,9 +13,6 @@
 ;-------------------------------------------------------------------------------
 %include "Nasm.inc"
 
-VacantFlag                    equ        00h
-NotVacantFlag                 equ        0ffh
-
 CPU_SWITCH_STATE_IDLE         equ        0
 CPU_SWITCH_STATE_STORED       equ        1
 CPU_SWITCH_STATE_LOADED       equ        2
@@ -72,7 +69,6 @@ endstruc
 ; Equivalent NASM structure of MP_CPU_EXCHANGE_INFO
 ;
 struc MP_CPU_EXCHANGE_INFO
-  .Lock:                         CTYPE_UINTN 1
   .StackStart:                   CTYPE_UINTN 1
   .StackSize:                    CTYPE_UINTN 1
   .CFunction:                    CTYPE_UINTN 1
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 2568986d8c..5040053dad 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1006,7 +1006,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/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index bf7faaf60b..50df802d1f 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_FIELD (Lock)
-    mov        rax, NotVacantFlag
+    add        edi, MP_CPU_EXCHANGE_INFO_FIELD (ApIndex)
+    mov        ebx, 1
+    lock xadd  dword [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_FIELD (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_FIELD (StackSize)
-- 
2.27.0.windows.1


  parent reply	other threads:[~2021-02-05  7:58 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-05  7:58 [PATCH v2 0/3] Use XADD to avoid lock acquire/release Ni, Ray
2021-02-05  7:58 ` [PATCH v2 1/3] MdePkg/Nasm.inc: add macros for C types used in structure definition Ni, Ray
2021-02-05 15:02   ` [edk2-devel] " Laszlo Ersek
2021-02-05  7:58 ` [PATCH v2 2/3] UefiCpuPkg/MpInitLib: Use NASM struc to avoid hardcode offset Ni, Ray
2021-02-05 15:09   ` [edk2-devel] " Laszlo Ersek
2021-02-05  7:58 ` Ni, Ray [this message]
2021-02-05 15:11   ` [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/MpInitLib: Use XADD to avoid lock acquire/release Laszlo Ersek
2021-02-05 17:11 ` [edk2-devel] [PATCH v2 0/3] " Michael D Kinney
2021-02-05 18:38   ` Ni, Ray
2021-02-05 19:54     ` Michael D Kinney
2021-02-06  4:32       ` Ni, Ray

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=20210205075810.981-4-ray.ni@intel.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