1. Patchew
  2. EDK2
  3. View series

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

Laszlo Ersek posted 1 patch 3 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/edk2 tags/patchew/20200729185217.10084-1-lersek@redhat.com
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
[edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: pause in WaitForSemaphore() before re-fetch
Posted by Laszlo Ersek 3 years, 8 months ago 
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@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1861718
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#63454): https://edk2.groups.io/g/devel/message/63454
Mute This Topic: https://groups.io/mt/75871294/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: pause in WaitForSemaphore() before re-fetch
Posted by Dong, Eric 3 years, 8 months ago 
Reviewed-by: Eric Dong <eric.dong@intel.com>

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, July 30, 2020 2:52 AM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> 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@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1861718
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> 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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#63531): https://edk2.groups.io/g/devel/message/63531
Mute This Topic: https://groups.io/mt/75871294/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: pause in WaitForSemaphore() before re-fetch
Posted by Laszlo Ersek 3 years, 8 months ago 
On 07/31/20 03:10, Dong, Eric wrote:
> Reviewed-by: Eric Dong <eric.dong@intel.com>

Thank you, merged as commit 9001b750df64, via
<https://github.com/tianocore/edk2/pull/843>.

Laszlo

>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Thursday, July 30, 2020 2:52 AM
>> To: edk2-devel-groups-io <devel@edk2.groups.io>
>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni, Ray
>> <ray.ni@intel.com>
>> 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@intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1861718
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> 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
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#63573): https://edk2.groups.io/g/devel/message/63573
Mute This Topic: https://groups.io/mt/75871294/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.