* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath
2023-10-10 15:06 [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath Aaron Young
@ 2023-10-10 16:38 ` Laszlo Ersek
2023-10-11 17:36 ` Aaron Young via groups.io
2023-10-24 2:18 ` Gao, Zhichao
1 sibling, 1 reply; 7+ messages in thread
From: Laszlo Ersek @ 2023-10-10 16:38 UTC (permalink / raw)
To: devel, aaron.young
On 10/10/23 17:06, Aaron Young wrote:
> 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.
I'm not going to look at the code part for now, but this sounds like a
very good idea.
For reference, the call tree is as follows:
PlatformBootManagerAfterConsole() [OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c]
PlatformBdsConnectSequence() [OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c]
ConnectDevicesFromQemu() [OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c]
...
EfiBootManagerRefreshAllBootOption() [MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c]
...
SetBootOrderFromQemu() [OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c]
Match() [OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c]
EfiBootManagerGetLoadOptionBuffer() [MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c]
BmGetNextLoadOptionBuffer() [MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c]
BmGetNextLoadOptionDevicePath() [MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c]
BmExpandPartitionDevicePath() [MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c]
Perhaps capture this in the commit message, if you agree it's useful.
An *additional* pain point for me has been the need to call
EfiBootManagerGetLoadOptionBuffer() from Match() -- because
EfiBootManagerGetLoadOptionBuffer() used to be the *only*
UefiBootManagerLib API that exposed the short-form completion logic.
EfiBootManagerGetLoadOptionBuffer() is suboptimal because it doesn't
only complete short-form device paths, but it also loads the referenced
file -- which is totally unnecessary for Match(), so we release the
lodaded buffer immediately.
I *think* what we're actually after is the
BmGetNextLoadOptionDevicePath() function -- but it is not exposed by the
UefiBootManagerLib class.
However, it seems that in commits b4e1ad87d0ee
("MdeModulePkg/CapsuleApp: Add a function used to get next DevicePath",
2019-01-31) and 68a4e15e1497 ("MdeModulePkg: Rename confusion function
name", 2019-02-25), BmGetNextLoadOptionDevicePath() got effectively
exposed as EfiBootManagerGetNextLoadOptionDevicePath().
And now I'm thinking, perhaps we could get away with calling
AbsDevicePath = EfiBootManagerGetNextLoadOptionDevicePath (
DevicePath,
NULL
);
from Match(), instead of
FileBuffer = EfiBootManagerGetLoadOptionBuffer (
DevicePath,
&AbsDevicePath,
&FileSize
);
? That would stop us from needlessly loading the file contents!
Can you investigate that as well, perhaps? :)
(The second parameter of EfiBootManagerGetNextLoadOptionDevicePath()
only matters if we are interested in multiple expansions of the same
short-form device path. If we pass a non-NULL device path in the second
parameter, then EfiBootManagerGetNextLoadOptionDevicePath() will first
locate *that* absoute device path in the possible resolutions, and
return the *next* one after that. It's pretty ineffective, but it lets
the caller cycle through all potential absolute resolutions for the same
short path. BmGetNextLoadOptionBuffer() contains such a loop / iteration
internally, but I'm not sure if we need that. Hmmm... there's an example
in "MdeModulePkg/Application/CapsuleApp/CapsuleOnDisk.c", but that call
site is also wrapped in a loop! So EfiBootManagerGetLoadOptionBuffer()
seems *safer* after all...)
Thanks!
Laszlo
>
> 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);
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109501): https://edk2.groups.io/g/devel/message/109501
Mute This Topic: https://groups.io/mt/101876973/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath
2023-10-10 15:06 [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath Aaron Young
2023-10-10 16:38 ` Laszlo Ersek
@ 2023-10-24 2:18 ` Gao, Zhichao
2023-10-24 12:29 ` Laszlo Ersek
1 sibling, 1 reply; 7+ messages in thread
From: Gao, Zhichao @ 2023-10-24 2:18 UTC (permalink / raw)
To: devel@edk2.groups.io, aaron.young@oracle.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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 7+ messages in thread