[v2,2/3] perf/x86/rapl: Support multiple rapl unit quirks
diff mbox series

Message ID 20200811153149.12242-3-rui.zhang@intel.com
State Accepted
Commit 74f41adab0f4a61857833e1b6fa8e9ad12c251b6
Headers show
Series
  • perf/x86/rapl: Add Intel SapphireRapids support
Related show

Commit Message

Zhang, Rui Aug. 11, 2020, 3:31 p.m. UTC
There will be more platforms with different fixed energy units.
Enhance the code to support different rapl unit quirks for different
platforms.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
---
 arch/x86/events/rapl.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

Comments

Joe Perches Aug. 11, 2020, 6:19 p.m. UTC | #1
On Tue, 2020-08-11 at 23:31 +0800, Zhang Rui wrote:
> There will be more platforms with different fixed energy units.
> Enhance the code to support different rapl unit quirks for different
> platforms.

This seems like one quirk per platform.

Should multiple quirks on individual platforms be supported?

> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
[]
> @@ -130,11 +130,16 @@ struct rapl_pmus {
>  	struct rapl_pmu		*pmus[];
>  };
>  
> +enum rapl_unit_quirk {
> +	RAPL_UNIT_QUIRK_NONE,
> +	RAPL_UNIT_QUIRK_INTEL_HSW,
> +};
> +
>  struct rapl_model {
>  	struct perf_msr *rapl_msrs;
>  	unsigned long	events;
>  	unsigned int	msr_power_unit;
> -	bool		apply_quirk;
> +	enum rapl_unit_quirk	unit_quirk;
>  };
Zhang, Rui Aug. 12, 2020, 3:29 a.m. UTC | #2
Hi,

Thanks for reviewing.

On Tue, 2020-08-11 at 11:19 -0700, Joe Perches wrote:
> On Tue, 2020-08-11 at 23:31 +0800, Zhang Rui wrote:
> > There will be more platforms with different fixed energy units.
> > Enhance the code to support different rapl unit quirks for
> > different
> > platforms.
> 
> This seems like one quirk per platform.
> 
> Should multiple quirks on individual platforms be supported?
> 
enum rapl_unit_quirk is just used as a flag.
multiple quirks can be deployed with the same flag, just like what I
did in patch 3/3.
Also different platforms can either have different flags or share the
same flag.

thanks,
rui

> > diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> 
> []
> > @@ -130,11 +130,16 @@ struct rapl_pmus {
> >  	struct rapl_pmu		*pmus[];
> >  };
> >  
> > +enum rapl_unit_quirk {
> > +	RAPL_UNIT_QUIRK_NONE,
> > +	RAPL_UNIT_QUIRK_INTEL_HSW,
> > +};
> > +
> >  struct rapl_model {
> >  	struct perf_msr *rapl_msrs;
> >  	unsigned long	events;
> >  	unsigned int	msr_power_unit;
> > -	bool		apply_quirk;
> > +	enum rapl_unit_quirk	unit_quirk;
> >  };
> 
>
Joe Perches Aug. 12, 2020, 3:51 a.m. UTC | #3
On Wed, 2020-08-12 at 11:29 +0800, Zhang Rui wrote:
> Hi,
> 
> Thanks for reviewing.
> 
> On Tue, 2020-08-11 at 11:19 -0700, Joe Perches wrote:
> > On Tue, 2020-08-11 at 23:31 +0800, Zhang Rui wrote:
> > > There will be more platforms with different fixed energy units.
> > > Enhance the code to support different rapl unit quirks for
> > > different
> > > platforms.
> > 
> > This seems like one quirk per platform.
> > 
> > Should multiple quirks on individual platforms be supported?
> > 
> enum rapl_unit_quirk is just used as a flag.
> multiple quirks can be deployed with the same flag, just like what I
> did in patch 3/3.
> Also different platforms can either have different flags or share the
> same flag.

Sure, but it does lead to possible code duplication in the quirks
as enums can not be combined like bit flags.

No worries, your code, your choice...

Patch
diff mbox series

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index e9723833551f..d0002eb971b7 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -130,11 +130,16 @@  struct rapl_pmus {
 	struct rapl_pmu		*pmus[];
 };
 
+enum rapl_unit_quirk {
+	RAPL_UNIT_QUIRK_NONE,
+	RAPL_UNIT_QUIRK_INTEL_HSW,
+};
+
 struct rapl_model {
 	struct perf_msr *rapl_msrs;
 	unsigned long	events;
 	unsigned int	msr_power_unit;
-	bool		apply_quirk;
+	enum rapl_unit_quirk	unit_quirk;
 };
 
  /* 1/2^hw_unit Joule */
@@ -612,14 +617,20 @@  static int rapl_check_hw_unit(struct rapl_model *rm)
 	for (i = 0; i < NR_RAPL_DOMAINS; i++)
 		rapl_hw_unit[i] = (msr_rapl_power_unit_bits >> 8) & 0x1FULL;
 
+	switch (rm->unit_quirk) {
 	/*
 	 * DRAM domain on HSW server and KNL has fixed energy unit which can be
 	 * different than the unit from power unit MSR. See
 	 * "Intel Xeon Processor E5-1600 and E5-2600 v3 Product Families, V2
 	 * of 2. Datasheet, September 2014, Reference Number: 330784-001 "
 	 */
-	if (rm->apply_quirk)
+	case RAPL_UNIT_QUIRK_INTEL_HSW:
 		rapl_hw_unit[PERF_RAPL_RAM] = 16;
+		break;
+	default:
+		break;
+	}
+
 
 	/*
 	 * Calculate the timer rate:
@@ -698,7 +709,6 @@  static struct rapl_model model_snb = {
 	.events		= BIT(PERF_RAPL_PP0) |
 			  BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_PP1),
-	.apply_quirk	= false,
 	.msr_power_unit = MSR_RAPL_POWER_UNIT,
 	.rapl_msrs      = intel_rapl_msrs,
 };
@@ -707,7 +717,6 @@  static struct rapl_model model_snbep = {
 	.events		= BIT(PERF_RAPL_PP0) |
 			  BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_RAM),
-	.apply_quirk	= false,
 	.msr_power_unit = MSR_RAPL_POWER_UNIT,
 	.rapl_msrs      = intel_rapl_msrs,
 };
@@ -717,7 +726,6 @@  static struct rapl_model model_hsw = {
 			  BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_RAM) |
 			  BIT(PERF_RAPL_PP1),
-	.apply_quirk	= false,
 	.msr_power_unit = MSR_RAPL_POWER_UNIT,
 	.rapl_msrs      = intel_rapl_msrs,
 };
@@ -726,7 +734,7 @@  static struct rapl_model model_hsx = {
 	.events		= BIT(PERF_RAPL_PP0) |
 			  BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_RAM),
-	.apply_quirk	= true,
+	.unit_quirk	= RAPL_UNIT_QUIRK_INTEL_HSW,
 	.msr_power_unit = MSR_RAPL_POWER_UNIT,
 	.rapl_msrs      = intel_rapl_msrs,
 };
@@ -734,7 +742,7 @@  static struct rapl_model model_hsx = {
 static struct rapl_model model_knl = {
 	.events		= BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_RAM),
-	.apply_quirk	= true,
+	.unit_quirk	= RAPL_UNIT_QUIRK_INTEL_HSW,
 	.msr_power_unit = MSR_RAPL_POWER_UNIT,
 	.rapl_msrs      = intel_rapl_msrs,
 };
@@ -745,14 +753,12 @@  static struct rapl_model model_skl = {
 			  BIT(PERF_RAPL_RAM) |
 			  BIT(PERF_RAPL_PP1) |
 			  BIT(PERF_RAPL_PSYS),
-	.apply_quirk	= false,
 	.msr_power_unit = MSR_RAPL_POWER_UNIT,
 	.rapl_msrs      = intel_rapl_msrs,
 };
 
 static struct rapl_model model_amd_fam17h = {
 	.events		= BIT(PERF_RAPL_PKG),
-	.apply_quirk	= false,
 	.msr_power_unit = MSR_AMD_RAPL_POWER_UNIT,
 	.rapl_msrs      = amd_rapl_msrs,
 };