[PATCH 1/2] perf/x86/rapl: Enable Apollo Lake RAPL support

[PATCH 1/2] perf/x86/rapl: Enable Apollo Lake RAPL support

[Harry Pan]

This patch enables RAPL counters (energy consumption counters)
support for Intel Apollo Lake (Goldmont) processors (Model 92):

RAPL of Goldmont, unlikes ESU increment of Silvermont/Airmont,
it likes the Haswell microarchitecture in 1/2^ESU joules and
supports power domains in PP0/PP1/PKG/RAM.

ESU and power domains refer to Intel Software Developers' Manual,
Vol. 3C, Order No. 325384, Table 35-12.

Usage example:

$ perf list
$ perf stat -a -e power/energy-cores/,power/energy-pkg/ sleep 10

Signed-off-by: Harry Pan <harry.pan@intel.com>
---
 arch/x86/events/intel/rapl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
index 2886593..f7924640 100644
--- a/arch/x86/events/intel/rapl.c
+++ b/arch/x86/events/intel/rapl.c
@@ -765,6 +765,8 @@ static const struct x86_cpu_id rapl_cpu_match[] __initconst = {
 	X86_RAPL_MODEL_MATCH(INTEL_FAM6_SKYLAKE_MOBILE,  skl_rapl_init),
 	X86_RAPL_MODEL_MATCH(INTEL_FAM6_SKYLAKE_DESKTOP, skl_rapl_init),
 	X86_RAPL_MODEL_MATCH(INTEL_FAM6_SKYLAKE_X,	 hsx_rapl_init),
+
+	X86_RAPL_MODEL_MATCH(INTEL_FAM6_ATOM_GOLDMONT, hsw_rapl_init),
 	{},
 };
 
-- 
2.6.6

[PATCH 2/2] perf/x86/rapl: Enable Baytrail/Braswell RAPL support

[Harry Pan]

This patch enables RAPL counters (energy consumption counters)
support for Intel Baytrail and Braswell processors (Model 55 and 76):

The Silvermont/Airmont microarchitecture actually uses fixed
energy status unit (ESU) in smallest unit of microjoule,
this patch adds quirk for these Atom processors (BYT/BSW)
to calculate energy increment in 2^ESU microjoules.

ESU and power domains refer to Intel Software Developers' Manual,
Vol. 3C, Order No. 325384, Table 35-8.

Usage example:

$ perf list
$ perf stat -a -e power/energy-cores/,power/energy-pkg/ sleep 10

This patch also enables multiple quirks.

Signed-off-by: Harry Pan <harry.pan@intel.com>
---
 arch/x86/events/intel/rapl.c | 68 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 58 insertions(+), 10 deletions(-)

diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
index f7924640..328fea4 100644
--- a/arch/x86/events/intel/rapl.c
+++ b/arch/x86/events/intel/rapl.c
@@ -110,6 +110,10 @@ static const char *const rapl_domain_names[NR_RAPL_DOMAINS] __initconst = {
 #define RAPL_IDX_KNL	(1<<RAPL_IDX_PKG_NRG_STAT|\
 			 1<<RAPL_IDX_RAM_NRG_STAT)
 
