Re: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change performance code

["Liming Gao" <liming.gao@intel.com>]

>-----Original Message-----
>From: Bi, Dandan
>Sent: Monday, June 17, 2019 12:47 PM
>To: Wu, Hao A <hao.a.wu@intel.com>; Gao, Zhichao
><zhichao.gao@intel.com>; devel@edk2.groups.io; Gao, Liming
><liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>
>Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J
><jian.j.wang@intel.com>; Zeng, Star <star.zeng@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.gao@intel.com>; devel@edk2.groups.io; Bi,
>> Dandan <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>; Ni,
>> Ray <ray.ni@intel.com>
>> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J
>> <jian.j.wang@intel.com>; Zeng, Star <star.zeng@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.Barkelew@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.wang@intel.com>
>> > Cc: Hao A Wu <hao.a.wu@intel.com>
>> > Cc: Ray Ni <ray.ni@intel.com>
>> > Cc: Star Zeng <star.zeng@intel.com>
>> > Cc: Liming Gao <liming.gao@intel.com>
>> > Signed-off-by: Zhichao Gao <zhichao.gao@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