* [PATCH 1/6] MdeModulePkg/DxeMain: Add performance code
2019-06-10 7:28 [PATCH 0/6] Change the performance code Gao, Zhichao
@ 2019-06-10 7:28 ` Gao, Zhichao
2019-06-14 8:43 ` Wu, Hao A
2019-06-10 7:28 ` [PATCH 2/6] MdeModule/PeiMain: " Gao, Zhichao
` (4 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Gao, Zhichao @ 2019-06-10 7:28 UTC (permalink / raw)
To: devel
Cc: Bret Barkelew, Jian J Wang, Hao A Wu, Ray Ni, Star Zeng,
Liming Gao, Sean Brogan, Michael Turner
From: Bret Barkelew <Bret.Barkelew@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888
Add missing PERF_FUNCTION_END before the return statement.
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>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c b/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
index c310b40b52..001a26aa62 100644
--- a/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
@@ -418,6 +418,7 @@ CoreDispatcher (
//
// If the dispatcher is running don't let it be restarted.
//
+ PERF_FUNCTION_END ();
return EFI_ALREADY_STARTED;
}
@@ -432,6 +433,7 @@ CoreDispatcher (
&DxeDispatchEvent
);
if (EFI_ERROR (Status)) {
+ PERF_FUNCTION_END ();
return Status;
}
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/6] MdeModulePkg/DxeMain: Add performance code
2019-06-10 7:28 ` [PATCH 1/6] MdeModulePkg/DxeMain: Add " Gao, Zhichao
@ 2019-06-14 8:43 ` Wu, Hao A
0 siblings, 0 replies; 12+ messages in thread
From: Wu, Hao A @ 2019-06-14 8:43 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Bret Barkelew, Wang, Jian J, Ni, Ray, Zeng, Star, Gao, Liming,
Sean Brogan, Michael Turner
> -----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;
> Sean Brogan; Michael Turner
> Subject: [PATCH 1/6] MdeModulePkg/DxeMain: Add performance code
I suggest to change the title to:
MdeModulePkg/DxeMain: Add missing PERF_FUNCTION_END for error paths
With the change,
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
Best Regards,
Hao Wu
>
> From: Bret Barkelew <Bret.Barkelew@microsoft.com>
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888
>
> Add missing PERF_FUNCTION_END before the return statement.
>
> 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>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Turner <Michael.Turner@microsoft.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
> MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
> b/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
> index c310b40b52..001a26aa62 100644
> --- a/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
> +++ b/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
> @@ -418,6 +418,7 @@ CoreDispatcher (
> //
> // If the dispatcher is running don't let it be restarted.
> //
> + PERF_FUNCTION_END ();
> return EFI_ALREADY_STARTED;
> }
>
> @@ -432,6 +433,7 @@ CoreDispatcher (
> &DxeDispatchEvent
> );
> if (EFI_ERROR (Status)) {
> + PERF_FUNCTION_END ();
> return Status;
> }
>
> --
> 2.21.0.windows.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/6] MdeModule/PeiMain: Add performance code
2019-06-10 7:28 [PATCH 0/6] Change the performance code Gao, Zhichao
2019-06-10 7:28 ` [PATCH 1/6] MdeModulePkg/DxeMain: Add " Gao, Zhichao
@ 2019-06-10 7:28 ` Gao, Zhichao
2019-06-14 8:43 ` Wu, Hao A
2019-06-10 7:28 ` [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change " Gao, Zhichao
` (3 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Gao, Zhichao @ 2019-06-10 7:28 UTC (permalink / raw)
To: devel; +Cc: Bret Barkelew, Jian J Wang, Hao A Wu, Ray Ni, Star Zeng,
Liming Gao
From: Bret Barkelew <Bret.Barkelew@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888
Add performance code for PeiDisaptcher function and Image function.
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/Core/Pei/Dispatcher/Dispatcher.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
index ba2fd0cae1..0caffe653b 100644
--- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
@@ -983,6 +983,8 @@ PeiDispatcher (
EFI_FV_FILE_INFO FvFileInfo;
PEI_CORE_FV_HANDLE *CoreFvHandle;
+ PERF_FUNCTION_BEGIN ();
+
PeiServices = (CONST EFI_PEI_SERVICES **) &Private->Ps;
PeimEntryPoint = NULL;
PeimFileHandle = NULL;
@@ -1004,6 +1006,7 @@ PeiDispatcher (
Private->CurrentFileHandle = PeimFileHandle;
Private->CurrentPeimFvCount = Index1;
Private->CurrentPeimCount = Index2;
+ PERF_LOAD_IMAGE_BEGIN (NULL);
Status = PeiLoadImage (
(CONST EFI_PEI_SERVICES **) &Private->Ps,
PeimFileHandle,
@@ -1012,6 +1015,7 @@ PeiDispatcher (
&AuthenticationState
);
if (Status == EFI_SUCCESS) {
+ PERF_LOAD_IMAGE_END (PeimFileHandle);
//
// PEIM_STATE_REGISTER_FOR_SHADOW move to PEIM_STATE_DONE
//
@@ -1118,6 +1122,7 @@ PeiDispatcher (
//
// For PEIM driver, Load its entry point
//
+ PERF_LOAD_IMAGE_BEGIN (NULL);
Status = PeiLoadImage (
PeiServices,
PeimFileHandle,
@@ -1126,6 +1131,7 @@ PeiDispatcher (
&AuthenticationState
);
if (Status == EFI_SUCCESS) {
+ PERF_LOAD_IMAGE_END (PeimFileHandle);
//
// The PEIM has its dependencies satisfied, and its entry point
// has been found, so invoke it.
@@ -1197,6 +1203,7 @@ PeiDispatcher (
//
// Load PEIM into Memory for Register for shadow PEIM.
//
+ PERF_LOAD_IMAGE_BEGIN (NULL);
Status = PeiLoadImage (
PeiServices,
PeimFileHandle,
@@ -1205,6 +1212,7 @@ PeiDispatcher (
&AuthenticationState
);
if (Status == EFI_SUCCESS) {
+ PERF_LOAD_IMAGE_END (PeimFileHandle);
PeimEntryPoint = (EFI_PEIM_ENTRY_POINT2)(UINTN)EntryPoint;
}
}
@@ -1252,6 +1260,8 @@ PeiDispatcher (
//
} while (Private->PeimNeedingDispatch && Private->PeimDispatchOnThisPass);
+ PERF_FUNCTION_END ();
+
}
/**
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/6] MdeModule/PeiMain: Add performance code
2019-06-10 7:28 ` [PATCH 2/6] MdeModule/PeiMain: " Gao, Zhichao
@ 2019-06-14 8:43 ` Wu, Hao A
0 siblings, 0 replies; 12+ messages in thread
From: Wu, Hao A @ 2019-06-14 8:43 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Bret Barkelew, Wang, Jian J, Ni, Ray, Zeng, Star, Gao, Liming
> -----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 2/6] MdeModule/PeiMain: Add performance code
>
> From: Bret Barkelew <Bret.Barkelew@microsoft.com>
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888
>
> Add performance code for PeiDisaptcher function and Image function.
>
> 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/Core/Pei/Dispatcher/Dispatcher.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> index ba2fd0cae1..0caffe653b 100644
> --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> @@ -983,6 +983,8 @@ PeiDispatcher (
> EFI_FV_FILE_INFO FvFileInfo;
> PEI_CORE_FV_HANDLE *CoreFvHandle;
>
> + PERF_FUNCTION_BEGIN ();
> +
> PeiServices = (CONST EFI_PEI_SERVICES **) &Private->Ps;
> PeimEntryPoint = NULL;
> PeimFileHandle = NULL;
> @@ -1004,6 +1006,7 @@ PeiDispatcher (
> Private->CurrentFileHandle = PeimFileHandle;
> Private->CurrentPeimFvCount = Index1;
> Private->CurrentPeimCount = Index2;
> + PERF_LOAD_IMAGE_BEGIN (NULL);
> Status = PeiLoadImage (
> (CONST EFI_PEI_SERVICES **) &Private->Ps,
> PeimFileHandle,
> @@ -1012,6 +1015,7 @@ PeiDispatcher (
> &AuthenticationState
> );
> if (Status == EFI_SUCCESS) {
> + PERF_LOAD_IMAGE_END (PeimFileHandle);
One question, what is the reason for the 'PERF_LOAD_IMAGE_END' macro being
only added when PeiLoadImage() returns with no error?
Best Regards,
Hao Wu
> //
> // PEIM_STATE_REGISTER_FOR_SHADOW move to
> PEIM_STATE_DONE
> //
> @@ -1118,6 +1122,7 @@ PeiDispatcher (
> //
> // For PEIM driver, Load its entry point
> //
> + PERF_LOAD_IMAGE_BEGIN (NULL);
> Status = PeiLoadImage (
> PeiServices,
> PeimFileHandle,
> @@ -1126,6 +1131,7 @@ PeiDispatcher (
> &AuthenticationState
> );
> if (Status == EFI_SUCCESS) {
> + PERF_LOAD_IMAGE_END (PeimFileHandle);
> //
> // The PEIM has its dependencies satisfied, and its entry point
> // has been found, so invoke it.
> @@ -1197,6 +1203,7 @@ PeiDispatcher (
> //
> // Load PEIM into Memory for Register for shadow PEIM.
> //
> + PERF_LOAD_IMAGE_BEGIN (NULL);
> Status = PeiLoadImage (
> PeiServices,
> PeimFileHandle,
> @@ -1205,6 +1212,7 @@ PeiDispatcher (
> &AuthenticationState
> );
> if (Status == EFI_SUCCESS) {
> + PERF_LOAD_IMAGE_END (PeimFileHandle);
> PeimEntryPoint = (EFI_PEIM_ENTRY_POINT2)(UINTN)EntryPoint;
> }
> }
> @@ -1252,6 +1260,8 @@ PeiDispatcher (
> //
> } while (Private->PeimNeedingDispatch && Private-
> >PeimDispatchOnThisPass);
>
> + PERF_FUNCTION_END ();
> +
> }
>
> /**
> --
> 2.21.0.windows.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change performance code
2019-06-10 7:28 [PATCH 0/6] Change the performance code Gao, Zhichao
2019-06-10 7:28 ` [PATCH 1/6] MdeModulePkg/DxeMain: Add " Gao, Zhichao
2019-06-10 7:28 ` [PATCH 2/6] MdeModule/PeiMain: " Gao, Zhichao
@ 2019-06-10 7:28 ` Gao, Zhichao
2019-06-14 8:43 ` Wu, Hao A
2019-06-10 7:28 ` [PATCH 4/6] SecurityPkg/Tcg2Dxe: " Gao, Zhichao
` (2 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Gao, Zhichao @ 2019-06-10 7:28 UTC (permalink / raw)
To: devel; +Cc: Bret Barkelew, Jian J Wang, Hao A Wu, Ray Ni, Star Zeng,
Liming Gao
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.
Use PERF_CROSSMODULE_END and PERF_CROSSMODULE_BEGIN to get the info
of one boot image's performance.
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");
+ PERF_CROSSMODULE_END ("BDS");
+ PERF_CROSSMODULE_BEGIN ("BDS");
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);
//
// Destroy the RAM disk
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change performance code
2019-06-10 7:28 ` [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change " Gao, Zhichao
@ 2019-06-14 8:43 ` Wu, Hao A
2019-06-17 4:47 ` Dandan Bi
0 siblings, 1 reply; 12+ messages in thread
From: Wu, Hao A @ 2019-06-14 8:43 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io, Bi, Dandan, Gao, Liming,
Ni, Ray
Cc: Bret Barkelew, Wang, Jian J, Zeng, Star
> -----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?)
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change performance code
2019-06-14 8:43 ` Wu, Hao A
@ 2019-06-17 4:47 ` Dandan Bi
2019-06-18 5:51 ` Liming Gao
0 siblings, 1 reply; 12+ messages in thread
From: Dandan Bi @ 2019-06-17 4:47 UTC (permalink / raw)
To: Wu, Hao A, Gao, Zhichao, devel@edk2.groups.io, Gao, Liming,
Ni, Ray
Cc: Bret Barkelew, Wang, Jian J, Zeng, Star
> -----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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change performance code
2019-06-17 4:47 ` Dandan Bi
@ 2019-06-18 5:51 ` Liming Gao
0 siblings, 0 replies; 12+ messages in thread
From: Liming Gao @ 2019-06-18 5:51 UTC (permalink / raw)
To: Bi, Dandan, Wu, Hao A, Gao, Zhichao, devel@edk2.groups.io,
Ni, Ray
Cc: Bret Barkelew, Wang, Jian J, Zeng, Star
>-----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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 4/6] SecurityPkg/Tcg2Dxe: Change performance code
2019-06-10 7:28 [PATCH 0/6] Change the performance code Gao, Zhichao
` (2 preceding siblings ...)
2019-06-10 7:28 ` [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change " Gao, Zhichao
@ 2019-06-10 7:28 ` Gao, Zhichao
2019-06-10 7:28 ` [PATCH 5/6] SecurityPkg/Tcg2Pei: " Gao, Zhichao
2019-06-10 7:28 ` [PATCH 6/6] IntelSiliconPkg/IntelVtdDxe: Change the " Gao, Zhichao
5 siblings, 0 replies; 12+ messages in thread
From: Gao, Zhichao @ 2019-06-10 7:28 UTC (permalink / raw)
To: devel; +Cc: Bret Barkelew, Chao Zhang, Jiewen Yao, Jian Wang
From: Bret Barkelew <Bret.Barkelew@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888
Use PERF_FUNCTION_BEGIN and PERF_FUNCTION_END to replace
PERF_START_EX and PERF_FUNCTION_END.
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian Wang <jian.j.wang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c b/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c
index a2729457b7..001e0aac4d 100644
--- a/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c
+++ b/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c
@@ -45,8 +45,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/ReportStatusCodeLib.h>
#include <Library/Tcg2PhysicalPresenceLib.h>
-#define PERF_ID_TCG2_DXE 0x3120
-
typedef struct {
CHAR16 *VariableName;
EFI_GUID *VendorGuid;
@@ -2250,7 +2248,7 @@ OnReadyToBoot (
EFI_STATUS Status;
TPM_PCRINDEX PcrIndex;
- PERF_START_EX (mImageHandle, "EventRec", "Tcg2Dxe", 0, PERF_ID_TCG2_DXE);
+ PERF_FUNCTION_BEGIN ();
if (mBootAttempts == 0) {
//
@@ -2332,7 +2330,7 @@ OnReadyToBoot (
// Increase boot attempt counter.
//
mBootAttempts++;
- PERF_END_EX (mImageHandle, "EventRec", "Tcg2Dxe", 0, PERF_ID_TCG2_DXE + 1);
+ PERF_FUNCTION_END ();
}
/**
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 5/6] SecurityPkg/Tcg2Pei: Change performance code
2019-06-10 7:28 [PATCH 0/6] Change the performance code Gao, Zhichao
` (3 preceding siblings ...)
2019-06-10 7:28 ` [PATCH 4/6] SecurityPkg/Tcg2Dxe: " Gao, Zhichao
@ 2019-06-10 7:28 ` Gao, Zhichao
2019-06-10 7:28 ` [PATCH 6/6] IntelSiliconPkg/IntelVtdDxe: Change the " Gao, Zhichao
5 siblings, 0 replies; 12+ messages in thread
From: Gao, Zhichao @ 2019-06-10 7:28 UTC (permalink / raw)
To: devel; +Cc: Bret Barkelew, Chao Zhang, Jiewen Yao, Jian Wang
From: Bret Barkelew <Bret.Barkelew@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888
Use PERF_FUNCTION_BEGIN and PERF_FUNCTION_END to replace
PERF_START_EX and PERF_FUNCTION_END.
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian Wang <jian.j.wang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
index f28f33fdec..4b5388726c 100644
--- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
+++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
@@ -1,7 +1,7 @@
/** @file
Initialize TPM2 device and measure FVs before handing off control to DXE.
-Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
Copyright (c) 2017, Microsoft Corporation. All rights reserved. <BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -38,8 +38,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/ReportStatusCodeLib.h>
#include <Library/ResetSystemLib.h>
-#define PERF_ID_TCG2_PEI 0x3080
-
typedef struct {
EFI_GUID *EventGuid;
EFI_TCG2_EVENT_LOG_FORMAT LogFormat;
@@ -651,7 +649,7 @@ MeasureMainBios (
EFI_FV_INFO VolumeInfo;
EFI_PEI_FIRMWARE_VOLUME_PPI *FvPpi;
- PERF_START_EX (mFileHandle, "EventRec", "Tcg2Pei", 0, PERF_ID_TCG2_PEI);
+ PERF_FUNCTION_BEGIN ();
//
// Only measure BFV at the very beginning. Other parts of Static Core Root of
@@ -682,7 +680,7 @@ MeasureMainBios (
Status = MeasureFvImage ((EFI_PHYSICAL_ADDRESS) (UINTN) VolumeInfo.FvStart, VolumeInfo.FvSize);
- PERF_END_EX (mFileHandle, "EventRec", "Tcg2Pei", 0, PERF_ID_TCG2_PEI + 1);
+ PERF_FUNCTION_END ();
return Status;
}
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 6/6] IntelSiliconPkg/IntelVtdDxe: Change the performance code
2019-06-10 7:28 [PATCH 0/6] Change the performance code Gao, Zhichao
` (4 preceding siblings ...)
2019-06-10 7:28 ` [PATCH 5/6] SecurityPkg/Tcg2Pei: " Gao, Zhichao
@ 2019-06-10 7:28 ` Gao, Zhichao
5 siblings, 0 replies; 12+ messages in thread
From: Gao, Zhichao @ 2019-06-10 7:28 UTC (permalink / raw)
To: devel; +Cc: Bret Barkelew, Ray Ni, Rangasai V Chaganty
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_CODE.
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
.../Feature/VTd/IntelVTdDxe/IntelVTdDxe.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c
index a6287be2cf..501933da5c 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c
@@ -230,7 +230,6 @@ VTdSetAttribute (
UINT16 Segment;
VTD_SOURCE_ID SourceId;
CHAR8 PerfToken[sizeof("VTD(S0000.B00.D00.F00)")];
- UINT32 Identifier;
DumpVtdIfError ();
@@ -257,18 +256,12 @@ VTdSetAttribute (
}
Status = RequestAccessAttribute (Segment, SourceId, DeviceAddress, Length, IoMmuAccess);
} else {
- PERF_CODE (
- AsciiSPrint (PerfToken, sizeof(PerfToken), "S%04xB%02xD%02xF%01x", Segment, SourceId.Bits.Bus, SourceId.Bits.Device, SourceId.Bits.Function);
- Identifier = (Segment << 16) | SourceId.Uint16;
- PERF_START_EX (gImageHandle, PerfToken, "IntelVTD", 0, Identifier);
- );
+ AsciiSPrint (PerfToken, sizeof (PerfToken), "S%04xB%02xD%02xF%01x", Segment, SourceId.Bits.Bus, SourceId.Bits.Device, SourceId.Bits.Function);
+ PERF_INMODULE_BEGIN (PerfToken);
Status = SetAccessAttribute (Segment, SourceId, DeviceAddress, Length, IoMmuAccess);
- PERF_CODE (
- Identifier = (Segment << 16) | SourceId.Uint16;
- PERF_END_EX (gImageHandle, PerfToken, "IntelVTD", 0, Identifier);
- );
+ PERF_INMODULE_END (PerfToken);
}
if (!EFI_ERROR(Status)) {
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread