Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath

["Gao, Zhichao" <zhichao.gao@intel.com>]

The patch looks good to me.
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

The comments of this patch and proposal would be taken care in another patch. Better to have a Bugzilla record for the planned change.

Thanks,
Zhichao

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Aaron
> Young
> Sent: Tuesday, October 10, 2023 11:07 PM
> To: devel@edk2.groups.io
> Subject: [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize
> BmExpandPartitionDevicePath
> 
> Reference: https://github.com/tianocore/edk2/pull/4892
> 
> BmExpandPartitionDevicePath is called to expand "short-form" device paths
> which are commonly used with OS boot options. To expand a device path, it
> calls EfiBootManagerConnectAll to connect all the possible BlockIo devices in
> the system to search for a matching partition. However, this is sometimes
> unnecessary on certain platforms (such as OVMF/QEMU) because the boot
> devices are previously explicity connected
> (See: ConnectDevicesFromQemu).  EfiBootManagerConnectAll calls are
> extremely costly in terms of boot time and resources and should be avoided
> whenever feasible.
> 
> Therefore optimize BmExpandPartitionDevicePath to first search the existing
> BlockIo handles for a match. If a match is not found, then fallback to the
> original code to call EfiBootManagerConnectAll and search again. Thus, this
> optimization should be extremely low-risk given the fallback to previous
> behavior.
> 
> NOTE: The existing optimization in the code to use a "HDDP" variable to save
> the last matched device paths does not cover the first time a boot option is
> expanded (i.e. before the "HDDP" is created) nor when the device
> configuration has changed (resulting in the boot device moving to a different
> location in the PCI Bus/Dev hierarchy). This new optimization covers both of
> these cases on requisite platforms which explicity connect boot devices.
> 
> In our testing on OVMF/QEMU VMs with dozens of configured vnic devices,
> these extraneous calls to EfiBootManagerConnectAll from
> BmExpandPartitionDevicePath were found to cause many seconds (or even
> minutes) of additional VM boot time in some cases - due to the vnics being
> unnecessarily connected.
> 
> Cc: Zhichao Gao zhichao.gao@intel.com
> Cc: Ray Ni ray.ni@intel.com
> Signed-off-by: Aaron Young <aaron.young@oracle.com>
> ---
>  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 90
> ++++++++++++--------
>  1 file changed, 56 insertions(+), 34 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> index c3763c4483c7..7a97f7cdcc6b 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> @@ -880,6 +880,8 @@ BmExpandPartitionDevicePath (
>    BOOLEAN                   NeedAdjust;   EFI_DEVICE_PATH_PROTOCOL  *Instance;
> UINTN                     Size;+  BOOLEAN                   MatchFound;+  BOOLEAN
> ConnectAllAttempted;    //   // Check if there is prestore 'HDDP' variable.@@
> -974,49 +976,69 @@ BmExpandPartitionDevicePath (
>    // If we get here we fail to find or 'HDDP' not exist, and now we need   // to
> search all devices in the system for a matched partition   //-
> EfiBootManagerConnectAll ();-  Status = gBS->LocateHandleBuffer
> (ByProtocol, &gEfiBlockIoProtocolGuid, NULL, &BlockIoHandleCount,
> &BlockIoBuffer);-  if (EFI_ERROR (Status)) {-    BlockIoHandleCount = 0;-
> BlockIoBuffer      = NULL;-  }--  //-  // Loop through all the device handles that
> support the BLOCK_IO Protocol-  //-  for (Index = 0; Index <
> BlockIoHandleCount; Index++) {-    BlockIoDevicePath =
> DevicePathFromHandle (BlockIoBuffer[Index]);-    if (BlockIoDevicePath ==
> NULL) {-      continue;+  BlockIoBuffer       = NULL;+  MatchFound          =
> FALSE;+  ConnectAllAttempted = FALSE;+  do {+    if (BlockIoBuffer != NULL)
> {+      FreePool (BlockIoBuffer);     } -    if (BmMatchPartitionDevicePathNode
> (BlockIoDevicePath, (HARDDRIVE_DEVICE_PATH *)FilePath)) {-      //-      //
> Find the matched partition device path-      //-      TempDevicePath =
> AppendDevicePath (BlockIoDevicePath, NextDevicePathNode (FilePath));-
> FullPath       = BmGetNextLoadOptionDevicePath (TempDevicePath, NULL);-
> FreePool (TempDevicePath);+    Status = gBS->LocateHandleBuffer
> (ByProtocol, &gEfiBlockIoProtocolGuid, NULL, &BlockIoHandleCount,
> &BlockIoBuffer);+    if (EFI_ERROR (Status)) {+      BlockIoHandleCount = 0;+
> BlockIoBuffer      = NULL;+    } -      if (FullPath != NULL) {-
> BmCachePartitionDevicePath (&CachedDevicePath, BlockIoDevicePath);+
> //+    // Loop through all the device handles that support the BLOCK_IO
> Protocol+    //+    for (Index = 0; Index < BlockIoHandleCount; Index++) {+
> BlockIoDevicePath = DevicePathFromHandle (BlockIoBuffer[Index]);+      if
> (BlockIoDevicePath == NULL) {+        continue;+      } +      if
> (BmMatchPartitionDevicePathNode (BlockIoDevicePath,
> (HARDDRIVE_DEVICE_PATH *)FilePath)) {         //-        // Save the matching
> Device Path so we don't need to do a connect all next time-        // Failing to
> save only impacts performance next time expanding the short-form device
> path+        // Find the matched partition device path         //-        Status = gRT-
> >SetVariable (-                        L"HDDP",-
> &mBmHardDriveBootVariableGuid,-
> EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_NON_VOLATILE,-
> GetDevicePathSize (CachedDevicePath),-                        CachedDevicePath-
>                         );+        TempDevicePath = AppendDevicePath (BlockIoDevicePath,
> NextDevicePathNode (FilePath));+        FullPath       =
> BmGetNextLoadOptionDevicePath (TempDevicePath, NULL);+        FreePool
> (TempDevicePath);++        if (FullPath != NULL) {+
> BmCachePartitionDevicePath (&CachedDevicePath, BlockIoDevicePath); -
> break;+          //+          // Save the matching Device Path so we don't need to
> do a connect all next time+          // Failing to save only impacts performance
> next time expanding the short-form device path+          //+          Status = gRT-
> >SetVariable (+                          L"HDDP",+
> &mBmHardDriveBootVariableGuid,+
> EFI_VARIABLE_BOOTSERVICE_ACCESS |+
> EFI_VARIABLE_NON_VOLATILE,+                          GetDevicePathSize
> (CachedDevicePath),+                          CachedDevicePath+                          );+
> MatchFound = TRUE;+          break;+        }       }     }-  }++    //+    // If we found a
> matching BLOCK_IO handle or we've already+    // tried a ConnectAll, we are
> done searching.+    //+    if (MatchFound || ConnectAllAttempted) {+
> break;+    }++    EfiBootManagerConnectAll ();+    ConnectAllAttempted =
> TRUE;+  } while (1);    if (CachedDevicePath != NULL) {     FreePool
> (CachedDevicePath);--
> 2.39.3
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#109496):
> https://edk2.groups.io/g/devel/message/109496
> Mute This Topic: https://groups.io/mt/101876973/1768756
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [zhichao.gao@intel.com]
> -=-=-=-=-=-=
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109979): https://edk2.groups.io/g/devel/message/109979
Mute This Topic: https://groups.io/mt/101876973/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-