Skip to contentSkip to conversation

perf/x86/rapl: Enable Apollo Lake RAPL support

PATCH In review Harry Pan 2 participants 7 messages Sep 9, 2016 – Sep 11, 2016

Subsystems (2)
x86 perf events
lore
Share & tools
lore

Harry Pan

author
#0 lore raw

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

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 <[email protected]>
---
 arch/x86/events/intel/rapl.c | 2 ++
 1 file changed, 2 insertions(+)
arch/x86/events/intel/rapl.c +2 -0
index 2886593..f7924640 100644
@@ -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),
 	{},
 };
Signature · 1 line
2.6.6
author
#1 lore raw

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

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 <[email protected]>
---
 arch/x86/events/intel/rapl.c | 68 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 58 insertions(+), 10 deletions(-)
arch/x86/events/intel/rapl.c +58 -10
index f7924640..328fea4 100644
@@ -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);
Signature · 1 line
2.6.6
#2 lore raw

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

arch/x86/events/intel/rapl.c static int rapl_check_hw_unit(bool apply_quirk) · line 678

Quoted 13 lines · Harry Pan
arch/x86/events/intel/rapl.c +9 -1
-	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.

arch/x86/events/intel/rapl.c static int __init init_rapl_pmus(void) · line 745

Quoted 10 lines
arch/x86/events/intel/rapl.c +2 -1
+	/*
 	 * 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.

arch/x86/events/intel/rapl.c static int __init init_rapl_pmus(void) · line 751

Quoted 7 lines
arch/x86/events/intel/rapl.c +1 -1
 	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

author
#3 lore raw

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

I refined/uploaded again, kindly advise.

Quoted 5 lines · Thomas Gleixner
Quoted 4 lines · Harry Pan
arch/x86/events/intel/rapl.c +1 -1
-	if (apply_quirk)
+	if (apply_quirk == RAPL_HSX_QUIRK)
 		rapl_hw_unit[RAPL_IDX_RAM_NRG_STAT] = 16;
Quoted 11 lines

arch/x86/events/intel/rapl.c static int rapl_check_hw_unit(bool apply_quirk) · line 678

Quoted 9 lines
arch/x86/events/intel/rapl.c +8 -0
 	/*
+	 * 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.

Quoted 8 lines
Quoted 8 lines
arch/x86/events/intel/rapl.c +1 -0
+	/*
 	 * 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 }
Quoted 6 lines

arch/x86/events/intel/rapl.c static int __init init_rapl_pmus(void) · line 745

Quoted 3 lines
arch/x86/events/intel/rapl.c +1 -1
 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.

Quoted 10 lines

arch/x86/events/intel/rapl.c static int __init init_rapl_pmus(void) · line 751

Quoted 8 lines
arch/x86/events/intel/rapl.c +1 -1
 	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

#4 lore raw

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

Quoted 7 lines · Harry
Quoted 6 lines · Thomas Gleixner

Review

Quoted 3 lines

struct intel_rapl_init_fun {

(quoted diff) +1 -1
-	bool apply_quirk;
+	enum rapl_quirk apply_quirk;

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

Quoted 8 lines

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.

Quoted 4 lines
Quoted 1 line

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 <[email protected]>
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 <[email protected]>
---
 arch/x86/events/intel/rapl.c |   92 +++++++++++++++++++++----------------------
 1 file changed, 46 insertions(+), 46 deletions(-)
arch/x86/events/intel/rapl.c +46 -46
@@ -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;
author
#5 lore raw

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

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

Quoted 205 lines · Thomas Gleixner
Quoted 7 lines · Harry
Quoted 6 lines · Thomas Gleixner

Review

Quoted 3 lines

struct intel_rapl_init_fun {

(quoted diff) +1 -1
-	bool apply_quirk;
+	enum rapl_quirk apply_quirk;

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

Quoted 8 lines

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.

Quoted 4 lines
Quoted 1 line

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 <[email protected]>
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 <[email protected]>
---
 arch/x86/events/intel/rapl.c |   92 +++++++++++++++++++++----------------------
 1 file changed, 46 insertions(+), 46 deletions(-)
arch/x86/events/intel/rapl.c +46 -46
@@ -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;