[PATCH 0/3] perf/x86/rapl: Enable RAPL for AMD Fam17h

[PATCH 0/3] perf/x86/rapl: Enable RAPL for AMD Fam17h

[Stephane Eranian]

This patch series adds support for AMD Fam17h RAPL counters. As per
AMD PPR, Fam17h support Package RAPL counters to monitor power usage.
The RAPL counter operates as with Intel RAPL. As such, it is beneficial
to share the code.

The series first moves the rapl.c file to common perf_events x86 and then
adds the support.
From the user's point of view, the interface is identical with
/sys/devices/power. The energy-pkg event is the only one supported.

$ perf stat -a --per-socket -I 1000 -e power/energy-pkg/

Signed-off-by: Stephane Eranian <eranian@google.com>

Stephane Eranian (3):
  perf/x86/rapl: move RAPL support to common x86 code
  perf/x86/rapl: refactor code for Intel/AMD sharing
  perf/x86/rapl: add AMD Fam17h RAPL support

 arch/x86/events/Kconfig            |  8 ++---
 arch/x86/events/Makefile           |  1 +
 arch/x86/events/intel/Makefile     |  2 --
 arch/x86/events/probe.c            |  4 +++
 arch/x86/events/{intel => }/rapl.c | 55 +++++++++++++++++++++++++-----
 arch/x86/include/asm/msr-index.h   |  3 ++
 6 files changed, 58 insertions(+), 15 deletions(-)
 rename arch/x86/events/{intel => }/rapl.c (92%)

-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 1/3] perf/x86/rapl: move RAPL support to common x86 code

[Stephane Eranian]

To prepare for support of both Intel and AMD RAPL.
Move rapl.c to arch/x86/events. Rename config option.
Fixup header paths.

Signed-off-by: Stephane Eranian <eranian@google.com>
---
 arch/x86/events/Kconfig            | 8 ++++----
 arch/x86/events/Makefile           | 1 +
 arch/x86/events/intel/Makefile     | 2 --
 arch/x86/events/{intel => }/rapl.c | 9 ++++++---
 4 files changed, 11 insertions(+), 9 deletions(-)
 rename arch/x86/events/{intel => }/rapl.c (98%)

diff --git a/arch/x86/events/Kconfig b/arch/x86/events/Kconfig
index 9a7a1446cb3a0..e542c32b0a55f 100644
--- a/arch/x86/events/Kconfig
+++ b/arch/x86/events/Kconfig
@@ -9,12 +9,12 @@ config PERF_EVENTS_INTEL_UNCORE
 	Include support for Intel uncore performance events. These are
 	available on NehalemEX and more modern processors.
 
-config PERF_EVENTS_INTEL_RAPL
-	tristate "Intel rapl performance events"
-	depends on PERF_EVENTS && CPU_SUP_INTEL && PCI
+config PERF_EVENTS_X86_RAPL
+	tristate "RAPL performance events"
+	depends on PERF_EVENTS && (CPU_SUP_INTEL || CPU_SUP_AMD) && PCI
 	default y
 	---help---
-	Include support for Intel rapl performance events for power
+	Include support for Intel and AMD rapl performance events for power
 	monitoring on modern processors.
 
 config PERF_EVENTS_INTEL_CSTATE
diff --git a/arch/x86/events/Makefile b/arch/x86/events/Makefile
index 6f1d1fde8b2de..d5087a5745108 100644
--- a/arch/x86/events/Makefile
+++ b/arch/x86/events/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-y					+= core.o probe.o
+obj-$(CONFIG_PERF_EVENTS_X86_RAPL)	+= rapl.o
 obj-y					+= amd/
 obj-$(CONFIG_X86_LOCAL_APIC)            += msr.o
 obj-$(CONFIG_CPU_SUP_INTEL)		+= intel/
diff --git a/arch/x86/events/intel/Makefile b/arch/x86/events/intel/Makefile
index 3468b0c1dc7c9..e67a5886336c1 100644
--- a/arch/x86/events/intel/Makefile
+++ b/arch/x86/events/intel/Makefile
@@ -2,8 +2,6 @@
 obj-$(CONFIG_CPU_SUP_INTEL)		+= core.o bts.o
 obj-$(CONFIG_CPU_SUP_INTEL)		+= ds.o knc.o
 obj-$(CONFIG_CPU_SUP_INTEL)		+= lbr.o p4.o p6.o pt.o
-obj-$(CONFIG_PERF_EVENTS_INTEL_RAPL)	+= intel-rapl-perf.o
-intel-rapl-perf-objs			:= rapl.o
 obj-$(CONFIG_PERF_EVENTS_INTEL_UNCORE)	+= intel-uncore.o
 intel-uncore-objs			:= uncore.o uncore_nhmex.o uncore_snb.o uncore_snbep.o
 obj-$(CONFIG_PERF_EVENTS_INTEL_CSTATE)	+= intel-cstate.o
diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/rapl.c
similarity index 98%
rename from arch/x86/events/intel/rapl.c
rename to arch/x86/events/rapl.c
index a5dbd25852cb7..ece043fb7b494 100644
--- a/arch/x86/events/intel/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -1,11 +1,14 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Support Intel RAPL energy consumption counters
+ * Support Intel/AMD RAPL energy consumption counters
  * Copyright (C) 2013 Google, Inc., Stephane Eranian
  *
  * Intel RAPL interface is specified in the IA-32 Manual Vol3b
  * section 14.7.1 (September 2013)
  *
+ * AMD RAPL interface for Fam17h is described in the public PPR:
+ * https://bugzilla.kernel.org/show_bug.cgi?id=206537
+ *
  * RAPL provides more controls than just reporting energy consumption
  * however here we only expose the 3 energy consumption free running
  * counters (pp0, pkg, dram).
@@ -58,8 +61,8 @@
 #include <linux/nospec.h>
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
-#include "../perf_event.h"
-#include "../probe.h"
+#include "perf_event.h"
+#include "probe.h"
 
 MODULE_LICENSE("GPL");
 
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 2/3] perf/x86/rapl: refactor code for Intel/AMD sharing

[Stephane Eranian]

This patch modifies the rapl_model struct to include architecture specific
knowledge to Intel specific structure, and in particular the MSR for
POWER_UNIT and the rapl_msrs array.

No functional changes.

Signed-off-by: Stephane Eranian <eranian@google.com>
---
 arch/x86/events/rapl.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index ece043fb7b494..e98f627a13fa8 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -131,7 +131,9 @@ struct rapl_pmus {
 };
 
 struct rapl_model {
+	struct perf_msr *rapl_msrs;
 	unsigned long	events;
+	int		msr_power_unit;
 	bool		apply_quirk;
 };
 
@@ -141,7 +143,7 @@ static struct rapl_pmus *rapl_pmus;
 static cpumask_t rapl_cpu_mask;
 static unsigned int rapl_cntr_mask;
 static u64 rapl_timer_ms;
-static struct perf_msr rapl_msrs[];
+static struct perf_msr *rapl_msrs;
 
 static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int cpu)
 {
@@ -516,7 +518,7 @@ static bool test_msr(int idx, void *data)
 	return test_bit(idx, (unsigned long *) data);
 }
 
