From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 41FFE941C3B for ; Tue, 10 Oct 2023 16:38:26 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=O31nBbaKc6XS6DEIG3pJjTEcDx6sgIc1q0tzHxsykSE=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1696955905; v=1; b=HMhDx3IbxUzMsY5KIyBtFlxGrDdbHKkfW07lIic0Fm6/EK9QCmfUtdphkBXPqW8vr+iD/m7h B7V2r37YPXc7a78Llsa2gF127ntIVmLuMghU6HxbupYTV9My6fPBwUuIUoBstoh7vEn56ENVhru azoFBmpB4aBdP4EyXNPKMcyE= X-Received: by 127.0.0.2 with SMTP id 0WslYY7687511xrqFidgzAwW; Tue, 10 Oct 2023 09:38:25 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.96294.1696955904304912296 for ; Tue, 10 Oct 2023 09:38:24 -0700 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-635-dv884MSSPv-cIsbVGtxJng-1; Tue, 10 Oct 2023 12:38:05 -0400 X-MC-Unique: dv884MSSPv-cIsbVGtxJng-1 X-Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B27E53C025B1; Tue, 10 Oct 2023 16:38:04 +0000 (UTC) X-Received: from [10.39.192.135] (unknown [10.39.192.135]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 24A376BC0; Tue, 10 Oct 2023 16:38:03 +0000 (UTC) Message-ID: <9348b3d2-eafb-ef88-9b2f-d70843afb428@redhat.com> Date: Tue, 10 Oct 2023 18:38:02 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath To: devel@edk2.groups.io, aaron.young@oracle.com References: <20231010150644.37857-1-Aaron.Young@oracle.com> From: "Laszlo Ersek" In-Reply-To: <20231010150644.37857-1-Aaron.Young@oracle.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: Se4mjXaw4xCw6G2I0CbDLwGMx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=HMhDx3Ib; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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 avoide= d > 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/PlatformBootMa= nagerLib/BdsPlatform.c] PlatformBdsConnectSequence() [OvmfPkg/Library/PlatformBootMa= nagerLib/BdsPlatform.c] ConnectDevicesFromQemu() [OvmfPkg/Library/QemuBootOrderL= ib/QemuBootOrderLib.c] ... EfiBootManagerRefreshAllBootOption() [MdeModulePkg/Library/UefiBootM= anagerLib/BmBoot.c] ... SetBootOrderFromQemu() [OvmfPkg/Library/QemuBootOrderL= ib/QemuBootOrderLib.c] Match() [OvmfPkg/Library/QemuBootOrderL= ib/QemuBootOrderLib.c] EfiBootManagerGetLoadOptionBuffer() [MdeModulePkg/Library/UefiBootM= anagerLib/BmBoot.c] BmGetNextLoadOptionBuffer() [MdeModulePkg/Library/UefiBootM= anagerLib/BmLoadOption.c] BmGetNextLoadOptionDevicePath() [MdeModulePkg/Library/UefiBootM= anagerLib/BmBoot.c] BmExpandPartitionDevicePath() [MdeModulePkg/Library/UefiBootM= anagerLib/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 =3D EfiBootManagerGetNextLoadOptionDevicePath ( DevicePath, NULL ); from Match(), instead of FileBuffer =3D 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 devic= e > 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 > --- > MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 90 ++++++++++++------= -- > 1 file changed, 56 insertions(+), 34 deletions(-) > > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModule= Pkg/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 =3D gBS->LocateHandleBuffer (ByProtocol, &gEfiBlockIoProtocolGu= id, NULL, &BlockIoHandleCount, &BlockIoBuffer); > - if (EFI_ERROR (Status)) { > - BlockIoHandleCount =3D 0; > - BlockIoBuffer =3D NULL; > - } > - > - // > - // Loop through all the device handles that support the BLOCK_IO Proto= col > - // > - for (Index =3D 0; Index < BlockIoHandleCount; Index++) { > - BlockIoDevicePath =3D DevicePathFromHandle (BlockIoBuffer[Index]); > - if (BlockIoDevicePath =3D=3D NULL) { > - continue; > + BlockIoBuffer =3D NULL; > + MatchFound =3D FALSE; > + ConnectAllAttempted =3D FALSE; > + do { > + if (BlockIoBuffer !=3D NULL) { > + FreePool (BlockIoBuffer); > } > > - if (BmMatchPartitionDevicePathNode (BlockIoDevicePath, (HARDDRIVE_DE= VICE_PATH *)FilePath)) { > - // > - // Find the matched partition device path > - // > - TempDevicePath =3D AppendDevicePath (BlockIoDevicePath, NextDevice= PathNode (FilePath)); > - FullPath =3D BmGetNextLoadOptionDevicePath (TempDevicePath, = NULL); > - FreePool (TempDevicePath); > + Status =3D gBS->LocateHandleBuffer (ByProtocol, &gEfiBlockIoProtocol= Guid, NULL, &BlockIoHandleCount, &BlockIoBuffer); > + if (EFI_ERROR (Status)) { > + BlockIoHandleCount =3D 0; > + BlockIoBuffer =3D NULL; > + } > > - if (FullPath !=3D NULL) { > - BmCachePartitionDevicePath (&CachedDevicePath, BlockIoDevicePath= ); > + // > + // Loop through all the device handles that support the BLOCK_IO Pro= tocol > + // > + for (Index =3D 0; Index < BlockIoHandleCount; Index++) { > + BlockIoDevicePath =3D DevicePathFromHandle (BlockIoBuffer[Index]); > + if (BlockIoDevicePath =3D=3D NULL) { > + continue; > + } > > + if (BmMatchPartitionDevicePathNode (BlockIoDevicePath, (HARDDRIVE_= DEVICE_PATH *)FilePath)) { > // > - // Save the matching Device Path so we don't need to do a connec= t all next time > - // Failing to save only impacts performance next time expanding = the short-form device path > + // Find the matched partition device path > // > - Status =3D gRT->SetVariable ( > - L"HDDP", > - &mBmHardDriveBootVariableGuid, > - EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_N= ON_VOLATILE, > - GetDevicePathSize (CachedDevicePath), > - CachedDevicePath > - ); > + TempDevicePath =3D AppendDevicePath (BlockIoDevicePath, NextDevi= cePathNode (FilePath)); > + FullPath =3D BmGetNextLoadOptionDevicePath (TempDevicePath= , NULL); > + FreePool (TempDevicePath); > + > + if (FullPath !=3D NULL) { > + BmCachePartitionDevicePath (&CachedDevicePath, BlockIoDevicePa= th); > > - break; > + // > + // Save the matching Device Path so we don't need to do a conn= ect all next time > + // Failing to save only impacts performance next time expandin= g the short-form device path > + // > + Status =3D gRT->SetVariable ( > + L"HDDP", > + &mBmHardDriveBootVariableGuid, > + EFI_VARIABLE_BOOTSERVICE_ACCESS | > + EFI_VARIABLE_NON_VOLATILE, > + GetDevicePathSize (CachedDevicePath), > + CachedDevicePath > + ); > + MatchFound =3D 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 =3D TRUE; > + } while (1); > > if (CachedDevicePath !=3D NULL) { > FreePool (CachedDevicePath); -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-