+/* Baytrail/Braswell clients have PP0, PKG */
+#define RAPL_IDX_BYT	(1<<RAPL_IDX_PP0_NRG_STAT|\
+			 1<<RAPL_IDX_PKG_NRG_STAT)
+
 /*
  * event code: LSB 8 bits, passed in attr->config
  * any other bit is reserved
@@ -136,6 +140,12 @@ static struct perf_pmu_events_attr event_attr_##v = {				\
 	.event_str	= str,							\
 };
 
+enum rapl_quirk {
+	RAPL_NO_QUIRK = 0,
+	RAPL_HSX_QUIRK,
+	RAPL_BYT_QUIRK,
+};
+
 struct rapl_pmu {
 	raw_spinlock_t		lock;
 	int			n_active;
@@ -452,6 +462,14 @@ RAPL_EVENT_ATTR_STR(energy-ram.scale,     rapl_ram_scale, "2.3283064365386962890
 RAPL_EVENT_ATTR_STR(energy-gpu.scale,     rapl_gpu_scale, "2.3283064365386962890625e-10");
 RAPL_EVENT_ATTR_STR(energy-psys.scale,   rapl_psys_scale, "2.3283064365386962890625e-10");
 
+/*
+ * Some Atom series processors (BYT/BSW) have fixed
+ * energy status unit (ESU) in smallest unit of microjoule,
+ * and its increment is in 2^ESU microjoules.
+ */
+RAPL_EVENT_ATTR_STR(energy-cores.scale, rapl_byt_cores_scale, "1.0e-6");
+RAPL_EVENT_ATTR_STR(energy-pkg.scale, rapl_byt_pkg_scale, "1.0e-6");
+
 static struct attribute *rapl_events_srv_attr[] = {
 	EVENT_PTR(rapl_cores),
 	EVENT_PTR(rapl_pkg),
@@ -533,6 +551,18 @@ static struct attribute *rapl_events_knl_attr[] = {
 	NULL,
 };
 
+static struct attribute *rapl_events_byt_attr[] = {
+	EVENT_PTR(rapl_cores),
+	EVENT_PTR(rapl_pkg),
+
+	EVENT_PTR(rapl_cores_unit),
+	EVENT_PTR(rapl_pkg_unit),
+
+	EVENT_PTR(rapl_byt_cores_scale),
+	EVENT_PTR(rapl_byt_pkg_scale),
+	NULL,
+};
+
 static struct attribute_group rapl_pmu_events_group = {
 	.name = "events",
 	.attrs = NULL, /* patched at runtime */
@@ -617,7 +647,7 @@ static int rapl_cpu_prepare(unsigned int cpu)
 	return 0;
 }
 
-static int rapl_check_hw_unit(bool apply_quirk)
+static int rapl_check_hw_unit(enum rapl_quirk apply_quirk)
 {
 	u64 msr_rapl_power_unit_bits;
 	int i;
@@ -634,10 +664,20 @@ static int rapl_check_hw_unit(bool apply_quirk)
 	 * "Intel Xeon Processor E5-1600 and E5-2600 v3 Product Families, V2
 	 * of 2. Datasheet, September 2014, Reference Number: 330784-001 "
 	 */
-	if (apply_quirk)
+	if (apply_quirk == RAPL_HSX_QUIRK)
 		rapl_hw_unit[RAPL_IDX_RAM_NRG_STAT] = 16;
 
 	/*
+	 * Some Atom processors (BYT/BSW) have 2^ESU microjoules increment,
+	 * refer to Software Developers' Manual, Vol. 3C, Order No. 325384,
+	 * Table 35-8 of MSR_RAPL_POWER_UNIT
+	 */
+	if (apply_quirk == RAPL_BYT_QUIRK) {
+		for (i = 0; i < NR_RAPL_DOMAINS; i++)
+			rapl_hw_unit[i] = 32 - rapl_hw_unit[i];
+	}
+
+	/*
 	 * Calculate the timer rate:
 	 * Use reference of 200W for scaling the timeout to avoid counter
 	 * overflows. 200W = 200 Joules/sec
@@ -702,47 +742,53 @@ static int __init init_rapl_pmus(void)
 	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (unsigned long)&init }
 
 struct intel_rapl_init_fun {
-	bool apply_quirk;
+	enum rapl_quirk apply_quirk;
 	int cntr_mask;
 	struct attribute **attrs;
 };
 
 static const struct intel_rapl_init_fun snb_rapl_init __initconst = {
-	.apply_quirk = false,
+	.apply_quirk = RAPL_NO_QUIRK,
 	.cntr_mask = RAPL_IDX_CLN,
 	.attrs = rapl_events_cln_attr,
 };
 
 static const struct intel_rapl_init_fun hsx_rapl_init __initconst = {
-	.apply_quirk = true,
+	.apply_quirk = RAPL_HSX_QUIRK,
 	.cntr_mask = RAPL_IDX_SRV,
 	.attrs = rapl_events_srv_attr,
 };
 
 static const struct intel_rapl_init_fun hsw_rapl_init __initconst = {
-	.apply_quirk = false,
+	.apply_quirk = RAPL_NO_QUIRK,
 	.cntr_mask = RAPL_IDX_HSW,
 	.attrs = rapl_events_hsw_attr,
 };
 
 static const struct intel_rapl_init_fun snbep_rapl_init __initconst = {
-	.apply_quirk = false,
+	.apply_quirk = RAPL_NO_QUIRK,
 	.cntr_mask = RAPL_IDX_SRV,
 	.attrs = rapl_events_srv_attr,
 };
 
 static const struct intel_rapl_init_fun knl_rapl_init __initconst = {
-	.apply_quirk = true,
+	.apply_quirk = RAPL_HSX_QUIRK,
 	.cntr_mask = RAPL_IDX_KNL,
 	.attrs = rapl_events_knl_attr,
 };
 
 static const struct intel_rapl_init_fun skl_rapl_init __initconst = {
-	.apply_quirk = false,
+	.apply_quirk = RAPL_NO_QUIRK,
 	.cntr_mask = RAPL_IDX_SKL_CLN,
 	.attrs = rapl_events_skl_attr,
 };
 
+static const struct intel_rapl_init_fun byt_rapl_init __initconst = {
+	.apply_quirk = RAPL_BYT_QUIRK,
+	.cntr_mask = RAPL_IDX_BYT,
+	.attrs = rapl_events_byt_attr,
+};
+
 static const struct x86_cpu_id rapl_cpu_match[] __initconst = {
 	X86_RAPL_MODEL_MATCH(INTEL_FAM6_SANDYBRIDGE,   snb_rapl_init),
 	X86_RAPL_MODEL_MATCH(INTEL_FAM6_SANDYBRIDGE_X, snbep_rapl_init),
@@ -766,6 +812,8 @@ static const struct x86_cpu_id rapl_cpu_match[] __initconst = {
 	X86_RAPL_MODEL_MATCH(INTEL_FAM6_SKYLAKE_DESKTOP, skl_rapl_init),
 	X86_RAPL_MODEL_MATCH(INTEL_FAM6_SKYLAKE_X,	 hsx_rapl_init),
 
+	X86_RAPL_MODEL_MATCH(INTEL_FAM6_ATOM_SILVERMONT1, byt_rapl_init),
+	X86_RAPL_MODEL_MATCH(INTEL_FAM6_ATOM_AIRMONT, byt_rapl_init),
 	X86_RAPL_MODEL_MATCH(INTEL_FAM6_ATOM_GOLDMONT, hsw_rapl_init),
 	{},
 };
@@ -776,7 +824,7 @@ static int __init rapl_pmu_init(void)
 {
 	const struct x86_cpu_id *id;
 	struct intel_rapl_init_fun *rapl_init;
-	bool apply_quirk;
+	enum rapl_quirk apply_quirk;
 	int ret;
 
 	id = x86_match_cpu(rapl_cpu_match);
-- 
2.6.6

Re: [PATCH 2/2] perf/x86/rapl: Enable Baytrail/Braswell RAPL support

[Thomas Gleixner]

On Fri, 9 Sep 2016, Harry Pan wrote:
> -	if (apply_quirk)
> +	if (apply_quirk == RAPL_HSX_QUIRK)
>  		rapl_hw_unit[RAPL_IDX_RAM_NRG_STAT] = 16;
>  
>  	/*
> +	 * Some Atom processors (BYT/BSW) have 2^ESU microjoules increment,
> +	 * refer to Software Developers' Manual, Vol. 3C, Order No. 325384,
> +	 * Table 35-8 of MSR_RAPL_POWER_UNIT
> +	 */
> +	if (apply_quirk == RAPL_BYT_QUIRK) {
> +		for (i = 0; i < NR_RAPL_DOMAINS; i++)
> +			rapl_hw_unit[i] = 32 - rapl_hw_unit[i];
> +	}

switch(quirk) if at all, but see below.

> +	/*
>  	 * Calculate the timer rate:
>  	 * Use reference of 200W for scaling the timeout to avoid counter
>  	 * overflows. 200W = 200 Joules/sec
> @@ -702,47 +742,53 @@ static int __init init_rapl_pmus(void)
>  	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (unsigned long)&init }
>  
>  struct intel_rapl_init_fun {
> -	bool apply_quirk;
> +	enum rapl_quirk apply_quirk;

This is silly. Make apply_quirk a function pointer and provide functions
for the different quirks.

>  	int cntr_mask;
>  	struct attribute **attrs;
>  };
>  
>  static const struct intel_rapl_init_fun snb_rapl_init __initconst = {
> -	.apply_quirk = false,
> +	.apply_quirk = RAPL_NO_QUIRK,

Zero ininitalization has no real value other than consuming state space.

Thanks,

	tglx

Re: [PATCH 2/2] perf/x86/rapl: Enable Baytrail/Braswell RAPL support

[Pan, Harry]

I refined/uploaded again, kindly advise.

On Fri, 2016-09-09 at 17:11 +0200, Thomas Gleixner wrote:
> On Fri, 9 Sep 2016, Harry Pan wrote:
> > -	if (apply_quirk)
> > +	if (apply_quirk == RAPL_HSX_QUIRK)
> >  		rapl_hw_unit[RAPL_IDX_RAM_NRG_STAT] = 16;
> >  
> >  	/*
> > +	 * Some Atom processors (BYT/BSW) have 2^ESU microjoules increment,
> > +	 * refer to Software Developers' Manual, Vol. 3C, Order No. 325384,
> > +	 * Table 35-8 of MSR_RAPL_POWER_UNIT
> > +	 */
> > +	if (apply_quirk == RAPL_BYT_QUIRK) {
> > +		for (i = 0; i < NR_RAPL_DOMAINS; i++)
> > +			rapl_hw_unit[i] = 32 - rapl_hw_unit[i];
> > +	}
> 
> switch(quirk) if at all, but see below.
Yes, v3 I refined as switch.

> 
> > +	/*
> >  	 * Calculate the timer rate:
> >  	 * Use reference of 200W for scaling the timeout to avoid counter
> >  	 * overflows. 200W = 200 Joules/sec
> > @@ -702,47 +742,53 @@ static int __init init_rapl_pmus(void)
> >  	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (unsigned long)&init }
> >  
> >  struct intel_rapl_init_fun {
> > -	bool apply_quirk;
> > +	enum rapl_quirk apply_quirk;
> 
> This is silly. Make apply_quirk a function pointer and provide functions
> for the different quirks.
I read the rapl_check_hw_unit() as: read MSR_RAPL_POWER_UNIT, apply
quirk if need, then estimate timer rate.

In case to refine struct intel_rapl_init_fun adding callback, then
either the quirk moving outside the rapl_check_hw_unit(), or replace
input parameter as whole rapl_init in order to assess quirk callback, by
far it looks to me centralize these two quirks inside this function more
easily to maintain.
 
> 
> >  	int cntr_mask;
> >  	struct attribute **attrs;
> >  };
> >  
> >  static const struct intel_rapl_init_fun snb_rapl_init __initconst = {
> > -	.apply_quirk = false,
> > +	.apply_quirk = RAPL_NO_QUIRK,
> 
> Zero ininitalization has no real value other than consuming state space.
To enable more than one quirk I extended bool to enum, I thought the
__initconst space would be freed after kernel initialized, is there more
detail concern I missed?

Sincerely,
Harry

Re: [PATCH 2/2] perf/x86/rapl: Enable Baytrail/Braswell RAPL support

[Thomas Gleixner]

On Fri, 9 Sep 2016, Pan, Harry wrote:
> On Fri, 2016-09-09 at 17:11 +0200, Thomas Gleixner wrote:
> > >  struct intel_rapl_init_fun {
> > > -	bool apply_quirk;
> > > +	enum rapl_quirk apply_quirk;
> > 
> > This is silly. Make apply_quirk a function pointer and provide functions
> > for the different quirks.
> I read the rapl_check_hw_unit() as: read MSR_RAPL_POWER_UNIT, apply
> quirk if need, then estimate timer rate.
> 
> In case to refine struct intel_rapl_init_fun adding callback, then
> either the quirk moving outside the rapl_check_hw_unit(), or replace
> input parameter as whole rapl_init in order to assess quirk callback, by
> far it looks to me centralize these two quirks inside this function more
> easily to maintain.

If you have more than 3 quirks then the function becomes completely
unreadable while with a function pointer nobody has to touch it when adding
a new quirk. Neither do you have to update enums.

> > Zero ininitalization has no real value other than consuming state space.
> To enable more than one quirk I extended bool to enum, I thought the
> __initconst space would be freed after kernel initialized, is there more
> detail concern I missed?

I meant screen space. What's the point of zero initialization other than
consuming code lines and providing zero information?

But instead of arguing with you in circles I took the 5 minutes to make it
function pointer based. Patch below.

Thanks,

	tglx

8<-------------------

Subject: x86/perf/rapl: Make quirk a function pointer
From: Thomas Gleixner <tglx@linutronix.de>
Date: Fri, 09 Sep 2016 22:46:17 +0200

There are more model specific quirks required. So we need to change the
single purpose boolean quirk flag to an easy extensible mechanism.

Make the quirk a function pointer and move the existing quirk into its own
function.

While at it make the init struct initializers readable and rename the
misnomed intel_rapl_hw_init_fun struct to intel_rapl_model_desc because
that's what it is a cpu model descriptor for the rapl features specific to
a particular model.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/events/intel/rapl.c |   92 +++++++++++++++++++++----------------------
 1 file changed, 46 insertions(+), 46 deletions(-)

--- a/arch/x86/events/intel/rapl.c
+++ b/arch/x86/events/intel/rapl.c
@@ -152,6 +152,12 @@ struct rapl_pmus {
 	struct rapl_pmu		*pmus[];
 };
 
+struct intel_rapl_model_desc {
+	void			(*quirk)(void);;
+	int			cntr_mask;
+	struct attribute	**attrs;
+};
+
  /* 1/2^hw_unit Joule */
 static int rapl_hw_unit[NR_RAPL_DOMAINS] __read_mostly;
 static struct rapl_pmus *rapl_pmus;
@@ -617,7 +623,18 @@ static int rapl_cpu_prepare(unsigned int
 	return 0;
 }
 
-static int rapl_check_hw_unit(bool apply_quirk)
+static void rapl_hsx_quirk(void)
+{
+	/*
+	 * 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 "
+	 */
+	rapl_hw_unit[RAPL_IDX_RAM_NRG_STAT] = 16;
+}
+
+static int rapl_check_hw_unit(const struct intel_rapl_model_desc *model)
 {
 	u64 msr_rapl_power_unit_bits;
 	int i;
@@ -628,14 +645,9 @@ static int rapl_check_hw_unit(bool apply
 	for (i = 0; i < NR_RAPL_DOMAINS; i++)
 		rapl_hw_unit[i] = (msr_rapl_power_unit_bits >> 8) & 0x1FULL;
 
-	/*
-	 * 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 (apply_quirk)
-		rapl_hw_unit[RAPL_IDX_RAM_NRG_STAT] = 16;
+	/* Apply quirk before initializing the timer rate */
+	if (model->quirk)
+		model->quirk();
 
 	/*
 	 * Calculate the timer rate:
@@ -701,46 +713,36 @@ static int __init init_rapl_pmus(void)
 #define X86_RAPL_MODEL_MATCH(model, init)	\
 	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (unsigned long)&init }
 
-struct intel_rapl_init_fun {
-	bool apply_quirk;
-	int cntr_mask;
-	struct attribute **attrs;
-};
-
-static const struct intel_rapl_init_fun snb_rapl_init __initconst = {
-	.apply_quirk = false,
-	.cntr_mask = RAPL_IDX_CLN,
-	.attrs = rapl_events_cln_attr,
+static const struct intel_rapl_model_desc snb_rapl_init __initconst = {
+	.cntr_mask	= RAPL_IDX_CLN,
+	.attrs		= rapl_events_cln_attr,
 };
 
-static const struct intel_rapl_init_fun hsx_rapl_init __initconst = {
-	.apply_quirk = true,
-	.cntr_mask = RAPL_IDX_SRV,
-	.attrs = rapl_events_srv_attr,
+static const struct intel_rapl_model_desc hsx_rapl_init __initconst = {
+	.quirk		= rapl_hsx_quirk,
+	.cntr_mask	= RAPL_IDX_SRV,
+	.attrs		= rapl_events_srv_attr,
 };
 
-static const struct intel_rapl_init_fun hsw_rapl_init __initconst = {
-	.apply_quirk = false,
-	.cntr_mask = RAPL_IDX_HSW,
-	.attrs = rapl_events_hsw_attr,
+static const struct intel_rapl_model_desc hsw_rapl_init __initconst = {
+	.cntr_mask	= RAPL_IDX_HSW,
+	.attrs		= rapl_events_hsw_attr,
 };
 
-static const struct intel_rapl_init_fun snbep_rapl_init __initconst = {
-	.apply_quirk = false,
-	.cntr_mask = RAPL_IDX_SRV,
-	.attrs = rapl_events_srv_attr,
+static const struct intel_rapl_model_desc snbep_rapl_init __initconst = {
+	.cntr_mask	= RAPL_IDX_SRV,
+	.attrs		= rapl_events_srv_attr,
 };
 
-static const struct intel_rapl_init_fun knl_rapl_init __initconst = {
-	.apply_quirk = true,
-	.cntr_mask = RAPL_IDX_KNL,
-	.attrs = rapl_events_knl_attr,
+static const struct intel_rapl_model_desc knl_rapl_init __initconst = {
+	.quirk		= rapl_hsx_quirk,
+	.cntr_mask	= RAPL_IDX_KNL,
+	.attrs		= rapl_events_knl_attr,
 };
 
-static const struct intel_rapl_init_fun skl_rapl_init __initconst = {
-	.apply_quirk = false,
-	.cntr_mask = RAPL_IDX_SKL_CLN,
-	.attrs = rapl_events_skl_attr,
+static const struct intel_rapl_model_desc skl_rapl_init __initconst = {
+	.cntr_mask	= RAPL_IDX_SKL_CLN,
+	.attrs		= rapl_events_skl_attr,
 };
 
 static const struct x86_cpu_id rapl_cpu_match[] __initconst = {
@@ -772,21 +774,19 @@ MODULE_DEVICE_TABLE(x86cpu, rapl_cpu_mat
 
 static int __init rapl_pmu_init(void)
 {
+	const struct intel_rapl_model_desc *model;
 	const struct x86_cpu_id *id;
-	struct intel_rapl_init_fun *rapl_init;
-	bool apply_quirk;
 	int ret;
 
 	id = x86_match_cpu(rapl_cpu_match);
 	if (!id)
 		return -ENODEV;
 
-	rapl_init = (struct intel_rapl_init_fun *)id->driver_data;
-	apply_quirk = rapl_init->apply_quirk;
-	rapl_cntr_mask = rapl_init->cntr_mask;
-	rapl_pmu_events_group.attrs = rapl_init->attrs;
+	model = (struct intel_rapl_model_desc*)id->driver_data;
+	rapl_cntr_mask = model->cntr_mask;
+	rapl_pmu_events_group.attrs = model->attrs;
 
-	ret = rapl_check_hw_unit(apply_quirk);
+	ret = rapl_check_hw_unit(model);
 	if (ret)
 		return ret;
 

Re: [PATCH 2/2] perf/x86/rapl: Enable Baytrail/Braswell RAPL support

[Pan, Harry]

Hi Thomas,

Appreciate comments, I understood and learned.
I just uploaded 3 patches integrated yours, yet there was 'git am'
failure thus I did hand work, kindly double check.

Sincerely,
Harry

On Fri, 2016-09-09 at 23:21 +0200, Thomas Gleixner wrote:
> On Fri, 9 Sep 2016, Pan, Harry wrote:
> > On Fri, 2016-09-09 at 17:11 +0200, Thomas Gleixner wrote:
> > > >  struct intel_rapl_init_fun {
> > > > -	bool apply_quirk;
> > > > +	enum rapl_quirk apply_quirk;
> > > 
> > > This is silly. Make apply_quirk a function pointer and provide functions
> > > for the different quirks.
> > I read the rapl_check_hw_unit() as: read MSR_RAPL_POWER_UNIT, apply
> > quirk if need, then estimate timer rate.
> > 
> > In case to refine struct intel_rapl_init_fun adding callback, then
> > either the quirk moving outside the rapl_check_hw_unit(), or replace
> > input parameter as whole rapl_init in order to assess quirk callback, by
> > far it looks to me centralize these two quirks inside this function more
> > easily to maintain.
> 
> If you have more than 3 quirks then the function becomes completely
> unreadable while with a function pointer nobody has to touch it when adding
> a new quirk. Neither do you have to update enums.
> 
> > > Zero ininitalization has no real value other than consuming state space.
> > To enable more than one quirk I extended bool to enum, I thought the
> > __initconst space would be freed after kernel initialized, is there more
> > detail concern I missed?
> 
> I meant screen space. What's the point of zero initialization other than
> consuming code lines and providing zero information?
> 
> But instead of arguing with you in circles I took the 5 minutes to make it
> function pointer based. Patch below.
> 
> Thanks,
> 
> 	tglx
> 
> 8<-------------------
> 
> Subject: x86/perf/rapl: Make quirk a function pointer
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Fri, 09 Sep 2016 22:46:17 +0200
> 
> There are more model specific quirks required. So we need to change the
> single purpose boolean quirk flag to an easy extensible mechanism.
> 
> Make the quirk a function pointer and move the existing quirk into its own
> function.
> 
> While at it make the init struct initializers readable and rename the
> misnomed intel_rapl_hw_init_fun struct to intel_rapl_model_desc because
> that's what it is a cpu model descriptor for the rapl features specific to
> a particular model.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/events/intel/rapl.c |   92 +++++++++++++++++++++----------------------
>  1 file changed, 46 insertions(+), 46 deletions(-)
> 
> --- a/arch/x86/events/intel/rapl.c
> +++ b/arch/x86/events/intel/rapl.c
> @@ -152,6 +152,12 @@ struct rapl_pmus {
>  	struct rapl_pmu		*pmus[];
>  };
>  
> +struct intel_rapl_model_desc {
> +	void			(*quirk)(void);;
> +	int			cntr_mask;
> +	struct attribute	**attrs;
> +};
> +
>   /* 1/2^hw_unit Joule */
>  static int rapl_hw_unit[NR_RAPL_DOMAINS] __read_mostly;
>  static struct rapl_pmus *rapl_pmus;
> @@ -617,7 +623,18 @@ static int rapl_cpu_prepare(unsigned int
>  	return 0;
>  }
>  
> -static int rapl_check_hw_unit(bool apply_quirk)
> +static void rapl_hsx_quirk(void)
> +{
> +	/*
> +	 * 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 "
> +	 */
> +	rapl_hw_unit[RAPL_IDX_RAM_NRG_STAT] = 16;
> +}
> +
> +static int rapl_check_hw_unit(const struct intel_rapl_model_desc *model)
>  {
>  	u64 msr_rapl_power_unit_bits;
>  	int i;
> @@ -628,14 +645,9 @@ static int rapl_check_hw_unit(bool apply
>  	for (i = 0; i < NR_RAPL_DOMAINS; i++)
>  		rapl_hw_unit[i] = (msr_rapl_power_unit_bits >> 8) & 0x1FULL;
>  
> -	/*
> -	 * 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 (apply_quirk)
> -		rapl_hw_unit[RAPL_IDX_RAM_NRG_STAT] = 16;
> +	/* Apply quirk before initializing the timer rate */
> +	if (model->quirk)
> +		model->quirk();
>  
>  	/*
>  	 * Calculate the timer rate:
> @@ -701,46 +713,36 @@ static int __init init_rapl_pmus(void)
>  #define X86_RAPL_MODEL_MATCH(model, init)	\
>  	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (unsigned long)&init }
>  
> -struct intel_rapl_init_fun {
> -	bool apply_quirk;
> -	int cntr_mask;
> -	struct attribute **attrs;
> -};
> -
> -static const struct intel_rapl_init_fun snb_rapl_init __initconst = {
> -	.apply_quirk = false,
> -	.cntr_mask = RAPL_IDX_CLN,
> -	.attrs = rapl_events_cln_attr,
> +static const struct intel_rapl_model_desc snb_rapl_init __initconst = {
> +	.cntr_mask	= RAPL_IDX_CLN,
> +	.attrs		= rapl_events_cln_attr,
>  };
>  
> -static const struct intel_rapl_init_fun hsx_rapl_init __initconst = {
> -	.apply_quirk = true,
> -	.cntr_mask = RAPL_IDX_SRV,
> -	.attrs = rapl_events_srv_attr,
> +static const struct intel_rapl_model_desc hsx_rapl_init __initconst = {
> +	.quirk		= rapl_hsx_quirk,
> +	.cntr_mask	= RAPL_IDX_SRV,
> +	.attrs		= rapl_events_srv_attr,
>  };
>  
> -static const struct intel_rapl_init_fun hsw_rapl_init __initconst = {
> -	.apply_quirk = false,
> -	.cntr_mask = RAPL_IDX_HSW,
> -	.attrs = rapl_events_hsw_attr,
> +static const struct intel_rapl_model_desc hsw_rapl_init __initconst = {
> +	.cntr_mask	= RAPL_IDX_HSW,
> +	.attrs		= rapl_events_hsw_attr,
>  };
>  
> -static const struct intel_rapl_init_fun snbep_rapl_init __initconst = {
> -	.apply_quirk = false,
> -	.cntr_mask = RAPL_IDX_SRV,
> -	.attrs = rapl_events_srv_attr,
> +static const struct intel_rapl_model_desc snbep_rapl_init __initconst = {
> +	.cntr_mask	= RAPL_IDX_SRV,
> +	.attrs		= rapl_events_srv_attr,
>  };
>  
> -static const struct intel_rapl_init_fun knl_rapl_init __initconst = {
> -	.apply_quirk = true,
> -	.cntr_mask = RAPL_IDX_KNL,
> -	.attrs = rapl_events_knl_attr,
> +static const struct intel_rapl_model_desc knl_rapl_init __initconst = {
> +	.quirk		= rapl_hsx_quirk,
> +	.cntr_mask	= RAPL_IDX_KNL,
> +	.attrs		= rapl_events_knl_attr,
>  };
>  
> -static const struct intel_rapl_init_fun skl_rapl_init __initconst = {
> -	.apply_quirk = false,
> -	.cntr_mask = RAPL_IDX_SKL_CLN,
> -	.attrs = rapl_events_skl_attr,
> +static const struct intel_rapl_model_desc skl_rapl_init __initconst = {
> +	.cntr_mask	= RAPL_IDX_SKL_CLN,
> +	.attrs		= rapl_events_skl_attr,
>  };
>  
>  static const struct x86_cpu_id rapl_cpu_match[] __initconst = {
> @@ -772,21 +774,19 @@ MODULE_DEVICE_TABLE(x86cpu, rapl_cpu_mat
>  
>  static int __init rapl_pmu_init(void)
>  {
> +	const struct intel_rapl_model_desc *model;
>  	const struct x86_cpu_id *id;
> -	struct intel_rapl_init_fun *rapl_init;
> -	bool apply_quirk;
>  	int ret;
>  
>  	id = x86_match_cpu(rapl_cpu_match);
>  	if (!id)
>  		return -ENODEV;
>  
> -	rapl_init = (struct intel_rapl_init_fun *)id->driver_data;
> -	apply_quirk = rapl_init->apply_quirk;
> -	rapl_cntr_mask = rapl_init->cntr_mask;
> -	rapl_pmu_events_group.attrs = rapl_init->attrs;
> +	model = (struct intel_rapl_model_desc*)id->driver_data;
> +	rapl_cntr_mask = model->cntr_mask;
> +	rapl_pmu_events_group.attrs = model->attrs;
>  
> -	ret = rapl_check_hw_unit(apply_quirk);
> +	ret = rapl_check_hw_unit(model);
>  	if (ret)
>  		return ret;
>  

Re: [PATCH 2/2] perf/x86/rapl: Enable Baytrail/Braswell RAPL support

[Pan, Harry]

My apology that I resent again in order to clean my junk of useless
enum, and ignore ambiguous in-reply-to (2016/9/11/8) due to subject.

new patchset is here:
https://lkml.org/lkml/2016/9/11/11
https://lkml.org/lkml/2016/9/11/12
https://lkml.org/lkml/2016/9/11/10

Thanks,
Harry

[PATCH 1/2] perf/x86/rapl: Enable Apollo Lake RAPL support

[Harry Pan]

This patch enables RAPL counters (energy consumption counters)
support for Intel Apollo Lake (Goldmont) processors (Model 92):

RAPL of Goldmont, unlikes ESU increment of Silvermont/Airmont,
it likes the Haswell microarchitecture in 1/2^ESU joules and
supports power domains in PP0/PP1/PKG/RAM.

ESU and power domains refer to Intel Software Developers' Manual,
Vol. 3C, Order No. 325384, Table 35-12.

Usage example:

$ perf list
$ perf stat -a -e power/energy-cores/,power/energy-pkg/ sleep 10

Signed-off-by: Harry Pan <harry.pan@intel.com>
---
 arch/x86/events/intel/rapl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
index 2886593..f7924640 100644
--- a/arch/x86/events/intel/rapl.c
+++ b/arch/x86/events/intel/rapl.c
@@ -765,6 +765,8 @@ static const struct x86_cpu_id rapl_cpu_match[] __initconst = {
 	X86_RAPL_MODEL_MATCH(INTEL_FAM6_SKYLAKE_MOBILE,  skl_rapl_init),
 	X86_RAPL_MODEL_MATCH(INTEL_FAM6_SKYLAKE_DESKTOP, skl_rapl_init),
 	X86_RAPL_MODEL_MATCH(INTEL_FAM6_SKYLAKE_X,	 hsx_rapl_init),
+
+	X86_RAPL_MODEL_MATCH(INTEL_FAM6_ATOM_GOLDMONT, hsw_rapl_init),
 	{},
 };
 
-- 
2.6.6

[PATCH 1/2] perf/x86/rapl: Enable Apollo Lake RAPL support

[Harry Pan]

This patch enables RAPL counters (energy consumption counters)
support for Intel Apollo Lake (Goldmont) processors (Model 92):

RAPL of Goldmont, unlikes ESU increment of Silvermont/Airmont,
it likes the Haswell microarchitecture in 1/2^ESU joules and
supports power domains in PP0/PP1/PKG/RAM.

ESU and power domains refer to Intel Software Developers' Manual,
Vol. 3C, Order No. 325384, Table 35-12.

Usage example:

$ perf list
$ perf stat -a -e power/energy-cores/,power/energy-pkg/ sleep 10

Signed-off-by: Harry Pan <harry.pan@intel.com>
---
 arch/x86/events/intel/rapl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
index 2886593..f7924640 100644
--- a/arch/x86/events/intel/rapl.c
+++ b/arch/x86/events/intel/rapl.c
@@ -765,6 +765,8 @@ static const struct x86_cpu_id rapl_cpu_match[] __initconst = {
 	X86_RAPL_MODEL_MATCH(INTEL_FAM6_SKYLAKE_MOBILE,  skl_rapl_init),
 	X86_RAPL_MODEL_MATCH(INTEL_FAM6_SKYLAKE_DESKTOP, skl_rapl_init),
 	X86_RAPL_MODEL_MATCH(INTEL_FAM6_SKYLAKE_X,	 hsx_rapl_init),
+
+	X86_RAPL_MODEL_MATCH(INTEL_FAM6_ATOM_GOLDMONT, hsw_rapl_init),
 	{},
 };
 
-- 
2.6.6