-static struct perf_msr rapl_msrs[] = {
+static struct perf_msr intel_rapl_msrs[] = {
 	[PERF_RAPL_PP0]  = { MSR_PP0_ENERGY_STATUS,      &rapl_events_cores_group, test_msr },
 	[PERF_RAPL_PKG]  = { MSR_PKG_ENERGY_STATUS,      &rapl_events_pkg_group,   test_msr },
 	[PERF_RAPL_RAM]  = { MSR_DRAM_ENERGY_STATUS,     &rapl_events_ram_group,   test_msr },
@@ -578,13 +580,13 @@ static int rapl_cpu_online(unsigned int cpu)
 	return 0;
 }
 
-static int rapl_check_hw_unit(bool apply_quirk)
+static int rapl_check_hw_unit(struct rapl_model *rm)
 {
 	u64 msr_rapl_power_unit_bits;
 	int i;
 
 	/* protect rdmsrl() to handle virtualization */
-	if (rdmsrl_safe(MSR_RAPL_POWER_UNIT, &msr_rapl_power_unit_bits))
+	if (rdmsrl_safe(rm->msr_power_unit, &msr_rapl_power_unit_bits))
 		return -1;
 	for (i = 0; i < NR_RAPL_DOMAINS; i++)
 		rapl_hw_unit[i] = (msr_rapl_power_unit_bits >> 8) & 0x1FULL;
@@ -595,7 +597,7 @@ 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 (rm->apply_quirk)
 		rapl_hw_unit[PERF_RAPL_RAM] = 16;
 
 	/*
@@ -676,6 +678,8 @@ static struct rapl_model model_snb = {
 			  BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_PP1),
 	.apply_quirk	= false,
+	.msr_power_unit = MSR_RAPL_POWER_UNIT,
+	.rapl_msrs      = intel_rapl_msrs,
 };
 
 static struct rapl_model model_snbep = {
@@ -683,6 +687,8 @@ static struct rapl_model model_snbep = {
 			  BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_RAM),
 	.apply_quirk	= false,
+	.msr_power_unit = MSR_RAPL_POWER_UNIT,
+	.rapl_msrs      = intel_rapl_msrs,
 };
 
 static struct rapl_model model_hsw = {
@@ -691,6 +697,8 @@ static struct rapl_model model_hsw = {
 			  BIT(PERF_RAPL_RAM) |
 			  BIT(PERF_RAPL_PP1),
 	.apply_quirk	= false,
+	.msr_power_unit = MSR_RAPL_POWER_UNIT,
+	.rapl_msrs      = intel_rapl_msrs,
 };
 
 static struct rapl_model model_hsx = {
@@ -698,12 +706,16 @@ static struct rapl_model model_hsx = {
 			  BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_RAM),
 	.apply_quirk	= true,
+	.msr_power_unit = MSR_RAPL_POWER_UNIT,
+	.rapl_msrs      = intel_rapl_msrs,
 };
 
 static struct rapl_model model_knl = {
 	.events		= BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_RAM),
 	.apply_quirk	= true,
+	.msr_power_unit = MSR_RAPL_POWER_UNIT,
+	.rapl_msrs      = intel_rapl_msrs,
 };
 
 static struct rapl_model model_skl = {
@@ -713,6 +725,8 @@ static struct rapl_model model_skl = {
 			  BIT(PERF_RAPL_PP1) |
 			  BIT(PERF_RAPL_PSYS),
 	.apply_quirk	= false,
+	.msr_power_unit = MSR_RAPL_POWER_UNIT,
+	.rapl_msrs      = intel_rapl_msrs,
 };
 
 static const struct x86_cpu_id rapl_model_match[] __initconst = {
@@ -758,10 +772,13 @@ static int __init rapl_pmu_init(void)
 		return -ENODEV;
 
 	rm = (struct rapl_model *) id->driver_data;
+
+	rapl_msrs = rm->rapl_msrs;
+
 	rapl_cntr_mask = perf_msr_probe(rapl_msrs, PERF_RAPL_MAX,
 					false, (void *) &rm->events);
 
-	ret = rapl_check_hw_unit(rm->apply_quirk);
+	ret = rapl_check_hw_unit(rm);
 	if (ret)
 		return ret;
 
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 3/3] perf/x86/rapl: add AMD Fam17h RAPL support

[Stephane Eranian]

This patch enables AMD Fam17h RAPL support for the Package level metric.
The support is as per AMD Fam17h Model31h (Zen2) and model 00-ffh (Zen1) PPR.

The same output is available via the energy-pkg pseudo event:

$ perf stat -a -I 1000 --per-socket -e power/energy-pkg/

Signed-off-by: Stephane Eranian <eranian@google.com>
---
 arch/x86/events/probe.c          |  4 ++++
 arch/x86/events/rapl.c           | 17 +++++++++++++++++
 arch/x86/include/asm/msr-index.h |  3 +++
 3 files changed, 24 insertions(+)

diff --git a/arch/x86/events/probe.c b/arch/x86/events/probe.c
index c2ede2f3b2770..b3a9df2e48dfa 100644
--- a/arch/x86/events/probe.c
+++ b/arch/x86/events/probe.c
@@ -26,6 +26,10 @@ perf_msr_probe(struct perf_msr *msr, int cnt, bool zero, void *data)
 
 			grp->is_visible = not_visible;
 
+			/* avoid unpopulated entries */
+			if (!msr[bit].msr)
+				continue;
+
 			if (msr[bit].test && !msr[bit].test(bit, data))
 				continue;
 			/* Virt sucks; you cannot tell if a R/O MSR is present :/ */
diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index e98f627a13fa8..47ff20dfde889 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -526,6 +526,15 @@ static struct perf_msr intel_rapl_msrs[] = {
 	[PERF_RAPL_PSYS] = { MSR_PLATFORM_ENERGY_STATUS, &rapl_events_psys_group,  test_msr },
 };
 
+static struct perf_msr amd_rapl_msrs[] = {
+	[PERF_RAPL_PP0]  = { 0, &rapl_events_cores_group, NULL},
+	[PERF_RAPL_PKG]  = { MSR_AMD_PKG_ENERGY_STATUS,  &rapl_events_pkg_group,   test_msr },
+	[PERF_RAPL_RAM]  = { 0, &rapl_events_ram_group,   NULL},
+	[PERF_RAPL_PP1]  = { 0, &rapl_events_gpu_group,   NULL},
+	[PERF_RAPL_PSYS] = { 0, &rapl_events_psys_group,  NULL},
+};
+
+
 static int rapl_cpu_offline(unsigned int cpu)
 {
 	struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);
@@ -729,6 +738,13 @@ static struct rapl_model model_skl = {
 	.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,
+};
+
 static const struct x86_cpu_id rapl_model_match[] __initconst = {
 	X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE,		&model_snb),
 	X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE_X,	&model_snbep),
@@ -757,6 +773,7 @@ static const struct x86_cpu_id rapl_model_match[] __initconst = {
 	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE,		&model_skl),
 	X86_MATCH_INTEL_FAM6_MODEL(COMETLAKE_L,		&model_skl),
 	X86_MATCH_INTEL_FAM6_MODEL(COMETLAKE,		&model_skl),
+	X86_MATCH_VENDOR_FAM(AMD, 0x17, &model_amd_fam17h),
 	{},
 };
 MODULE_DEVICE_TABLE(x86cpu, rapl_model_match);
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 12c9684d59ba6..ef452b817f44f 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -301,6 +301,9 @@
 #define MSR_PP1_ENERGY_STATUS		0x00000641
 #define MSR_PP1_POLICY			0x00000642
 
+#define MSR_AMD_PKG_ENERGY_STATUS	0xc001029b
+#define MSR_AMD_RAPL_POWER_UNIT		0xc0010299
+
 /* Config TDP MSRs */
 #define MSR_CONFIG_TDP_NOMINAL		0x00000648
 #define MSR_CONFIG_TDP_LEVEL_1		0x00000649
-- 
2.26.2.761.g0e0b3e54be-goog

Re: [PATCH 0/3] perf/x86/rapl: Enable RAPL for AMD Fam17h

[Alexander Monakov]

On Fri, 15 May 2020, Stephane Eranian wrote:

> The series first moves the rapl.c file to common perf_events x86 and then
> adds the support.
> From the user's point of view, the interface is identical with
> /sys/devices/power. The energy-pkg event is the only one supported.

AMD also has per-core energy metering via MSR 0xc001029a, and I wonder
if you have plans to expose it to perf as well. I see it does not fit
so nicely with the existing code (as it's per-core instead of per-die).

The turbostat tool already exposes these per-core readings:

Core    CPU     Avg_MHz Busy%   Bzy_MHz TSC_MHz CorWatt PkgWatt
-       -       3951    100.00  3951    2373    54.92   30.04
0       0       3945    100.00  3945    2370    8.97    29.98
1       1       3945    100.00  3945    2370    9.11
2       2       3945    100.00  3945    2370    8.96
4       3       3946    100.00  3946    2370    9.32
5       4       3946    100.00  3946    2370    9.11
6       5       3946    100.00  3946    2370    9.39

turbostat sums the per-core energy figures to show the per-socket 54.92W
value. Though as you can see on this example, the figure is not in agreement
with the per-socket MSR you're using in your patch. Maybe the per-core
values are less reliable, but I believe I have a test that shows per-package
figure to be inaccurate as well. It would be nice if AMD clarified how the
counters work.

And, for what (little) it's worth, the series is:

Tested-by: Alexander Monakov <amonakov@ispras.ru>

Thank you.
Alexander

Re: [PATCH 0/3] perf/x86/rapl: Enable RAPL for AMD Fam17h

[Jiri Olsa]

On Fri, May 15, 2020 at 02:57:30PM -0700, Stephane Eranian wrote:
> This patch series adds support for AMD Fam17h RAPL counters. As per
> AMD PPR, Fam17h support Package RAPL counters to monitor power usage.
> The RAPL counter operates as with Intel RAPL. As such, it is beneficial
> to share the code.
> 
> The series first moves the rapl.c file to common perf_events x86 and then
> adds the support.
> From the user's point of view, the interface is identical with
> /sys/devices/power. The energy-pkg event is the only one supported.
> 
> $ perf stat -a --per-socket -I 1000 -e power/energy-pkg/
> 
> Signed-off-by: Stephane Eranian <eranian@google.com>
> 
> Stephane Eranian (3):
>   perf/x86/rapl: move RAPL support to common x86 code
>   perf/x86/rapl: refactor code for Intel/AMD sharing
>   perf/x86/rapl: add AMD Fam17h RAPL support
> 
>  arch/x86/events/Kconfig            |  8 ++---
>  arch/x86/events/Makefile           |  1 +
>  arch/x86/events/intel/Makefile     |  2 --
>  arch/x86/events/probe.c            |  4 +++
>  arch/x86/events/{intel => }/rapl.c | 55 +++++++++++++++++++++++++-----
>  arch/x86/include/asm/msr-index.h   |  3 ++
>  6 files changed, 58 insertions(+), 15 deletions(-)
>  rename arch/x86/events/{intel => }/rapl.c (92%)

looks good to me, intel rapl keeps working for me ;-)

