>-----Original Message-----
>From: Bi, Dandan
>Sent: Monday, June 17, 2019 12:47 PM
>To: Wu, Hao A <hao.a...@intel.com>; Gao, Zhichao
><zhichao....@intel.com>; devel@edk2.groups.io; Gao, Liming
><liming....@intel.com>; Ni, Ray <ray...@intel.com>
>Cc: Bret Barkelew <bret.barke...@microsoft.com>; Wang, Jian J
><jian.j.w...@intel.com>; Zeng, Star <star.z...@intel.com>
>Subject: RE: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change
>performance code
>
>> -----Original Message-----
>> From: Wu, Hao A
>> Sent: Friday, June 14, 2019 4:44 PM
>> To: Gao, Zhichao <zhichao....@intel.com>; devel@edk2.groups.io; Bi,
>> Dandan <dandan...@intel.com>; Gao, Liming <liming....@intel.com>; Ni,
>> Ray <ray...@intel.com>
>> Cc: Bret Barkelew <bret.barke...@microsoft.com>; Wang, Jian J
>> <jian.j.w...@intel.com>; Zeng, Star <star.z...@intel.com>
>> Subject: RE: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change
>> performance code
>>
>> > -----Original Message-----
>> > From: Gao, Zhichao
>> > Sent: Monday, June 10, 2019 3:29 PM
>> > To: devel@edk2.groups.io
>> > Cc: Bret Barkelew; Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao,
>> > Liming
>> > Subject: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change
>> > performance code
>> >
>> > From: Bret Barkelew <bret.barke...@microsoft.com>
>> >
>> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888
>> >
>> > Use PERF_INMODULE_BEGIN and PERF_INMODULE_END to replace
>> > PERF_START_EX, PERF_CODE and PERF_END_EX.
>>
>>
>> Hello Dandan & Liming,
>>
>> May I know the reason for 'PERF_START_EX' & 'PERF_END_EX' macros are
>> not
>> being replaced in commit:
>>
>> Revision: 67e9ab84ef042bd59c4297fdad7f6aece6b7bbca
>> MdeModulePkg: Use new added Perf macros
>>
>> Is there a special reason for this?
>> ('OptionNumber' as the identifier?)
>
>Hi Hao and Zhichao
>
>The 'PERF_START_EX' & 'PERF_END_EX' here specifies 'OptionNumber' as the
>identifier. We will miss the information of 'OptionNumber' if we replace it
>with
>new macros. It may impact current usage model. That's why we kept it before.
>For other 'PERF_START_EX' & 'PERF_END_EX' replacements in this patch
>series may have the similar issue.
>
>Zhichao, please hold the patches that replace 'PERF_START_EX' &
>'PERF_END_EX' firstly, I think it may need more discussion.
>
>Liming, do you have any comments for this?
>
Yes. I don't think the replacement is good. I suggest to keep current, and add
new ones. I add my comments below.
>
>Thanks,
>Dandan
>>
>>
>> > Use PERF_CROSSMODULE_END and PERF_CROSSMODULE_BEGIN to get
>> the
>> > info
>> > of one boot image's performance.
>>
>>
>> Hello Zhichao,
>>
>> May I know what kind of test has been done for this patch?
>> Also, some inline comments below:
>>
>>
>> >
>> > Cc: Jian J Wang <jian.j.w...@intel.com>
>> > Cc: Hao A Wu <hao.a...@intel.com>
>> > Cc: Ray Ni <ray...@intel.com>
>> > Cc: Star Zeng <star.z...@intel.com>
>> > Cc: Liming Gao <liming....@intel.com>
>> > Signed-off-by: Zhichao Gao <zhichao....@intel.com>
>> > ---
>> > MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 9 ++++-----
>> > 1 file changed, 4 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> > b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> > index 952033fc82..af1024cacd 100644
>> > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> > @@ -1812,7 +1812,7 @@ EfiBootManagerBoot (
>> > BmRepairAllControllers (0);
>> > }
>> >
>> > - PERF_START_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
>> > OptionNumber);
>> > + PERF_INMODULE_BEGIN ("BdsAttempt");
>> >
>> > //
>> > // 5. Adjust the different type memory page number just before booting
>> > @@ -1932,9 +1932,9 @@ EfiBootManagerBoot (
>> > //
>> > // Write boot to OS performance data for UEFI boot
>> > //
>> > - PERF_CODE (
>> > - BmEndOfBdsPerfCode (NULL, NULL);
>> > - );
Seemly, this change is not related to this patch. This code is unused any more.
I agree to remove it in another separate patch.
Thanks
Liming
>> > + PERF_INMODULE_END ("BdsAttempt");
>>
>>
>> I think the patch missed to replace the below 'PERF_END_EX' macro:
>>
>> //
>> if ((DevicePathType (BootOption->FilePath) == BBS_DEVICE_PATH) &&
>> (DevicePathSubType (BootOption->FilePath) == BBS_BBS_DP)) {
>> ...
>>
>> PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
>> OptionNumber);
>> ^^^^^^^^^^^
>> return;
>> }
>>
>>
>> > + PERF_CROSSMODULE_END ("BDS");
>> > + PERF_CROSSMODULE_BEGIN ("BDS");
>>
>>
>> Could you help to introduce the purpose for the above
>> 'PERF_CROSSMODULE_BEGIN' in more detail?
>>
>>
>> >
>> > REPORT_STATUS_CODE (EFI_PROGRESS_CODE, PcdGet32
>> > (PcdProgressCodeOsLoaderStart));
>> >
>> > @@ -1947,7 +1947,6 @@ EfiBootManagerBoot (
>> > //
>> > BmReportLoadFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED,
>> > Status);
>> > }
>> > - PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
>> > OptionNumber);
>>
>>
>> The patch excludes the time consumed by StartImage() from performance
>> data
>> for the "BdsAttempt" token.
>>
>> If the image starts successfully, there will not be an matching PERF_END
>> macro for the origin code.
>>
>> Ray, do you think it is a reasonable change here?
>>
>> Best Regards,
>> Hao Wu
>>
>>
>> >
>> > //
>> > // Destroy the RAM disk
>> > --
>> > 2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#42533): https://edk2.groups.io/g/devel/message/42533
Mute This Topic: https://groups.io/mt/32001828/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
