From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, aaron.young@oracle.com
Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath
Date: Tue, 10 Oct 2023 18:38:02 +0200 [thread overview]
Message-ID: <9348b3d2-eafb-ef88-9b2f-d70843afb428@redhat.com> (raw)
In-Reply-To: <20231010150644.37857-1-Aaron.Young@oracle.com>
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]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2023-10-10 16:38 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-10 15:06 [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath Aaron Young
2023-10-10 16:38 ` Laszlo Ersek [this message]
2023-10-11 17:36 ` Aaron Young via groups.io
2023-10-12 15:14 ` Laszlo Ersek
2023-10-24 2:18 ` Gao, Zhichao
2023-10-24 12:29 ` Laszlo Ersek
2023-10-24 13:15 ` Laszlo Ersek
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=9348b3d2-eafb-ef88-9b2f-d70843afb428@redhat.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