[patch 2/3] ShellPkg/Dp: Initialize summary date when run DP

[patch 2/3] ShellPkg/Dp: Initialize summary date when run DP

[Dandan Bi]

Issue:
When run "dp -s" or ("dp -v") command in shell several times,
the summary reuslts are different each time.

The root cause is that the previous global data "SummaryData"
is not cleaned when the dp command is callled next time.
This patch initializes the global data "SummaryData"
when the dp dymanic command is called.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
---
 ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c b/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
index fe85937f557..d8451dbf59f 100644
--- a/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
+++ b/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
@@ -672,10 +672,28 @@ InitCumulativeData (
     CumData[Index].MaxDur = 0;
     CumData[Index].Duration = 0;
   }
 }
 
+/**
+  Initialize the Summary data.
+
+**/
+VOID
+InitSummaryData (
+  VOID
+  )
+{
+  SummaryData.NumTrace      = 0;
+  SummaryData.NumProfile    = 0 ;
+  SummaryData.NumIncomplete = 0;
+  SummaryData.NumSummary    = 0;
+  SummaryData.NumHandles    = 0;
+  SummaryData.NumPEIMs      = 0;
+  SummaryData.NumGlobal     = 0;
+}
+
 /**
   Dump performance data.
   
   @param[in]  ImageHandle     The image handle.
   @param[in]  SystemTable     The system table.
@@ -817,10 +835,15 @@ RunDp (
   //
   // Initialize the pre-defined cumulative data.
   //
   InitCumulativeData ();
 
+  //
+  // Initialize the Summary data.
+  //
+  InitSummaryData ();
+
   //
   // Init the custom cumulative data.
   //
   CustomCumulativeToken = ShellCommandLineGetValue (ParamPackage, L"-c");
   if (CustomCumulativeToken != NULL) {
-- 
2.14.3.windows.1

[patch 1/3] ShellPkg/Dp: make sure memory is freed before exit

[Dandan Bi]

Run dp command now:
Firstly it will get performance records from FPDT and then
parse the DP command. And if encounter invalid parameters,
it will exit directly. Thus the performance records got before
are invalid. And what's worse is that the memory allocated in
getting performance records phase is not freed.

This patch update the code to parse the command firstly and
then get the performance records. And make sure that all the
clean work has been done before exiting.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
---
 ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c | 70 ++++++++++++++-------------
 1 file changed, 37 insertions(+), 33 deletions(-)

diff --git a/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c b/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
index aa9c2cdf7a8..fe85937f557 100644
--- a/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
+++ b/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
@@ -390,11 +390,11 @@ BuildCachedGuidHandleTable (
   }
   if (HandleBuffer != NULL) {
     FreePool (HandleBuffer);
     HandleBuffer = NULL;
   }
-  return Status;
+  return EFI_SUCCESS;
 }
 
 /**
   Get Measurement form Fpdt records.
 
@@ -729,39 +729,10 @@ RunDp (
   // initialize the shell lib (we must be in non-auto-init...)
   //
   Status = ShellInitialize();
   ASSERT_EFI_ERROR(Status);
 
-  //
-  // DP dump performance data by parsing FPDT table in ACPI table.
-  // Folloing 3 steps are to get the measurement form the FPDT table.
-  //
-
-  //
-  //1. Get FPDT from ACPI table.
-  //
-  Status = GetBootPerformanceTable ();
-  if (EFI_ERROR(Status)) {
-    return Status;
-  }
-
-  //
-  //2. Cache the ModuleGuid and hanlde mapping table.
-  //
-  Status = BuildCachedGuidHandleTable();
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
-  //
-  //3. Build the measurement array form the FPDT records.
-  //
-  Status = BuildMeasurementList ();
-  if (EFI_ERROR(Status)) {
-    return Status;
-  }
-
   //
   // Process Command Line arguments
   //
   Status = ShellCommandLineParse (ParamList, &ParamPackage, NULL, TRUE);
   if (EFI_ERROR(Status)) {
@@ -809,10 +780,42 @@ RunDp (
 #if PROFILING_IMPLEMENTED
     ProfileMode = TRUE;
 #endif  // PROFILING_IMPLEMENTED
   }
 
+  //
+  // DP dump performance data by parsing FPDT table in ACPI table.
+  // Folloing 3 steps are to get the measurement form the FPDT table.
+  //
+
+  //
+  //1. Get FPDT from ACPI table.
+  //
+  Status = GetBootPerformanceTable ();
+  if (EFI_ERROR (Status)) {
+    ShellStatus = Status;
+    goto Done;
+  }
+
+  //
+  //2. Cache the ModuleGuid and hanlde mapping table.
+  //
+  Status = BuildCachedGuidHandleTable();
+  if (EFI_ERROR (Status)) {
+    ShellStatus = Status;
+    goto Done;
+  }
+
+  //
+  //3. Build the measurement array form the FPDT records.
+  //
+  Status = BuildMeasurementList ();
+  if (EFI_ERROR (Status)) {
+    ShellStatus = SHELL_OUT_OF_RESOURCES;
+    goto Done;
+  }
+
   //
   // Initialize the pre-defined cumulative data.
   //
   InitCumulativeData ();
 
@@ -821,21 +824,22 @@ RunDp (
   //
   CustomCumulativeToken = ShellCommandLineGetValue (ParamPackage, L"-c");
   if (CustomCumulativeToken != NULL) {
     CustomCumulativeData = AllocateZeroPool (sizeof (PERF_CUM_DATA));
     if (CustomCumulativeData == NULL) {
-      return SHELL_OUT_OF_RESOURCES;
+      ShellStatus = SHELL_OUT_OF_RESOURCES;
+      goto Done;
     }
     CustomCumulativeData->MinDur = PERF_MAXDUR;
     CustomCumulativeData->MaxDur = 0;
     CustomCumulativeData->Count  = 0;
     CustomCumulativeData->Duration = 0;
     NameSize = StrLen (CustomCumulativeToken) + 1;
     CustomCumulativeData->Name   = AllocateZeroPool (NameSize);
     if (CustomCumulativeData->Name == NULL) {
-      FreePool (CustomCumulativeData);
-      return SHELL_OUT_OF_RESOURCES;
+      ShellStatus = SHELL_OUT_OF_RESOURCES;
+      goto Done;
     }
     UnicodeStrToAsciiStrS (CustomCumulativeToken, CustomCumulativeData->Name, NameSize);
   }
 
   //
-- 
2.14.3.windows.1

[patch 2/3] ShellPkg/Dp: Initialize summary date when run DP

[Dandan Bi]

Issue:
When run "dp -s" or ("dp -v") command in shell several times,
the summary reuslts are different each time.

The root cause is that the previous global data "SummaryData"
is not cleaned when the dp command is callled next time.
This patch initializes the global data "SummaryData"
when the dp dymanic command is called.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
---
 ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c b/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
index fe85937f557..d8451dbf59f 100644
--- a/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
+++ b/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
@@ -672,10 +672,28 @@ InitCumulativeData (
     CumData[Index].MaxDur = 0;
     CumData[Index].Duration = 0;
   }
 }
 
+/**
+  Initialize the Summary data.
+
+**/
+VOID
+InitSummaryData (
+  VOID
+  )
+{
+  SummaryData.NumTrace      = 0;
+  SummaryData.NumProfile    = 0 ;
+  SummaryData.NumIncomplete = 0;
+  SummaryData.NumSummary    = 0;
+  SummaryData.NumHandles    = 0;
+  SummaryData.NumPEIMs      = 0;
+  SummaryData.NumGlobal     = 0;
+}
+
 /**
   Dump performance data.
   
   @param[in]  ImageHandle     The image handle.
   @param[in]  SystemTable     The system table.
@@ -817,10 +835,15 @@ RunDp (
   //
   // Initialize the pre-defined cumulative data.
   //
   InitCumulativeData ();
 
+  //
+  // Initialize the Summary data.
+  //
+  InitSummaryData ();
+
   //
   // Init the custom cumulative data.
   //
   CustomCumulativeToken = ShellCommandLineGetValue (ParamPackage, L"-c");
   if (CustomCumulativeToken != NULL) {
-- 
2.14.3.windows.1

[patch 3/3] ShellPkg/Dp: Make the help info align with code

[Dandan Bi]

Remove -T, -P, -h flags in the help info of DP to
align with current code implementation.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
---
 ShellPkg/DynamicCommand/DpDynamicCommand/Dp.uni | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.uni b/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.uni
index c7eb0fbd71e..ede59069b79 100644
--- a/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.uni
+++ b/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.uni
@@ -96,21 +96,19 @@
 #string STR_GET_HELP_DP         #language en-US ""
 ".TH dp 0 "Display performance metrics"\r\n"
 ".SH NAME\r\n"
 "Displays performance metrics that are stored in memory.\r\n"
 ".SH SYNOPSIS\r\n"
-"DP [-b] [-v] [-x] [-s | -A | -R] [-T] [-P] [-t value] [-n count] [-c [token]][-i] [-h | -?]\r\n"
+"DP [-b] [-v] [-x] [-s | -A | -R] [-t value] [-n count] [-c [token]][-i] [-?]\r\n"
 ".SH OPTIONS\r\n"
 " \r\n"
 "  -b       - Displays on multiple pages\r\n"
 "  -v       - Displays additional information\r\n"
 "  -x       - Prevents display of individual measurements for cumulative items\r\n"
 "  -s       - Displays summary information only\r\n"
 "  -A       - Displays all measurements in a list\r\n"
 "  -R       - Displays all measurements in raw format\r\n"
-"  -T       - Displays trace measurements only\r\n"
-"  -P       - Displays profile measurements only\r\n"
 "  -t VALUE - Sets display threshold to VALUE microseconds\r\n"
 "  -n COUNT - Limits display to COUNT lines in All and Raw modes\r\n"
 "  -i       - Displays identifier\r\n"
 "  -c TOKEN - Display pre-defined and custom cumulative data\r\n" 
 "             Pre-defined cumulative token are:\r\n"
-- 
2.14.3.windows.1

Re: [patch 1/3] ShellPkg/Dp: make sure memory is freed before exit

[Ni, Ruiyu]

Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

Thanks/Ray

> -----Original Message-----
> From: Bi, Dandan
> Sent: Tuesday, June 5, 2018 4:33 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
> Carsey, Jaben <jaben.carsey@intel.com>
> Subject: [patch 1/3] ShellPkg/Dp: make sure memory is freed before exit
> 
> Run dp command now:
> Firstly it will get performance records from FPDT and then parse the DP
> command. And if encounter invalid parameters, it will exit directly. Thus the
> performance records got before are invalid. And what's worse is that the
> memory allocated in getting performance records phase is not freed.
> 
> This patch update the code to parse the command firstly and then get the
> performance records. And make sure that all the clean work has been done
> before exiting.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> ---
>  ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c | 70
> ++++++++++++++-------------
>  1 file changed, 37 insertions(+), 33 deletions(-)
> 
> diff --git a/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
> b/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
> index aa9c2cdf7a8..fe85937f557 100644
> --- a/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
> +++ b/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c
> @@ -390,11 +390,11 @@ BuildCachedGuidHandleTable (
>    }
>    if (HandleBuffer != NULL) {
>      FreePool (HandleBuffer);
>      HandleBuffer = NULL;
>    }
> -  return Status;
> +  return EFI_SUCCESS;
>  }
> 
>  /**
>    Get Measurement form Fpdt records.
> 
> @@ -729,39 +729,10 @@ RunDp (
>    // initialize the shell lib (we must be in non-auto-init...)
>    //
>    Status = ShellInitialize();
>    ASSERT_EFI_ERROR(Status);
> 
> -  //
> -  // DP dump performance data by parsing FPDT table in ACPI table.
> -  // Folloing 3 steps are to get the measurement form the FPDT table.
> -  //
> -
> -  //
> -  //1. Get FPDT from ACPI table.
> -  //
> -  Status = GetBootPerformanceTable ();
> -  if (EFI_ERROR(Status)) {
> -    return Status;
> -  }
> -
> -  //
> -  //2. Cache the ModuleGuid and hanlde mapping table.
> -  //
> -  Status = BuildCachedGuidHandleTable();
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
> -  //
> -  //3. Build the measurement array form the FPDT records.
> -  //
> -  Status = BuildMeasurementList ();
> -  if (EFI_ERROR(Status)) {
> -    return Status;
> -  }
> -
>    //
>    // Process Command Line arguments
>    //
>    Status = ShellCommandLineParse (ParamList, &ParamPackage, NULL, TRUE);
>    if (EFI_ERROR(Status)) {
> @@ -809,10 +780,42 @@ RunDp (
>  #if PROFILING_IMPLEMENTED
>      ProfileMode = TRUE;
>  #endif  // PROFILING_IMPLEMENTED
>    }
> 
> +  //
> +  // DP dump performance data by parsing FPDT table in ACPI table.
> +  // Folloing 3 steps are to get the measurement form the FPDT table.
> +  //
> +
> +  //
> +  //1. Get FPDT from ACPI table.
> +  //
> +  Status = GetBootPerformanceTable ();
> +  if (EFI_ERROR (Status)) {
> +    ShellStatus = Status;
> +    goto Done;
> +  }
> +
> +  //
> +  //2. Cache the ModuleGuid and hanlde mapping table.
> +  //
> +  Status = BuildCachedGuidHandleTable();  if (EFI_ERROR (Status)) {
> +    ShellStatus = Status;
> +    goto Done;
> +  }
> +
> +  //
> +  //3. Build the measurement array form the FPDT records.
> +  //
> +  Status = BuildMeasurementList ();
> +  if (EFI_ERROR (Status)) {
> +    ShellStatus = SHELL_OUT_OF_RESOURCES;
> +    goto Done;
> +  }
> +
>    //
>    // Initialize the pre-defined cumulative data.
>    //
>    InitCumulativeData ();
> 
> @@ -821,21 +824,22 @@ RunDp (
>    //
>    CustomCumulativeToken = ShellCommandLineGetValue (ParamPackage,
> L"-c");
>    if (CustomCumulativeToken != NULL) {
>      CustomCumulativeData = AllocateZeroPool (sizeof (PERF_CUM_DATA));
>      if (CustomCumulativeData == NULL) {
> -      return SHELL_OUT_OF_RESOURCES;
> +      ShellStatus = SHELL_OUT_OF_RESOURCES;
> +      goto Done;
>      }
>      CustomCumulativeData->MinDur = PERF_MAXDUR;
>      CustomCumulativeData->MaxDur = 0;
>      CustomCumulativeData->Count  = 0;
>      CustomCumulativeData->Duration = 0;
>      NameSize = StrLen (CustomCumulativeToken) + 1;
>      CustomCumulativeData->Name   = AllocateZeroPool (NameSize);
>      if (CustomCumulativeData->Name == NULL) {
> -      FreePool (CustomCumulativeData);
> -      return SHELL_OUT_OF_RESOURCES;
> +      ShellStatus = SHELL_OUT_OF_RESOURCES;
> +      goto Done;
>      }
>      UnicodeStrToAsciiStrS (CustomCumulativeToken, CustomCumulativeData-
> >Name, NameSize);
>    }
> 
>    //
> --
> 2.14.3.windows.1

Re: [patch 3/3] ShellPkg/Dp: Make the help info align with code

[Gao, Liming]

Dandan:
  I find there is still some code related to -T, -P flags. If they are not implemented, I suggest to remove them from source code also. 

Thanks
Liming
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>Dandan Bi
>Sent: Tuesday, June 05, 2018 4:33 PM
>To: edk2-devel@lists.01.org
>Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Carsey, Jaben <jaben.carsey@intel.com>;
>Gao, Liming <liming.gao@intel.com>
>Subject: [edk2] [patch 3/3] ShellPkg/Dp: Make the help info align with code
>
>Remove -T, -P, -h flags in the help info of DP to
>align with current code implementation.
>
>Cc: Liming Gao <liming.gao@intel.com>
>Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>Cc: Jaben Carsey <jaben.carsey@intel.com>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Dandan Bi <dandan.bi@intel.com>
>---
> ShellPkg/DynamicCommand/DpDynamicCommand/Dp.uni | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
>diff --git a/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.uni
>b/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.uni
>index c7eb0fbd71e..ede59069b79 100644
>--- a/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.uni
>+++ b/ShellPkg/DynamicCommand/DpDynamicCommand/Dp.uni
>@@ -96,21 +96,19 @@
> #string STR_GET_HELP_DP         #language en-US ""
> ".TH dp 0 "Display performance metrics"\r\n"
> ".SH NAME\r\n"
> "Displays performance metrics that are stored in memory.\r\n"
> ".SH SYNOPSIS\r\n"
>-"DP [-b] [-v] [-x] [-s | -A | -R] [-T] [-P] [-t value] [-n count] [-c [token]][-i] [-h
>| -?]\r\n"
>+"DP [-b] [-v] [-x] [-s | -A | -R] [-t value] [-n count] [-c [token]][-i] [-?]\r\n"
> ".SH OPTIONS\r\n"
> " \r\n"
> "  -b       - Displays on multiple pages\r\n"
> "  -v       - Displays additional information\r\n"
> "  -x       - Prevents display of individual measurements for cumulative
>items\r\n"
> "  -s       - Displays summary information only\r\n"
> "  -A       - Displays all measurements in a list\r\n"
> "  -R       - Displays all measurements in raw format\r\n"
>-"  -T       - Displays trace measurements only\r\n"
>-"  -P       - Displays profile measurements only\r\n"
> "  -t VALUE - Sets display threshold to VALUE microseconds\r\n"
> "  -n COUNT - Limits display to COUNT lines in All and Raw modes\r\n"
> "  -i       - Displays identifier\r\n"
> "  -c TOKEN - Display pre-defined and custom cumulative data\r\n"
> "             Pre-defined cumulative token are:\r\n"
>--
>2.14.3.windows.1
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel