From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.136, mailfrom: dandan.bi@intel.com) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by groups.io with SMTP; Sun, 16 Jun 2019 21:47:19 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Jun 2019 21:47:18 -0700 X-ExtLoop1: 1 Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga006.fm.intel.com with ESMTP; 16 Jun 2019 21:47:18 -0700 Received: from fmsmsx163.amr.corp.intel.com (10.18.125.72) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 16 Jun 2019 21:47:18 -0700 Received: from shsmsx107.ccr.corp.intel.com (10.239.4.96) by fmsmsx163.amr.corp.intel.com (10.18.125.72) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 16 Jun 2019 21:47:18 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.185]) by SHSMSX107.ccr.corp.intel.com ([169.254.9.173]) with mapi id 14.03.0439.000; Mon, 17 Jun 2019 12:47:16 +0800 From: "Dandan Bi" To: "Wu, Hao A" , "Gao, Zhichao" , "devel@edk2.groups.io" , "Gao, Liming" , "Ni, Ray" CC: Bret Barkelew , "Wang, Jian J" , "Zeng, Star" Subject: Re: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change performance code Thread-Topic: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change performance code Thread-Index: AQHVIo1BhaCB2r7ZqkWMfnb3V1OuTqafOPZA Date: Mon, 17 Jun 2019 04:47:16 +0000 Message-ID: <3C0D5C461C9E904E8F62152F6274C0BB40C03720@SHSMSX104.ccr.corp.intel.com> References: <20190610072855.2800-1-zhichao.gao@intel.com> <20190610072855.2800-4-zhichao.gao@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: dandan.bi@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Wu, Hao A > Sent: Friday, June 14, 2019 4:44 PM > To: Gao, Zhichao ; devel@edk2.groups.io; Bi, > Dandan ; Gao, Liming ; Ni, > Ray > Cc: Bret Barkelew ; Wang, Jian J > ; Zeng, Star > Subject: RE: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change > performance code >=20 > > -----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 > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1888 > > > > Use PERF_INMODULE_BEGIN and PERF_INMODULE_END to replace > > PERF_START_EX, PERF_CODE and PERF_END_EX. >=20 >=20 > Hello Dandan & Liming, >=20 > May I know the reason for 'PERF_START_EX' & 'PERF_END_EX' macros are > not > being replaced in commit: >=20 > Revision: 67e9ab84ef042bd59c4297fdad7f6aece6b7bbca > MdeModulePkg: Use new added Perf macros >=20 > 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 i= dentifier. 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 b= efore. For other 'PERF_START_EX' & 'PERF_END_EX' replacements in this patch serie= s may have the similar issue. Zhichao, please hold the patches that replace 'PERF_START_EX' & 'PERF_END_E= X' firstly, I think it may need more discussion. Liming, do you have any comments for this? Thanks, Dandan >=20 >=20 > > Use PERF_CROSSMODULE_END and PERF_CROSSMODULE_BEGIN to get > the > > info > > of one boot image's performance. >=20 >=20 > Hello Zhichao, >=20 > May I know what kind of test has been done for this patch? > Also, some inline comments below: >=20 >=20 > > > > Cc: Jian J Wang > > Cc: Hao A Wu > > Cc: Ray Ni > > Cc: Star Zeng > > Cc: Liming Gao > > Signed-off-by: Zhichao Gao > > --- > > 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 booti= ng > > @@ -1932,9 +1932,9 @@ EfiBootManagerBoot ( > > // > > // Write boot to OS performance data for UEFI boot > > // > > - PERF_CODE ( > > - BmEndOfBdsPerfCode (NULL, NULL); > > - ); > > + PERF_INMODULE_END ("BdsAttempt"); >=20 >=20 > I think the patch missed to replace the below 'PERF_END_EX' macro: >=20 > // > if ((DevicePathType (BootOption->FilePath) =3D=3D BBS_DEVICE_PATH) && > (DevicePathSubType (BootOption->FilePath) =3D=3D BBS_BBS_DP)) { > ... >=20 > PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) > OptionNumber); > ^^^^^^^^^^^ > return; > } >=20 >=20 > > + PERF_CROSSMODULE_END ("BDS"); > > + PERF_CROSSMODULE_BEGIN ("BDS"); >=20 >=20 > Could you help to introduce the purpose for the above > 'PERF_CROSSMODULE_BEGIN' in more detail? >=20 >=20 > > > > 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); >=20 >=20 > The patch excludes the time consumed by StartImage() from performance > data > for the "BdsAttempt" token. >=20 > If the image starts successfully, there will not be an matching PERF_END > macro for the origin code. >=20 > Ray, do you think it is a reasonable change here? >=20 > Best Regards, > Hao Wu >=20 >=20 > > > > // > > // Destroy the RAM disk > > -- > > 2.21.0.windows.1