[edk2-devel] [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change performance code

[Dandan Bi]

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42479): https://edk2.groups.io/g/devel/message/42479
Mute This Topic: https://groups.io/mt/32001828/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-