Acked-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

Re: [PATCH 1/3] perf/x86/rapl: move RAPL support to common x86 code

[Peter Zijlstra]

On Fri, May 15, 2020 at 02:57:31PM -0700, Stephane Eranian wrote:
> To prepare for support of both Intel and AMD RAPL.
> Move rapl.c to arch/x86/events. Rename config option.
> Fixup header paths.
> 
> Signed-off-by: Stephane Eranian <eranian@google.com>
> ---
>  arch/x86/events/Kconfig            | 8 ++++----
>  arch/x86/events/Makefile           | 1 +
>  arch/x86/events/intel/Makefile     | 2 --
>  arch/x86/events/{intel => }/rapl.c | 9 ++++++---
>  4 files changed, 11 insertions(+), 9 deletions(-)
>  rename arch/x86/events/{intel => }/rapl.c (98%)
> 
> diff --git a/arch/x86/events/Kconfig b/arch/x86/events/Kconfig
> index 9a7a1446cb3a0..e542c32b0a55f 100644
> --- a/arch/x86/events/Kconfig
> +++ b/arch/x86/events/Kconfig
> @@ -9,12 +9,12 @@ config PERF_EVENTS_INTEL_UNCORE
>  	Include support for Intel uncore performance events. These are
>  	available on NehalemEX and more modern processors.
>  
> -config PERF_EVENTS_INTEL_RAPL
> -	tristate "Intel rapl performance events"
> -	depends on PERF_EVENTS && CPU_SUP_INTEL && PCI
> +config PERF_EVENTS_X86_RAPL

This is going to make everybody's kconfig prompt for this again. Best to
to a backwards compat thing or just leave it have Intel in the name.

Re: [PATCH 2/3] perf/x86/rapl: refactor code for Intel/AMD sharing

[Peter Zijlstra]

