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

["Dandan Bi" <dandan.bi@intel.com>]

> -----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?


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);
> > -  );
> > +  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