Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: pause in WaitForSemaphore() before re-fetch


Dong, Eric
 

Reviewed-by: Eric Dong <eric.dong@...>

-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Thursday, July 30, 2020 2:52 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>
Cc: Dong, Eric <eric.dong@...>; Philippe Mathieu-Daudé
<philmd@...>; Kumar, Rahul1 <rahul1.kumar@...>; Ni, Ray
<ray.ni@...>
Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: pause in
WaitForSemaphore() before re-fetch

Most busy waits (spinlocks) in
"UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c"
already call CpuPause() in their loop bodies; see SmmWaitForApArrival(),
APHandler(), and SmiRendezvous(). However, the "main wait" within
APHandler():

//
// Wait for something to happen
//
WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
doesn't do so, as WaitForSemaphore() keeps trying to acquire the
semaphore without pausing.

The performance impact is especially notable in QEMU/KVM + OVMF
virtualization with CPU overcommit (that is, when the guest has significantly
more VCPUs than the host has physical CPUs). The guest BSP is working
heavily in:

BSPHandler() [MpService.c]
PerformRemainingTasks() [PiSmmCpuDxeSmm.c]
SetUefiMemMapAttributes() [SmmCpuMemoryManagement.c]

while the many guest APs are spinning in the "Wait for something to happen"
semaphore acquisition, in APHandler(). The guest APs are generating useless
memory traffic and saturating host CPUs, hindering the guest BSP's progress
in SetUefiMemMapAttributes().

Rework the loop in WaitForSemaphore(): call CpuPause() in every iteration
after the first check fails. Due to Pause Loop Exiting (known as Pause Filter on
AMD), the host scheduler can favor the guest BSP over the guest APs.

Running a 16 GB RAM + 512 VCPU guest on a 448 PCPU host, this patch
reduces OVMF boot time (counted until reaching grub) from 20-30 minutes
to less than 4 minutes.

The patch should benefit physical machines as well -- according to the Intel
SDM, PAUSE "Improves the performance of spin-wait loops". Adding PAUSE
to the generic WaitForSemaphore() function is considered a general
improvement.

Cc: Eric Dong <eric.dong@...>
Cc: Philippe Mathieu-Daudé <philmd@...>
Cc: Rahul Kumar <rahul1.kumar@...>
Cc: Ray Ni <ray.ni@...>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1861718
Signed-off-by: Laszlo Ersek <lersek@...>
---

Notes:
Repo: https://pagure.io/lersek/edk2.git
Branch: sem_wait_pause_rhbz1861718

UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 57e788c01b1f..4bcd217917d7 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -40,14 +40,18 @@ WaitForSemaphore (
{
UINT32 Value;

- do {
+ for (;;) {
Value = *Sem;
- } while (Value == 0 ||
- InterlockedCompareExchange32 (
- (UINT32*)Sem,
- Value,
- Value - 1
- ) != Value);
+ if (Value != 0 &&
+ InterlockedCompareExchange32 (
+ (UINT32*)Sem,
+ Value,
+ Value - 1
+ ) == Value) {
+ break;
+ }
+ CpuPause ();
+ }
return Value - 1;
}

--
2.19.1.3.g30247aa5d201

Join devel@edk2.groups.io to automatically receive all group messages.