On Fri, May 15, 2020 at 02:57:32PM -0700, Stephane Eranian wrote:
> This patch modifies the rapl_model struct to include architecture specific
> knowledge to Intel specific structure, and in particular the MSR for
> POWER_UNIT and the rapl_msrs array.
> 
> No functional changes.
> 
> Signed-off-by: Stephane Eranian <eranian@google.com>
> ---
>  arch/x86/events/rapl.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index ece043fb7b494..e98f627a13fa8 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -131,7 +131,9 @@ struct rapl_pmus {
>  };
>  
>  struct rapl_model {
> +	struct perf_msr *rapl_msrs;
>  	unsigned long	events;
> +	int		msr_power_unit;

MSR addresses go negative these days?

Re: [PATCH 3/3] perf/x86/rapl: add AMD Fam17h RAPL support

[Peter Zijlstra]

On Fri, May 15, 2020 at 02:57:33PM -0700, Stephane Eranian wrote:

> +static struct perf_msr amd_rapl_msrs[] = {
> +	[PERF_RAPL_PP0]  = { 0, &rapl_events_cores_group, NULL},
> +	[PERF_RAPL_PKG]  = { MSR_AMD_PKG_ENERGY_STATUS,  &rapl_events_pkg_group,   test_msr },
> +	[PERF_RAPL_RAM]  = { 0, &rapl_events_ram_group,   NULL},
> +	[PERF_RAPL_PP1]  = { 0, &rapl_events_gpu_group,   NULL},
> +	[PERF_RAPL_PSYS] = { 0, &rapl_events_psys_group,  NULL},
> +};

Why have those !PKG things initialized? Wouldn't they default to 0
anyway? If not, surely { 0, } is sufficient.

Re: [PATCH 3/3] perf/x86/rapl: add AMD Fam17h RAPL support

[Stephane Eranian]

On Mon, May 18, 2020 at 2:34 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, May 15, 2020 at 02:57:33PM -0700, Stephane Eranian wrote:
>
> > +static struct perf_msr amd_rapl_msrs[] = {
> > +     [PERF_RAPL_PP0]  = { 0, &rapl_events_cores_group, NULL},
> > +     [PERF_RAPL_PKG]  = { MSR_AMD_PKG_ENERGY_STATUS,  &rapl_events_pkg_group,   test_msr },
> > +     [PERF_RAPL_RAM]  = { 0, &rapl_events_ram_group,   NULL},
> > +     [PERF_RAPL_PP1]  = { 0, &rapl_events_gpu_group,   NULL},
> > +     [PERF_RAPL_PSYS] = { 0, &rapl_events_psys_group,  NULL},
> > +};
>
> Why have those !PKG things initialized? Wouldn't they default to 0
> anyway? If not, surely { 0, } is sufficient.

Yes, but that assumes that perf_msr_probe() is fixed to not expect a grp.
I think it is best to fix perf_msr_probe(). I already fixed one
problem, I'll fix this one as well.

Re: [PATCH 3/3] perf/x86/rapl: add AMD Fam17h RAPL support

[Stephane Eranian]

Hi,

On Mon, May 18, 2020 at 1:16 PM Stephane Eranian <eranian@google.com> wrote:
>
> On Mon, May 18, 2020 at 2:34 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Fri, May 15, 2020 at 02:57:33PM -0700, Stephane Eranian wrote:
> >
> > > +static struct perf_msr amd_rapl_msrs[] = {
> > > +     [PERF_RAPL_PP0]  = { 0, &rapl_events_cores_group, NULL},
> > > +     [PERF_RAPL_PKG]  = { MSR_AMD_PKG_ENERGY_STATUS,  &rapl_events_pkg_group,   test_msr },
> > > +     [PERF_RAPL_RAM]  = { 0, &rapl_events_ram_group,   NULL},
> > > +     [PERF_RAPL_PP1]  = { 0, &rapl_events_gpu_group,   NULL},
> > > +     [PERF_RAPL_PSYS] = { 0, &rapl_events_psys_group,  NULL},
> > > +};
> >
> > Why have those !PKG things initialized? Wouldn't they default to 0
> > anyway? If not, surely { 0, } is sufficient.
>
> Yes, but that assumes that perf_msr_probe() is fixed to not expect a grp.
> I think it is best to fix perf_msr_probe(). I already fixed one
> problem, I'll fix this one as well.

Well, now I remember what I did it the way it is in the patch. This
grp is going to sysfs, i.e., visible vs. not_visible callback.
Even if I fix the handling of NULL grp in perf_msr_probe(), the rest
of the rapl code pushes every event to sysfs and if the visible
callback is set to NULL this means the event is visible for sysfs! We
can fix that in init_rapl_pmus() but that is not pretty or leave it
as is, your call.