[PATCH v6 00/10] Add RAPL core energy counter support for AMD CPUs

[PATCH v6 00/10] Add RAPL core energy counter support for AMD CPUs

[Dhananjay Ugwekar]

Currently the energy-cores event in the power PMU aggregates energy
consumption data at a package level. On the other hand the core energy
RAPL counter in AMD CPUs has a core scope (which means the energy 
consumption is recorded separately for each core). Earlier efforts to add
the core event in the power PMU had failed [1], due to the difference in 
the scope of these two events. Hence, there is a need for a new core scope
PMU.

This patchset adds a new "power_core" PMU alongside the existing
"power" PMU, which will be responsible for collecting the new
"energy-core" event.

Tested the package level and core level PMU counters with workloads
pinned to different CPUs.

Results with workload pinned to CPU 4 in core 4 on an AMD Zen4 Genoa 
machine:

$ sudo perf stat --per-core -e power_core/energy-core/ -- taskset -c 4 stress-ng --matrix 1 --timeout 5s
stress-ng: info:  [21250] setting to a 5 second run per stressor
stress-ng: info:  [21250] dispatching hogs: 1 matrix
stress-ng: info:  [21250] successful run completed in 5.00s

 Performance counter stats for 'system wide':

S0-D0-C0              1               0.00 Joules power_core/energy-core/
S0-D0-C1              1               0.00 Joules power_core/energy-core/
S0-D0-C2              1               0.00 Joules power_core/energy-core/
S0-D0-C3              1               0.00 Joules power_core/energy-core/
S0-D0-C4              1               8.43 Joules power_core/energy-core/
S0-D0-C5              1               0.00 Joules power_core/energy-core/
S0-D0-C6              1               0.00 Joules power_core/energy-core/
S0-D0-C7              1               0.00 Joules power_core/energy-core/
S0-D1-C8              1               0.00 Joules power_core/energy-core/
S0-D1-C9              1               0.00 Joules power_core/energy-core/
S0-D1-C10             1               0.00 Joules power_core/energy-core/

v5 Link: https://lore.kernel.org/all/20240913152149.6317-1-Dhananjay.Ugwekar@amd.com/

v6 Changes:
* Rebase on top of V2 of Kan Liang's "PMU scope" patchset [2]
* Modify the PMU name from "power_per_core" to "power_core" and event name
  from "event_per_core" to "event_core"

Base: tip/master + [2]
 
[1]: https://lore.kernel.org/lkml/3e766f0e-37d4-0f82-3868-31b14228868d@linux.intel.com/
[2]: https://lore.kernel.org/all/20241010142604.770192-1-kan.liang@linux.intel.com/

Dhananjay Ugwekar (9):
  perf/x86/rapl: Remove the unused get_rapl_pmu_cpumask() function
  perf/x86/rapl: Remove the cpu_to_rapl_pmu() function
  perf/x86/rapl: Rename rapl_pmu variables
  perf/x86/rapl: Make rapl_model struct global
  perf/x86/rapl: Add arguments to the init and cleanup functions
  perf/x86/rapl: Modify the generic variable names to *_pkg*
  perf/x86/rapl: Remove the global variable rapl_msrs
  perf/x86/rapl: Move the cntr_mask to rapl_pmus struct
  perf/x86/rapl: Add core energy counter support for AMD CPUs

K Prateek Nayak (1):
  x86/topology: Introduce topology_logical_core_id()

 Documentation/arch/x86/topology.rst   |   4 +
 arch/x86/events/rapl.c                | 412 ++++++++++++++++----------
 arch/x86/include/asm/processor.h      |   1 +
 arch/x86/include/asm/topology.h       |   1 +
 arch/x86/kernel/cpu/debugfs.c         |   1 +
 arch/x86/kernel/cpu/topology_common.c |   1 +
 6 files changed, 268 insertions(+), 152 deletions(-)

-- 
2.34.1

[PATCH v6 01/10] perf/x86/rapl: Remove the unused get_rapl_pmu_cpumask() function

[Dhananjay Ugwekar]

An earlier commit eliminates the need for this function, so remove it.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 arch/x86/events/rapl.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index a8defc813c36..f70c49ca0ef3 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -153,7 +153,7 @@ static u64 rapl_timer_ms;
 static struct perf_msr *rapl_msrs;
 
 /*
- * Helper functions to get the correct topology macros according to the
+ * Helper function to get the correct topology id according to the
  * RAPL PMU scope.
  */
 static inline unsigned int get_rapl_pmu_idx(int cpu)
@@ -162,12 +162,6 @@ static inline unsigned int get_rapl_pmu_idx(int cpu)
 					 topology_logical_die_id(cpu);
 }
 
-static inline const struct cpumask *get_rapl_pmu_cpumask(int cpu)
-{
-	return rapl_pmu_is_pkg_scope() ? topology_core_cpumask(cpu) :
-					 topology_die_cpumask(cpu);
-}
-
 static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int cpu)
 {
 	unsigned int rapl_pmu_idx = get_rapl_pmu_idx(cpu);
-- 
2.34.1

Re: [PATCH v6 01/10] perf/x86/rapl: Remove the unused get_rapl_pmu_cpumask() function

[Gautham R. Shenoy]

Hello Dhananjay,

On Fri, Oct 25, 2024 at 11:13:39AM +0000, Dhananjay Ugwekar wrote:
> An earlier commit eliminates the need for this function, so remove it.

If the commit is merged, please provide the commit id. If it is not
merged, please share the shortlog here with the link to the patch on
the mailing list.

--
Thanks and Regards
gautham.

> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> ---
>  arch/x86/events/rapl.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index a8defc813c36..f70c49ca0ef3 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -153,7 +153,7 @@ static u64 rapl_timer_ms;
>  static struct perf_msr *rapl_msrs;
>  
>  /*
> - * Helper functions to get the correct topology macros according to the
> + * Helper function to get the correct topology id according to the
>   * RAPL PMU scope.
>   */
>  static inline unsigned int get_rapl_pmu_idx(int cpu)
> @@ -162,12 +162,6 @@ static inline unsigned int get_rapl_pmu_idx(int cpu)
>  					 topology_logical_die_id(cpu);
>  }
>  
> -static inline const struct cpumask *get_rapl_pmu_cpumask(int cpu)
> -{
> -	return rapl_pmu_is_pkg_scope() ? topology_core_cpumask(cpu) :
> -					 topology_die_cpumask(cpu);
> -}
> -
>  static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int cpu)
>  {
>  	unsigned int rapl_pmu_idx = get_rapl_pmu_idx(cpu);
> -- 
> 2.34.1
> 

Re: [PATCH v6 01/10] perf/x86/rapl: Remove the unused get_rapl_pmu_cpumask() function

[Dhananjay Ugwekar]

Hello Gautham,

On 10/28/2024 11:42 AM, Gautham R. Shenoy wrote:
> Hello Dhananjay,
> 
> On Fri, Oct 25, 2024 at 11:13:39AM +0000, Dhananjay Ugwekar wrote:
>> An earlier commit eliminates the need for this function, so remove it.
> 
> If the commit is merged, please provide the commit id. If it is not
> merged, please share the shortlog here with the link to the patch on
> the mailing list.

Sure, The commit I'm referring to here is 
"[PATCH V2 2/2] perf/x86/rapl: Clean up cpumask and hotplug"
(https://lore.kernel.org/all/20241010142604.770192-2-kan.liang@linux.intel.com/)
It is not yet merged, but I've taken it as base for this patch series.

It removes the cpumask handling from rapl.c. Hence, we no longer need the 
get_rapl_pmu_cpumask() function.

I can post a new version or provide newly drafted commit msg here, whichever way is 
okay with the maintainers.

Thanks,
Dhananjay

> 
> --
> Thanks and Regards
> gautham.
> 
>>
>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
>> ---
>>  arch/x86/events/rapl.c | 8 +-------
>>  1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
>> index a8defc813c36..f70c49ca0ef3 100644
>> --- a/arch/x86/events/rapl.c
>> +++ b/arch/x86/events/rapl.c
>> @@ -153,7 +153,7 @@ static u64 rapl_timer_ms;
>>  static struct perf_msr *rapl_msrs;
>>  
>>  /*
>> - * Helper functions to get the correct topology macros according to the
>> + * Helper function to get the correct topology id according to the
>>   * RAPL PMU scope.
>>   */
>>  static inline unsigned int get_rapl_pmu_idx(int cpu)
>> @@ -162,12 +162,6 @@ static inline unsigned int get_rapl_pmu_idx(int cpu)
>>  					 topology_logical_die_id(cpu);
>>  }
>>  
>> -static inline const struct cpumask *get_rapl_pmu_cpumask(int cpu)
>> -{
>> -	return rapl_pmu_is_pkg_scope() ? topology_core_cpumask(cpu) :
>> -					 topology_die_cpumask(cpu);
>> -}
>> -
>>  static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int cpu)
>>  {
>>  	unsigned int rapl_pmu_idx = get_rapl_pmu_idx(cpu);
>> -- 
>> 2.34.1
>>

Re: [PATCH v6 01/10] perf/x86/rapl: Remove the unused get_rapl_pmu_cpumask() function

[Zhang, Rui]

On Mon, 2024-10-28 at 12:08 +0530, Dhananjay Ugwekar wrote:
> > Hello Gautham,
> > 
> > On 10/28/2024 11:42 AM, Gautham R. Shenoy wrote:
> > > > Hello Dhananjay,
> > > > 
> > > > On Fri, Oct 25, 2024 at 11:13:39AM +0000, Dhananjay Ugwekar
> > > > wrote:
> > > > > > An earlier commit eliminates the need for this function, so
> > > > > > > > > remove it.
> > > > 
> > > > If the commit is merged, please provide the commit id. If it is
> > > > not
> > > > merged, please share the shortlog here with the link to the
> > > > patch > > on
> > > > the mailing list.
> > 
> > Sure, The commit I'm referring to here is 
> > "[PATCH V2 2/2] perf/x86/rapl: Clean up cpumask and hotplug"
> > (
> > https://lore.kernel.org/all/20241010142604.770192-2-kan.liang@linux.
> > intel.com/)
> > It is not yet merged, but I've taken it as base for this patch >
> > series.

I see the patch merged in tip tree yesterday.

> > 
> > It removes the cpumask handling from rapl.c. Hence, we no longer
> > need > the 
> > get_rapl_pmu_cpumask() function.
> > 
> > I can post a new version or provide newly drafted commit msg here,
> > > whichever way is 
> > okay with the maintainers.
> > 
> > 

Tested this patch set on top of latest tip/perf/core on an Intel
Raptorlake laptop, which has pkg/cores/gpu/psys counters, and didn't
observe any issue. So

Reviewed-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui

> > > > 
> > > > --
> > > > Thanks and Regards
> > > > gautham.
> > > > 
> > > > > > 
> > > > > > Signed-off-by: Dhananjay Ugwekar
> > > > > > <Dhananjay.Ugwekar@amd.com>
> > > > > > ---
> > > > > >  arch/x86/events/rapl.c | 8 +-------
> > > > > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/x86/events/rapl.c
> > > > > > b/arch/x86/events/rapl.c
> > > > > > index a8defc813c36..f70c49ca0ef3 100644
> > > > > > --- a/arch/x86/events/rapl.c
> > > > > > +++ b/arch/x86/events/rapl.c
> > > > > > @@ -153,7 +153,7 @@ static u64 rapl_timer_ms;
> > > > > >  static struct perf_msr *rapl_msrs;
> > > > > >  
> > > > > >  /*
> > > > > > - * Helper functions to get the correct topology macros
> > > > > > according > > > to the
> > > > > > + * Helper function to get the correct topology id
> > > > > > according to > > > the
> > > > > >   * RAPL PMU scope.
> > > > > >   */
> > > > > >  static inline unsigned int get_rapl_pmu_idx(int cpu)
> > > > > > @@ -162,12 +162,6 @@ static inline unsigned int > > >
> > > > > > get_rapl_pmu_idx(int cpu)
> > > > > >                                          > > >
> > > > > > topology_logical_die_id(cpu);
> > > > > >  }
> > > > > >  
> > > > > > -static inline const struct cpumask
> > > > > > *get_rapl_pmu_cpumask(int > > > cpu)
> > > > > > -{
> > > > > > -       return rapl_pmu_is_pkg_scope() ? > > >
> > > > > > topology_core_cpumask(cpu) :
> > > > > > -                                        > > >
> > > > > > topology_die_cpumask(cpu);
> > > > > > -}
> > > > > > -
> > > > > >  static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned
> > > > > > int cpu)
> > > > > >  {
> > > > > >         unsigned int rapl_pmu_idx = get_rapl_pmu_idx(cpu);
> > > > > > -- 
> > > > > > 2.34.1
> > > > > > 

[PATCH v6 02/10] x86/topology: Introduce topology_logical_core_id()

[Dhananjay Ugwekar]

From: K Prateek Nayak <kprateek.nayak@amd.com>

On x86, topology_core_id() returns a unique core ID within the PKG
domain. Looking at match_smt() suggests that a core ID just needs to be
unique within a LLC domain. For use cases such as the core RAPL PMU,
there exists a need for a unique core ID across the entire system with
multiple PKG domains. Introduce topology_logical_core_id() to derive a
unique core ID across the system.

Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
Reviewed-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: K Prateek Nayak <kprateek.nayak@amd.com>
Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 Documentation/arch/x86/topology.rst   | 4 ++++
 arch/x86/include/asm/processor.h      | 1 +
 arch/x86/include/asm/topology.h       | 1 +
 arch/x86/kernel/cpu/debugfs.c         | 1 +
 arch/x86/kernel/cpu/topology_common.c | 1 +
 5 files changed, 8 insertions(+)

diff --git a/Documentation/arch/x86/topology.rst b/Documentation/arch/x86/topology.rst
index 7352ab89a55a..c12837e61bda 100644
--- a/Documentation/arch/x86/topology.rst
+++ b/Documentation/arch/x86/topology.rst
@@ -135,6 +135,10 @@ Thread-related topology information in the kernel:
     The ID of the core to which a thread belongs. It is also printed in /proc/cpuinfo
     "core_id."
 
+  - topology_logical_core_id();
+
+    The logical core ID to which a thread belongs.
+
 
 
 System topology examples
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 4a686f0e5dbf..595859cbf7ff 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -98,6 +98,7 @@ struct cpuinfo_topology {
 	// Logical ID mappings
 	u32			logical_pkg_id;
 	u32			logical_die_id;
+	u32			logical_core_id;
 
 	// AMD Node ID and Nodes per Package info
 	u32			amd_node_id;
diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
index aef70336d624..672fccf9f845 100644
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -137,6 +137,7 @@ extern const struct cpumask *cpu_clustergroup_mask(int cpu);
 #define topology_logical_package_id(cpu)	(cpu_data(cpu).topo.logical_pkg_id)
 #define topology_physical_package_id(cpu)	(cpu_data(cpu).topo.pkg_id)
 #define topology_logical_die_id(cpu)		(cpu_data(cpu).topo.logical_die_id)
+#define topology_logical_core_id(cpu)		(cpu_data(cpu).topo.logical_core_id)
 #define topology_die_id(cpu)			(cpu_data(cpu).topo.die_id)
 #define topology_core_id(cpu)			(cpu_data(cpu).topo.core_id)
 #define topology_ppin(cpu)			(cpu_data(cpu).ppin)
diff --git a/arch/x86/kernel/cpu/debugfs.c b/arch/x86/kernel/cpu/debugfs.c
index 3baf3e435834..b1eb6d7828db 100644
--- a/arch/x86/kernel/cpu/debugfs.c
+++ b/arch/x86/kernel/cpu/debugfs.c
@@ -24,6 +24,7 @@ static int cpu_debug_show(struct seq_file *m, void *p)
 	seq_printf(m, "core_id:             %u\n", c->topo.core_id);
 	seq_printf(m, "logical_pkg_id:      %u\n", c->topo.logical_pkg_id);
 	seq_printf(m, "logical_die_id:      %u\n", c->topo.logical_die_id);
+	seq_printf(m, "logical_core_id:     %u\n", c->topo.logical_core_id);
 	seq_printf(m, "llc_id:              %u\n", c->topo.llc_id);
 	seq_printf(m, "l2c_id:              %u\n", c->topo.l2c_id);
 	seq_printf(m, "amd_node_id:         %u\n", c->topo.amd_node_id);
diff --git a/arch/x86/kernel/cpu/topology_common.c b/arch/x86/kernel/cpu/topology_common.c
index 9a6069e7133c..23722aa21e2f 100644
--- a/arch/x86/kernel/cpu/topology_common.c
+++ b/arch/x86/kernel/cpu/topology_common.c
@@ -151,6 +151,7 @@ static void topo_set_ids(struct topo_scan *tscan, bool early)
 	if (!early) {
 		c->topo.logical_pkg_id = topology_get_logical_id(apicid, TOPO_PKG_DOMAIN);
 		c->topo.logical_die_id = topology_get_logical_id(apicid, TOPO_DIE_DOMAIN);
+		c->topo.logical_core_id = topology_get_logical_id(apicid, TOPO_CORE_DOMAIN);
 	}
 
 	/* Package relative core ID */
-- 
2.34.1

Re: [PATCH v6 02/10] x86/topology: Introduce topology_logical_core_id()

[Gautham R. Shenoy]

On Fri, Oct 25, 2024 at 11:13:40AM +0000, Dhananjay Ugwekar wrote:
> From: K Prateek Nayak <kprateek.nayak@amd.com>
> 
> On x86, topology_core_id() returns a unique core ID within the PKG
> domain. Looking at match_smt() suggests that a core ID just needs to be
> unique within a LLC domain. For use cases such as the core RAPL PMU,
> there exists a need for a unique core ID across the entire system with
> multiple PKG domains. Introduce topology_logical_core_id() to derive a
> unique core ID across the system.
> 
> Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
> Reviewed-by: Zhang Rui <rui.zhang@intel.com>
> Tested-by: K Prateek Nayak <kprateek.nayak@amd.com>
> Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

LGTM

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

> ---
>  Documentation/arch/x86/topology.rst   | 4 ++++
>  arch/x86/include/asm/processor.h      | 1 +
>  arch/x86/include/asm/topology.h       | 1 +
>  arch/x86/kernel/cpu/debugfs.c         | 1 +
>  arch/x86/kernel/cpu/topology_common.c | 1 +
>  5 files changed, 8 insertions(+)
> 
> diff --git a/Documentation/arch/x86/topology.rst b/Documentation/arch/x86/topology.rst
> index 7352ab89a55a..c12837e61bda 100644
> --- a/Documentation/arch/x86/topology.rst
> +++ b/Documentation/arch/x86/topology.rst
> @@ -135,6 +135,10 @@ Thread-related topology information in the kernel:
>      The ID of the core to which a thread belongs. It is also printed in /proc/cpuinfo
>      "core_id."
>  
> +  - topology_logical_core_id();
> +
> +    The logical core ID to which a thread belongs.
> +
>  
>  
>  System topology examples
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 4a686f0e5dbf..595859cbf7ff 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -98,6 +98,7 @@ struct cpuinfo_topology {
>  	// Logical ID mappings
>  	u32			logical_pkg_id;
>  	u32			logical_die_id;
> +	u32			logical_core_id;
>  
>  	// AMD Node ID and Nodes per Package info
>  	u32			amd_node_id;
> diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
> index aef70336d624..672fccf9f845 100644
> --- a/arch/x86/include/asm/topology.h
> +++ b/arch/x86/include/asm/topology.h
> @@ -137,6 +137,7 @@ extern const struct cpumask *cpu_clustergroup_mask(int cpu);
>  #define topology_logical_package_id(cpu)	(cpu_data(cpu).topo.logical_pkg_id)
>  #define topology_physical_package_id(cpu)	(cpu_data(cpu).topo.pkg_id)
>  #define topology_logical_die_id(cpu)		(cpu_data(cpu).topo.logical_die_id)
> +#define topology_logical_core_id(cpu)		(cpu_data(cpu).topo.logical_core_id)
>  #define topology_die_id(cpu)			(cpu_data(cpu).topo.die_id)
>  #define topology_core_id(cpu)			(cpu_data(cpu).topo.core_id)
>  #define topology_ppin(cpu)			(cpu_data(cpu).ppin)
> diff --git a/arch/x86/kernel/cpu/debugfs.c b/arch/x86/kernel/cpu/debugfs.c
> index 3baf3e435834..b1eb6d7828db 100644
> --- a/arch/x86/kernel/cpu/debugfs.c
> +++ b/arch/x86/kernel/cpu/debugfs.c
> @@ -24,6 +24,7 @@ static int cpu_debug_show(struct seq_file *m, void *p)
>  	seq_printf(m, "core_id:             %u\n", c->topo.core_id);
>  	seq_printf(m, "logical_pkg_id:      %u\n", c->topo.logical_pkg_id);
>  	seq_printf(m, "logical_die_id:      %u\n", c->topo.logical_die_id);
> +	seq_printf(m, "logical_core_id:     %u\n", c->topo.logical_core_id);
>  	seq_printf(m, "llc_id:              %u\n", c->topo.llc_id);
>  	seq_printf(m, "l2c_id:              %u\n", c->topo.l2c_id);
>  	seq_printf(m, "amd_node_id:         %u\n", c->topo.amd_node_id);
> diff --git a/arch/x86/kernel/cpu/topology_common.c b/arch/x86/kernel/cpu/topology_common.c
> index 9a6069e7133c..23722aa21e2f 100644
> --- a/arch/x86/kernel/cpu/topology_common.c
> +++ b/arch/x86/kernel/cpu/topology_common.c
> @@ -151,6 +151,7 @@ static void topo_set_ids(struct topo_scan *tscan, bool early)
>  	if (!early) {
>  		c->topo.logical_pkg_id = topology_get_logical_id(apicid, TOPO_PKG_DOMAIN);
>  		c->topo.logical_die_id = topology_get_logical_id(apicid, TOPO_DIE_DOMAIN);
> +		c->topo.logical_core_id = topology_get_logical_id(apicid, TOPO_CORE_DOMAIN);
>  	}
>  
>  	/* Package relative core ID */
> -- 
> 2.34.1
> 

[PATCH v6 03/10] perf/x86/rapl: Remove the cpu_to_rapl_pmu() function

[Dhananjay Ugwekar]

Prepare for the addition of RAPL core energy counter support.
Post which, one CPU might be mapped to more than one rapl_pmu
(package/die one and a core one). So, remove the cpu_to_rapl_pmu()
function.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 arch/x86/events/rapl.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index f70c49ca0ef3..d20c5b1dd0ad 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -162,17 +162,6 @@ static inline unsigned int get_rapl_pmu_idx(int cpu)
 					 topology_logical_die_id(cpu);
 }
 
-static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int cpu)
-{
-	unsigned int rapl_pmu_idx = get_rapl_pmu_idx(cpu);
-
-	/*
-	 * The unsigned check also catches the '-1' return value for non
-	 * existent mappings in the topology map.
-	 */
-	return rapl_pmu_idx < rapl_pmus->nr_rapl_pmu ? rapl_pmus->pmus[rapl_pmu_idx] : NULL;
-}
-
 static inline u64 rapl_read_counter(struct perf_event *event)
 {
 	u64 raw;
@@ -348,7 +337,7 @@ static void rapl_pmu_event_del(struct perf_event *event, int flags)
 static int rapl_pmu_event_init(struct perf_event *event)
 {
 	u64 cfg = event->attr.config & RAPL_EVENT_MASK;
-	int bit, ret = 0;
+	int bit, rapl_pmu_idx, ret = 0;
 	struct rapl_pmu *pmu;
 
 	/* only look at RAPL events */
@@ -376,8 +365,12 @@ static int rapl_pmu_event_init(struct perf_event *event)
 	if (event->attr.sample_period) /* no sampling */
 		return -EINVAL;
 
+	rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
+	if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
+		return -EINVAL;
+
 	/* must be done before validate_group */
-	pmu = cpu_to_rapl_pmu(event->cpu);
+	pmu = rapl_pmus->pmus[rapl_pmu_idx];
 	if (!pmu)
 		return -EINVAL;
 	event->pmu_private = pmu;
-- 
2.34.1

Re: [PATCH v6 03/10] perf/x86/rapl: Remove the cpu_to_rapl_pmu() function

[Gautham R. Shenoy]

Hello Dhananjay,

On Fri, Oct 25, 2024 at 11:13:41AM +0000, Dhananjay Ugwekar wrote:
> Prepare for the addition of RAPL core energy counter support.
> Post which, one CPU might be mapped to more than one rapl_pmu
> (package/die one and a core one). So, remove the cpu_to_rapl_pmu()
> function.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> ---
>  arch/x86/events/rapl.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index f70c49ca0ef3..d20c5b1dd0ad 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -162,17 +162,6 @@ static inline unsigned int get_rapl_pmu_idx(int cpu)
>  					 topology_logical_die_id(cpu);
>  }
>  
> -static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int cpu)
> -{
> -	unsigned int rapl_pmu_idx = get_rapl_pmu_idx(cpu);
> -
> -	/*
> -	 * The unsigned check also catches the '-1' return value for non
> -	 * existent mappings in the topology map.
> -	 */


See the comment here why rapl_pmu_idx should be an "unsigned int".


> -	return rapl_pmu_idx < rapl_pmus->nr_rapl_pmu ? rapl_pmus->pmus[rapl_pmu_idx] : NULL;
> -}
> -
>  static inline u64 rapl_read_counter(struct perf_event *event)
>  {
>  	u64 raw;
> @@ -348,7 +337,7 @@ static void rapl_pmu_event_del(struct perf_event *event, int flags)
>  static int rapl_pmu_event_init(struct perf_event *event)
>  {
>  	u64 cfg = event->attr.config & RAPL_EVENT_MASK;
> -	int bit, ret = 0;
> +	int bit, rapl_pmu_idx, ret = 0;

Considering that, shouldn't rapl_pmu_idx be an "unsigned int" no?

--
Thanks and Regards
gautham.


>  	struct rapl_pmu *pmu;
>  
>  	/* only look at RAPL events */
> @@ -376,8 +365,12 @@ static int rapl_pmu_event_init(struct perf_event *event)
>  	if (event->attr.sample_period) /* no sampling */
>  		return -EINVAL;
>  
> +	rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
> +	if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
> +		return -EINVAL;
> +
>  	/* must be done before validate_group */
> -	pmu = cpu_to_rapl_pmu(event->cpu);
> +	pmu = rapl_pmus->pmus[rapl_pmu_idx];
>  	if (!pmu)
>  		return -EINVAL;
>  	event->pmu_private = pmu;
> -- 
> 2.34.1
> 

Re: [PATCH v6 03/10] perf/x86/rapl: Remove the cpu_to_rapl_pmu() function

[Dhananjay Ugwekar]

Hello Gautham,

On 10/28/2024 2:23 PM, Gautham R. Shenoy wrote:
> Hello Dhananjay,
> 
> On Fri, Oct 25, 2024 at 11:13:41AM +0000, Dhananjay Ugwekar wrote:
>> Prepare for the addition of RAPL core energy counter support.
>> Post which, one CPU might be mapped to more than one rapl_pmu
>> (package/die one and a core one). So, remove the cpu_to_rapl_pmu()
>> function.
>>
>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
>> ---
>>  arch/x86/events/rapl.c | 19 ++++++-------------
>>  1 file changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
>> index f70c49ca0ef3..d20c5b1dd0ad 100644
>> --- a/arch/x86/events/rapl.c
>> +++ b/arch/x86/events/rapl.c
>> @@ -162,17 +162,6 @@ static inline unsigned int get_rapl_pmu_idx(int cpu)
>>  					 topology_logical_die_id(cpu);
>>  }
>>  
>> -static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int cpu)
>> -{
>> -	unsigned int rapl_pmu_idx = get_rapl_pmu_idx(cpu);
>> -
>> -	/*
>> -	 * The unsigned check also catches the '-1' return value for non
>> -	 * existent mappings in the topology map.
>> -	 */
> 
> 
> See the comment here why rapl_pmu_idx should be an "unsigned int".
> 
> 
>> -	return rapl_pmu_idx < rapl_pmus->nr_rapl_pmu ? rapl_pmus->pmus[rapl_pmu_idx] : NULL;
>> -}
>> -
>>  static inline u64 rapl_read_counter(struct perf_event *event)
>>  {
>>  	u64 raw;
>> @@ -348,7 +337,7 @@ static void rapl_pmu_event_del(struct perf_event *event, int flags)
>>  static int rapl_pmu_event_init(struct perf_event *event)
>>  {
>>  	u64 cfg = event->attr.config & RAPL_EVENT_MASK;
>> -	int bit, ret = 0;
>> +	int bit, rapl_pmu_idx, ret = 0;
> 
> Considering that, shouldn't rapl_pmu_idx be an "unsigned int" no?

Correct, with unsigned int we will be able to check for negative values as well with the 
"if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)" check. Will fix this in next version.

Thanks,
Dhananjay

> 
> --
> Thanks and Regards
> gautham.
> 
> 
>>  	struct rapl_pmu *pmu;
>>  
>>  	/* only look at RAPL events */
>> @@ -376,8 +365,12 @@ static int rapl_pmu_event_init(struct perf_event *event)
>>  	if (event->attr.sample_period) /* no sampling */
>>  		return -EINVAL;
>>  
>> +	rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
>> +	if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
>> +		return -EINVAL;
>> +
>>  	/* must be done before validate_group */
>> -	pmu = cpu_to_rapl_pmu(event->cpu);
>> +	pmu = rapl_pmus->pmus[rapl_pmu_idx];
>>  	if (!pmu)
>>  		return -EINVAL;
>>  	event->pmu_private = pmu;
>> -- 
>> 2.34.1
>>

Re: [PATCH v6 03/10] perf/x86/rapl: Remove the cpu_to_rapl_pmu() function

[Zhang, Rui]

On Mon, 2024-10-28 at 14:49 +0530, Dhananjay Ugwekar wrote:
> Hello Gautham,
> 
> On 10/28/2024 2:23 PM, Gautham R. Shenoy wrote:
> > Hello Dhananjay,
> > 
> > On Fri, Oct 25, 2024 at 11:13:41AM +0000, Dhananjay Ugwekar wrote:
> > > Prepare for the addition of RAPL core energy counter support.
> > > Post which, one CPU might be mapped to more than one rapl_pmu
> > > (package/die one and a core one). So, remove the
> > > cpu_to_rapl_pmu()
> > > function.
> > > 
> > > Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> > > ---
> > >  arch/x86/events/rapl.c | 19 ++++++-------------
> > >  1 file changed, 6 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> > > index f70c49ca0ef3..d20c5b1dd0ad 100644
> > > --- a/arch/x86/events/rapl.c
> > > +++ b/arch/x86/events/rapl.c
> > > @@ -162,17 +162,6 @@ static inline unsigned int
> > > get_rapl_pmu_idx(int cpu)
> > >                                         
> > > topology_logical_die_id(cpu);
> > >  }
> > >  
> > > -static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int cpu)
> > > -{
> > > -       unsigned int rapl_pmu_idx = get_rapl_pmu_idx(cpu);
> > > -
> > > -       /*
> > > -        * The unsigned check also catches the '-1' return value
> > > for non
> > > -        * existent mappings in the topology map.
> > > -        */
> > 
> > 
> > See the comment here why rapl_pmu_idx should be an "unsigned int".
> > 
> > 
> > > -       return rapl_pmu_idx < rapl_pmus->nr_rapl_pmu ? rapl_pmus-
> > > >pmus[rapl_pmu_idx] : NULL;
> > > -}
> > > -
> > >  static inline u64 rapl_read_counter(struct perf_event *event)
> > >  {
> > >         u64 raw;
> > > @@ -348,7 +337,7 @@ static void rapl_pmu_event_del(struct
> > > perf_event *event, int flags)
> > >  static int rapl_pmu_event_init(struct perf_event *event)
> > >  {
> > >         u64 cfg = event->attr.config & RAPL_EVENT_MASK;
> > > -       int bit, ret = 0;
> > > +       int bit, rapl_pmu_idx, ret = 0;
> > 
> > Considering that, shouldn't rapl_pmu_idx be an "unsigned int" no?
> 
> Correct, with unsigned int we will be able to check for negative
> values as well with the 
> "if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)" check. Will fix this in
> next version.
> 
you can stick with unsigned int here, but in patch 10/10, IMO, making
get_rapl_pmu_idx() return int instead of unsigned int is more
straightforward.

thanks,
rui

> Thanks,
> Dhananjay
> 
> > 
> > --
> > Thanks and Regards
> > gautham.
> > 
> > 
> > >         struct rapl_pmu *pmu;
> > >  
> > >         /* only look at RAPL events */
> > > @@ -376,8 +365,12 @@ static int rapl_pmu_event_init(struct
> > > perf_event *event)
> > >         if (event->attr.sample_period) /* no sampling */
> > >                 return -EINVAL;
> > >  
> > > +       rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
> > > +       if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
> > > +               return -EINVAL;
> > > +
> > >         /* must be done before validate_group */
> > > -       pmu = cpu_to_rapl_pmu(event->cpu);
> > > +       pmu = rapl_pmus->pmus[rapl_pmu_idx];
> > >         if (!pmu)
> > >                 return -EINVAL;
> > >         event->pmu_private = pmu;
> > > -- 
> > > 2.34.1
> > > 

Re: [PATCH v6 03/10] perf/x86/rapl: Remove the cpu_to_rapl_pmu() function

[Dhananjay Ugwekar]

Hello Rui,

Thanks for reviewing and testing the series!,

On 11/1/2024 1:36 PM, Zhang, Rui wrote:
> On Mon, 2024-10-28 at 14:49 +0530, Dhananjay Ugwekar wrote:
>> Hello Gautham,
>>
>> On 10/28/2024 2:23 PM, Gautham R. Shenoy wrote:
>>> Hello Dhananjay,
>>>
>>> On Fri, Oct 25, 2024 at 11:13:41AM +0000, Dhananjay Ugwekar wrote:
>>>> Prepare for the addition of RAPL core energy counter support.
>>>> Post which, one CPU might be mapped to more than one rapl_pmu
>>>> (package/die one and a core one). So, remove the
>>>> cpu_to_rapl_pmu()
>>>> function.
>>>>
>>>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
>>>> ---
>>>>  arch/x86/events/rapl.c | 19 ++++++-------------
>>>>  1 file changed, 6 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
>>>> index f70c49ca0ef3..d20c5b1dd0ad 100644
>>>> --- a/arch/x86/events/rapl.c
>>>> +++ b/arch/x86/events/rapl.c
>>>> @@ -162,17 +162,6 @@ static inline unsigned int
>>>> get_rapl_pmu_idx(int cpu)
>>>>                                         
>>>> topology_logical_die_id(cpu);
>>>>  }
>>>>  
>>>> -static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int cpu)
>>>> -{
>>>> -       unsigned int rapl_pmu_idx = get_rapl_pmu_idx(cpu);
>>>> -
>>>> -       /*
>>>> -        * The unsigned check also catches the '-1' return value
>>>> for non
>>>> -        * existent mappings in the topology map.
>>>> -        */
>>>
>>>
>>> See the comment here why rapl_pmu_idx should be an "unsigned int".
>>>
>>>
>>>> -       return rapl_pmu_idx < rapl_pmus->nr_rapl_pmu ? rapl_pmus-
>>>>> pmus[rapl_pmu_idx] : NULL;
>>>> -}
>>>> -
>>>>  static inline u64 rapl_read_counter(struct perf_event *event)
>>>>  {
>>>>         u64 raw;
>>>> @@ -348,7 +337,7 @@ static void rapl_pmu_event_del(struct
>>>> perf_event *event, int flags)
>>>>  static int rapl_pmu_event_init(struct perf_event *event)
>>>>  {
>>>>         u64 cfg = event->attr.config & RAPL_EVENT_MASK;
>>>> -       int bit, ret = 0;
>>>> +       int bit, rapl_pmu_idx, ret = 0;
>>>
>>> Considering that, shouldn't rapl_pmu_idx be an "unsigned int" no?
>>
>> Correct, with unsigned int we will be able to check for negative
>> values as well with the 
>> "if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)" check. Will fix this in
>> next version.
>>
> you can stick with unsigned int here, but in patch 10/10, IMO, making
> get_rapl_pmu_idx() return int instead of unsigned int is more
> straightforward.

But I have one doubt, there wont be any functional difference in returning 
"unsigned int" vs "int" right?, we will still need to check the same condition 
for the return value i.e. "if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)" 
(assuming we are still storing the return value in "unsigned int rapl_pmu_idx"), 
I think I didnt get your point.

Thanks,
Dhananjay

> 
> thanks,
> rui
> 
>> Thanks,
>> Dhananjay
>>
>>>
>>> --
>>> Thanks and Regards
>>> gautham.
>>>
>>>
>>>>         struct rapl_pmu *pmu;
>>>>  
>>>>         /* only look at RAPL events */
>>>> @@ -376,8 +365,12 @@ static int rapl_pmu_event_init(struct
>>>> perf_event *event)
>>>>         if (event->attr.sample_period) /* no sampling */
>>>>                 return -EINVAL;
>>>>  
>>>> +       rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
>>>> +       if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
>>>> +               return -EINVAL;
>>>> +
>>>>         /* must be done before validate_group */
>>>> -       pmu = cpu_to_rapl_pmu(event->cpu);
>>>> +       pmu = rapl_pmus->pmus[rapl_pmu_idx];
>>>>         if (!pmu)
>>>>                 return -EINVAL;
>>>>         event->pmu_private = pmu;
>>>> -- 
>>>> 2.34.1
>>>>
> 

Re: [PATCH v6 03/10] perf/x86/rapl: Remove the cpu_to_rapl_pmu() function

[Zhang, Rui]

On Mon, 2024-11-04 at 08:45 +0530, Dhananjay Ugwekar wrote:
> Hello Rui,
> 
> Thanks for reviewing and testing the series!,
> 
> On 11/1/2024 1:36 PM, Zhang, Rui wrote:
> > On Mon, 2024-10-28 at 14:49 +0530, Dhananjay Ugwekar wrote:
> > > Hello Gautham,
> > > 
> > > On 10/28/2024 2:23 PM, Gautham R. Shenoy wrote:
> > > > Hello Dhananjay,
> > > > 
> > > > On Fri, Oct 25, 2024 at 11:13:41AM +0000, Dhananjay Ugwekar
> > > > wrote:
> > > > > Prepare for the addition of RAPL core energy counter support.
> > > > > Post which, one CPU might be mapped to more than one rapl_pmu
> > > > > (package/die one and a core one). So, remove the
> > > > > cpu_to_rapl_pmu()
> > > > > function.
> > > > > 
> > > > > Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> > > > > ---
> > > > >  arch/x86/events/rapl.c | 19 ++++++-------------
> > > > >  1 file changed, 6 insertions(+), 13 deletions(-)
> > > > > 
> > > > > diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> > > > > index f70c49ca0ef3..d20c5b1dd0ad 100644
> > > > > --- a/arch/x86/events/rapl.c
> > > > > +++ b/arch/x86/events/rapl.c
> > > > > @@ -162,17 +162,6 @@ static inline unsigned int
> > > > > get_rapl_pmu_idx(int cpu)
> > > > >                                         
> > > > > topology_logical_die_id(cpu);
> > > > >  }
> > > > >  
> > > > > -static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int
> > > > > cpu)
> > > > > -{
> > > > > -       unsigned int rapl_pmu_idx = get_rapl_pmu_idx(cpu);
> > > > > -
> > > > > -       /*
> > > > > -        * The unsigned check also catches the '-1' return
> > > > > value
> > > > > for non
> > > > > -        * existent mappings in the topology map.
> > > > > -        */
> > > > 
> > > > 
> > > > See the comment here why rapl_pmu_idx should be an "unsigned
> > > > int".
> > > > 
> > > > 
> > > > > -       return rapl_pmu_idx < rapl_pmus->nr_rapl_pmu ?
> > > > > rapl_pmus-
> > > > > > pmus[rapl_pmu_idx] : NULL;
> > > > > -}
> > > > > -
> > > > >  static inline u64 rapl_read_counter(struct perf_event
> > > > > *event)
> > > > >  {
> > > > >         u64 raw;
> > > > > @@ -348,7 +337,7 @@ static void rapl_pmu_event_del(struct
> > > > > perf_event *event, int flags)
> > > > >  static int rapl_pmu_event_init(struct perf_event *event)
> > > > >  {
> > > > >         u64 cfg = event->attr.config & RAPL_EVENT_MASK;
> > > > > -       int bit, ret = 0;
> > > > > +       int bit, rapl_pmu_idx, ret = 0;
> > > > 
> > > > Considering that, shouldn't rapl_pmu_idx be an "unsigned int"
> > > > no?
> > > 
> > > Correct, with unsigned int we will be able to check for negative
> > > values as well with the 
> > > "if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)" check. Will fix
> > > this in
> > > next version.
> > > 
> > you can stick with unsigned int here, but in patch 10/10, IMO,
> > making
> > get_rapl_pmu_idx() return int instead of unsigned int is more
> > straightforward.
> 
> But I have one doubt, there wont be any functional difference in
> returning 
> "unsigned int" vs "int" right?

yes, this doesn't cause any issue.

> , we will still need to check the same condition 
> for the return value i.e. "if (rapl_pmu_idx >= rapl_pmus-
> >nr_rapl_pmu)" 
> (assuming we are still storing the return value in "unsigned int
> rapl_pmu_idx"), 
> I think I didnt get your point.

With this patch, below comment is removed
 /*
  * The unsigned check also catches the '-1' return
value for non
  * existent mappings in the topology map.
  */
And we still rely on the unsigned int -> int conversion for the error
check.

So IMO, we should either add back a similar comment, or convert
get_rapl_pmu_idx() to return int and modify the error check.

thanks,
rui


> Thanks,
> Dhananjay
> 
> > 
> > thanks,
> > rui
> > 
> > > Thanks,
> > > Dhananjay
> > > 
> > > > 
> > > > --
> > > > Thanks and Regards
> > > > gautham.
> > > > 
> > > > 
> > > > >         struct rapl_pmu *pmu;
> > > > >  
> > > > >         /* only look at RAPL events */
> > > > > @@ -376,8 +365,12 @@ static int rapl_pmu_event_init(struct
> > > > > perf_event *event)
> > > > >         if (event->attr.sample_period) /* no sampling */
> > > > >                 return -EINVAL;
> > > > >  
> > > > > +       rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
> > > > > +       if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
> > > > > +               return -EINVAL;
> > > > > +
> > > > >         /* must be done before validate_group */
> > > > > -       pmu = cpu_to_rapl_pmu(event->cpu);
> > > > > +       pmu = rapl_pmus->pmus[rapl_pmu_idx];
> > > > >         if (!pmu)
> > > > >                 return -EINVAL;
> > > > >         event->pmu_private = pmu;
> > > > > -- 
> > > > > 2.34.1
> > > > > 
> > 

Re: [PATCH v6 03/10] perf/x86/rapl: Remove the cpu_to_rapl_pmu() function

[Dhananjay Ugwekar]

On 11/4/2024 12:45 PM, Zhang, Rui wrote:
> On Mon, 2024-11-04 at 08:45 +0530, Dhananjay Ugwekar wrote:
>> Hello Rui,
>>
>> Thanks for reviewing and testing the series!,
>>
>> On 11/1/2024 1:36 PM, Zhang, Rui wrote:
>>> On Mon, 2024-10-28 at 14:49 +0530, Dhananjay Ugwekar wrote:
>>>> Hello Gautham,
>>>>
>>>> On 10/28/2024 2:23 PM, Gautham R. Shenoy wrote:
>>>>> Hello Dhananjay,
>>>>>
>>>>> On Fri, Oct 25, 2024 at 11:13:41AM +0000, Dhananjay Ugwekar
>>>>> wrote:
>>>>>> Prepare for the addition of RAPL core energy counter support.
>>>>>> Post which, one CPU might be mapped to more than one rapl_pmu
>>>>>> (package/die one and a core one). So, remove the
>>>>>> cpu_to_rapl_pmu()
>>>>>> function.
>>>>>>
>>>>>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
>>>>>> ---
>>>>>>  arch/x86/events/rapl.c | 19 ++++++-------------
>>>>>>  1 file changed, 6 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
>>>>>> index f70c49ca0ef3..d20c5b1dd0ad 100644
>>>>>> --- a/arch/x86/events/rapl.c
>>>>>> +++ b/arch/x86/events/rapl.c
>>>>>> @@ -162,17 +162,6 @@ static inline unsigned int
>>>>>> get_rapl_pmu_idx(int cpu)
>>>>>>                                         
>>>>>> topology_logical_die_id(cpu);
>>>>>>  }
>>>>>>  
>>>>>> -static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int
>>>>>> cpu)
>>>>>> -{
>>>>>> -       unsigned int rapl_pmu_idx = get_rapl_pmu_idx(cpu);
>>>>>> -
>>>>>> -       /*
>>>>>> -        * The unsigned check also catches the '-1' return
>>>>>> value
>>>>>> for non
>>>>>> -        * existent mappings in the topology map.
>>>>>> -        */
>>>>>
>>>>>
>>>>> See the comment here why rapl_pmu_idx should be an "unsigned
>>>>> int".
>>>>>
>>>>>
>>>>>> -       return rapl_pmu_idx < rapl_pmus->nr_rapl_pmu ?
>>>>>> rapl_pmus-
>>>>>>> pmus[rapl_pmu_idx] : NULL;
>>>>>> -}
>>>>>> -
>>>>>>  static inline u64 rapl_read_counter(struct perf_event
>>>>>> *event)
>>>>>>  {
>>>>>>         u64 raw;
>>>>>> @@ -348,7 +337,7 @@ static void rapl_pmu_event_del(struct
>>>>>> perf_event *event, int flags)
>>>>>>  static int rapl_pmu_event_init(struct perf_event *event)
>>>>>>  {
>>>>>>         u64 cfg = event->attr.config & RAPL_EVENT_MASK;
>>>>>> -       int bit, ret = 0;
>>>>>> +       int bit, rapl_pmu_idx, ret = 0;
>>>>>
>>>>> Considering that, shouldn't rapl_pmu_idx be an "unsigned int"
>>>>> no?
>>>>
>>>> Correct, with unsigned int we will be able to check for negative
>>>> values as well with the 
>>>> "if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)" check. Will fix
>>>> this in
>>>> next version.
>>>>
>>> you can stick with unsigned int here, but in patch 10/10, IMO,
>>> making
>>> get_rapl_pmu_idx() return int instead of unsigned int is more
>>> straightforward.
>>
>> But I have one doubt, there wont be any functional difference in
>> returning 
>> "unsigned int" vs "int" right?
> 
> yes, this doesn't cause any issue.
> 
>> , we will still need to check the same condition 
>> for the return value i.e. "if (rapl_pmu_idx >= rapl_pmus-
>>> nr_rapl_pmu)" 
>> (assuming we are still storing the return value in "unsigned int
>> rapl_pmu_idx"), 
>> I think I didnt get your point.
> 
> With this patch, below comment is removed
>  /*
>   * The unsigned check also catches the '-1' return
> value for non
>   * existent mappings in the topology map.
>   */
> And we still rely on the unsigned int -> int conversion for the error
> check.
> 
> So IMO, we should either add back a similar comment, or convert
> get_rapl_pmu_idx() to return int and modify the error check.

Correct, I think I'll prefer adding a similar comment and keeping the 
error check as is, will fix this.

Thanks,
Dhananjay

> 
> thanks,
> rui
> 
> 
>> Thanks,
>> Dhananjay
>>
>>>
>>> thanks,
>>> rui
>>>
>>>> Thanks,
>>>> Dhananjay
>>>>
>>>>>
>>>>> --
>>>>> Thanks and Regards
>>>>> gautham.
>>>>>
>>>>>
>>>>>>         struct rapl_pmu *pmu;
>>>>>>  
>>>>>>         /* only look at RAPL events */
>>>>>> @@ -376,8 +365,12 @@ static int rapl_pmu_event_init(struct
>>>>>> perf_event *event)
>>>>>>         if (event->attr.sample_period) /* no sampling */
>>>>>>                 return -EINVAL;
>>>>>>  
>>>>>> +       rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
>>>>>> +       if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
>>>>>> +               return -EINVAL;
>>>>>> +
>>>>>>         /* must be done before validate_group */
>>>>>> -       pmu = cpu_to_rapl_pmu(event->cpu);
>>>>>> +       pmu = rapl_pmus->pmus[rapl_pmu_idx];
>>>>>>         if (!pmu)
>>>>>>                 return -EINVAL;
>>>>>>         event->pmu_private = pmu;
>>>>>> -- 
>>>>>> 2.34.1
>>>>>>
>>>
> 

Re: [PATCH v6 03/10] perf/x86/rapl: Remove the cpu_to_rapl_pmu() function

[Zhang, Rui]

On Fri, 2024-10-25 at 11:13 +0000, Dhananjay Ugwekar wrote:
> Prepare for the addition of RAPL core energy counter support.
> Post which, one CPU might be mapped to more than one rapl_pmu
> (package/die one and a core one). So, remove the cpu_to_rapl_pmu()
> function.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

Reviewed-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui

> ---
>  arch/x86/events/rapl.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index f70c49ca0ef3..d20c5b1dd0ad 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -162,17 +162,6 @@ static inline unsigned int get_rapl_pmu_idx(int
> cpu)
>                                         
> topology_logical_die_id(cpu);
>  }
>  
> -static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int cpu)
> -{
> -       unsigned int rapl_pmu_idx = get_rapl_pmu_idx(cpu);
> -
> -       /*
> -        * The unsigned check also catches the '-1' return value for
> non
> -        * existent mappings in the topology map.
> -        */
> -       return rapl_pmu_idx < rapl_pmus->nr_rapl_pmu ? rapl_pmus-
> >pmus[rapl_pmu_idx] : NULL;
> -}
> -
>  static inline u64 rapl_read_counter(struct perf_event *event)
>  {
>         u64 raw;
> @@ -348,7 +337,7 @@ static void rapl_pmu_event_del(struct perf_event
> *event, int flags)
>  static int rapl_pmu_event_init(struct perf_event *event)
>  {
>         u64 cfg = event->attr.config & RAPL_EVENT_MASK;
> -       int bit, ret = 0;
> +       int bit, rapl_pmu_idx, ret = 0;
>         struct rapl_pmu *pmu;
>  
>         /* only look at RAPL events */
> @@ -376,8 +365,12 @@ static int rapl_pmu_event_init(struct perf_event
> *event)
>         if (event->attr.sample_period) /* no sampling */
>                 return -EINVAL;
>  
> +       rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
> +       if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
> +               return -EINVAL;
> +
>         /* must be done before validate_group */
> -       pmu = cpu_to_rapl_pmu(event->cpu);
> +       pmu = rapl_pmus->pmus[rapl_pmu_idx];
>         if (!pmu)
>                 return -EINVAL;
>         event->pmu_private = pmu;

[PATCH v6 04/10] perf/x86/rapl: Rename rapl_pmu variables

[Dhananjay Ugwekar]

Rename struct rapl_pmu variables from "pmu" to "rapl_pmu", to
avoid any confusion between the variables of two different
structs pmu and rapl_pmu. As rapl_pmu also contains a pointer to
struct pmu, which leads to situations in code like pmu->pmu,
which is needlessly confusing. Above scenario is replaced with
much more readable rapl_pmu->pmu with this change.

Also rename "pmus" member in rapl_pmus struct, for same reason.

No functional change.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 arch/x86/events/rapl.c | 91 +++++++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 45 deletions(-)

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index d20c5b1dd0ad..7387bca95018 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -129,7 +129,7 @@ struct rapl_pmu {
 struct rapl_pmus {
 	struct pmu		pmu;
 	unsigned int		nr_rapl_pmu;
-	struct rapl_pmu		*pmus[] __counted_by(nr_rapl_pmu);
+	struct rapl_pmu		*rapl_pmu[] __counted_by(nr_rapl_pmu);
 };
 
 enum rapl_unit_quirk {
@@ -223,34 +223,34 @@ static void rapl_start_hrtimer(struct rapl_pmu *pmu)
 
 static enum hrtimer_restart rapl_hrtimer_handle(struct hrtimer *hrtimer)
 {
-	struct rapl_pmu *pmu = container_of(hrtimer, struct rapl_pmu, hrtimer);
+	struct rapl_pmu *rapl_pmu = container_of(hrtimer, struct rapl_pmu, hrtimer);
 	struct perf_event *event;
 	unsigned long flags;
 
-	if (!pmu->n_active)
+	if (!rapl_pmu->n_active)
 		return HRTIMER_NORESTART;
 
-	raw_spin_lock_irqsave(&pmu->lock, flags);
+	raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
 
-	list_for_each_entry(event, &pmu->active_list, active_entry)
+	list_for_each_entry(event, &rapl_pmu->active_list, active_entry)
 		rapl_event_update(event);
 
-	raw_spin_unlock_irqrestore(&pmu->lock, flags);
+	raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
 
-	hrtimer_forward_now(hrtimer, pmu->timer_interval);
+	hrtimer_forward_now(hrtimer, rapl_pmu->timer_interval);
 
 	return HRTIMER_RESTART;
 }
 
-static void rapl_hrtimer_init(struct rapl_pmu *pmu)
+static void rapl_hrtimer_init(struct rapl_pmu *rapl_pmu)
 {
-	struct hrtimer *hr = &pmu->hrtimer;
+	struct hrtimer *hr = &rapl_pmu->hrtimer;
 
 	hrtimer_init(hr, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	hr->function = rapl_hrtimer_handle;
 }
 
-static void __rapl_pmu_event_start(struct rapl_pmu *pmu,
+static void __rapl_pmu_event_start(struct rapl_pmu *rapl_pmu,
 				   struct perf_event *event)
 {
 	if (WARN_ON_ONCE(!(event->hw.state & PERF_HES_STOPPED)))
@@ -258,39 +258,39 @@ static void __rapl_pmu_event_start(struct rapl_pmu *pmu,
 
 	event->hw.state = 0;
 
-	list_add_tail(&event->active_entry, &pmu->active_list);
+	list_add_tail(&event->active_entry, &rapl_pmu->active_list);
 
 	local64_set(&event->hw.prev_count, rapl_read_counter(event));
 
-	pmu->n_active++;
-	if (pmu->n_active == 1)
-		rapl_start_hrtimer(pmu);
+	rapl_pmu->n_active++;
+	if (rapl_pmu->n_active == 1)
+		rapl_start_hrtimer(rapl_pmu);
 }
 
 static void rapl_pmu_event_start(struct perf_event *event, int mode)
 {
-	struct rapl_pmu *pmu = event->pmu_private;
+	struct rapl_pmu *rapl_pmu = event->pmu_private;
 	unsigned long flags;
 
-	raw_spin_lock_irqsave(&pmu->lock, flags);
-	__rapl_pmu_event_start(pmu, event);
-	raw_spin_unlock_irqrestore(&pmu->lock, flags);
+	raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
+	__rapl_pmu_event_start(rapl_pmu, event);
+	raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
 }
 
 static void rapl_pmu_event_stop(struct perf_event *event, int mode)
 {
-	struct rapl_pmu *pmu = event->pmu_private;
+	struct rapl_pmu *rapl_pmu = event->pmu_private;
 	struct hw_perf_event *hwc = &event->hw;
 	unsigned long flags;
 
-	raw_spin_lock_irqsave(&pmu->lock, flags);
+	raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
 
 	/* mark event as deactivated and stopped */
 	if (!(hwc->state & PERF_HES_STOPPED)) {
-		WARN_ON_ONCE(pmu->n_active <= 0);
-		pmu->n_active--;
-		if (pmu->n_active == 0)
-			hrtimer_cancel(&pmu->hrtimer);
+		WARN_ON_ONCE(rapl_pmu->n_active <= 0);
+		rapl_pmu->n_active--;
+		if (rapl_pmu->n_active == 0)
+			hrtimer_cancel(&rapl_pmu->hrtimer);
 
 		list_del(&event->active_entry);
 
@@ -308,23 +308,23 @@ static void rapl_pmu_event_stop(struct perf_event *event, int mode)
 		hwc->state |= PERF_HES_UPTODATE;
 	}
 
-	raw_spin_unlock_irqrestore(&pmu->lock, flags);
+	raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
 }
 
 static int rapl_pmu_event_add(struct perf_event *event, int mode)
 {
-	struct rapl_pmu *pmu = event->pmu_private;
+	struct rapl_pmu *rapl_pmu = event->pmu_private;
 	struct hw_perf_event *hwc = &event->hw;
 	unsigned long flags;
 
-	raw_spin_lock_irqsave(&pmu->lock, flags);
+	raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
 
 	hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
 
 	if (mode & PERF_EF_START)
-		__rapl_pmu_event_start(pmu, event);
+		__rapl_pmu_event_start(rapl_pmu, event);
 
-	raw_spin_unlock_irqrestore(&pmu->lock, flags);
+	raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
 
 	return 0;
 }
@@ -338,7 +338,7 @@ static int rapl_pmu_event_init(struct perf_event *event)
 {
 	u64 cfg = event->attr.config & RAPL_EVENT_MASK;
 	int bit, rapl_pmu_idx, ret = 0;
-	struct rapl_pmu *pmu;
+	struct rapl_pmu *rapl_pmu;
 
 	/* only look at RAPL events */
 	if (event->attr.type != rapl_pmus->pmu.type)
@@ -370,10 +370,11 @@ static int rapl_pmu_event_init(struct perf_event *event)
 		return -EINVAL;
 
 	/* must be done before validate_group */
-	pmu = rapl_pmus->pmus[rapl_pmu_idx];
-	if (!pmu)
+	rapl_pmu = rapl_pmus->rapl_pmu[rapl_pmu_idx];
+	if (!rapl_pmu)
 		return -EINVAL;
-	event->pmu_private = pmu;
+
+	event->pmu_private = rapl_pmu;
 	event->hw.event_base = rapl_msrs[bit].msr;
 	event->hw.config = cfg;
 	event->hw.idx = bit;
@@ -600,7 +601,7 @@ static void cleanup_rapl_pmus(void)
 	int i;
 
 	for (i = 0; i < rapl_pmus->nr_rapl_pmu; i++)
-		kfree(rapl_pmus->pmus[i]);
+		kfree(rapl_pmus->rapl_pmu[i]);
 	kfree(rapl_pmus);
 }
 
@@ -615,27 +616,27 @@ static const struct attribute_group *rapl_attr_update[] = {
 
 static int __init init_rapl_pmu(void)
 {
-	struct rapl_pmu *pmu;
+	struct rapl_pmu *rapl_pmu;
 	int idx;
 
 	for (idx = 0; idx < rapl_pmus->nr_rapl_pmu; idx++) {
-		pmu = kzalloc(sizeof(*pmu), GFP_KERNEL);
-		if (!pmu)
+		rapl_pmu = kzalloc(sizeof(*rapl_pmu), GFP_KERNEL);
+		if (!rapl_pmu)
 			goto free;
 
-		raw_spin_lock_init(&pmu->lock);
-		INIT_LIST_HEAD(&pmu->active_list);
-		pmu->pmu = &rapl_pmus->pmu;
-		pmu->timer_interval = ms_to_ktime(rapl_timer_ms);
-		rapl_hrtimer_init(pmu);
+		raw_spin_lock_init(&rapl_pmu->lock);
+		INIT_LIST_HEAD(&rapl_pmu->active_list);
+		rapl_pmu->pmu = &rapl_pmus->pmu;
+		rapl_pmu->timer_interval = ms_to_ktime(rapl_timer_ms);
+		rapl_hrtimer_init(rapl_pmu);
 
-		rapl_pmus->pmus[idx] = pmu;
+		rapl_pmus->rapl_pmu[idx] = rapl_pmu;
 	}
 
 	return 0;
 free:
 	for (; idx > 0; idx--)
-		kfree(rapl_pmus->pmus[idx - 1]);
+		kfree(rapl_pmus->rapl_pmu[idx - 1]);
 	return -ENOMEM;
 }
 
@@ -649,7 +650,7 @@ static int __init init_rapl_pmus(void)
 		rapl_pmu_scope = PERF_PMU_SCOPE_DIE;
 	}
 
-	rapl_pmus = kzalloc(struct_size(rapl_pmus, pmus, nr_rapl_pmu), GFP_KERNEL);
+	rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu, nr_rapl_pmu), GFP_KERNEL);
 	if (!rapl_pmus)
 		return -ENOMEM;
 
-- 
2.34.1

Re: [PATCH v6 04/10] perf/x86/rapl: Rename rapl_pmu variables

[Gautham R. Shenoy]

Hello Dhananjay,

On Fri, Oct 25, 2024 at 11:13:42AM +0000, Dhananjay Ugwekar wrote:
> Rename struct rapl_pmu variables from "pmu" to "rapl_pmu", to
> avoid any confusion between the variables of two different
> structs pmu and rapl_pmu. As rapl_pmu also contains a pointer to
> struct pmu, which leads to situations in code like pmu->pmu,
> which is needlessly confusing. Above scenario is replaced with
> much more readable rapl_pmu->pmu with this change.
> 
> Also rename "pmus" member in rapl_pmus struct, for same reason.
> 
> No functional change.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

This looks good to me.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
--
Thanks and Regards
gautham.

> ---
>  arch/x86/events/rapl.c | 91 +++++++++++++++++++++---------------------
>  1 file changed, 46 insertions(+), 45 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index d20c5b1dd0ad..7387bca95018 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -129,7 +129,7 @@ struct rapl_pmu {
>  struct rapl_pmus {
>  	struct pmu		pmu;
>  	unsigned int		nr_rapl_pmu;
> -	struct rapl_pmu		*pmus[] __counted_by(nr_rapl_pmu);
> +	struct rapl_pmu		*rapl_pmu[] __counted_by(nr_rapl_pmu);
>  };
>  
>  enum rapl_unit_quirk {
> @@ -223,34 +223,34 @@ static void rapl_start_hrtimer(struct rapl_pmu *pmu)
>  
>  static enum hrtimer_restart rapl_hrtimer_handle(struct hrtimer *hrtimer)
>  {
> -	struct rapl_pmu *pmu = container_of(hrtimer, struct rapl_pmu, hrtimer);
> +	struct rapl_pmu *rapl_pmu = container_of(hrtimer, struct rapl_pmu, hrtimer);
>  	struct perf_event *event;
>  	unsigned long flags;
>  
> -	if (!pmu->n_active)
> +	if (!rapl_pmu->n_active)
>  		return HRTIMER_NORESTART;
>  
> -	raw_spin_lock_irqsave(&pmu->lock, flags);
> +	raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
>  
> -	list_for_each_entry(event, &pmu->active_list, active_entry)
> +	list_for_each_entry(event, &rapl_pmu->active_list, active_entry)
>  		rapl_event_update(event);
>  
> -	raw_spin_unlock_irqrestore(&pmu->lock, flags);
> +	raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
>  
> -	hrtimer_forward_now(hrtimer, pmu->timer_interval);
> +	hrtimer_forward_now(hrtimer, rapl_pmu->timer_interval);
>  
>  	return HRTIMER_RESTART;
>  }
>  
> -static void rapl_hrtimer_init(struct rapl_pmu *pmu)
> +static void rapl_hrtimer_init(struct rapl_pmu *rapl_pmu)
>  {
> -	struct hrtimer *hr = &pmu->hrtimer;
> +	struct hrtimer *hr = &rapl_pmu->hrtimer;
>  
>  	hrtimer_init(hr, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>  	hr->function = rapl_hrtimer_handle;
>  }
>  
> -static void __rapl_pmu_event_start(struct rapl_pmu *pmu,
> +static void __rapl_pmu_event_start(struct rapl_pmu *rapl_pmu,
>  				   struct perf_event *event)
>  {
>  	if (WARN_ON_ONCE(!(event->hw.state & PERF_HES_STOPPED)))
> @@ -258,39 +258,39 @@ static void __rapl_pmu_event_start(struct rapl_pmu *pmu,
>  
>  	event->hw.state = 0;
>  
> -	list_add_tail(&event->active_entry, &pmu->active_list);
> +	list_add_tail(&event->active_entry, &rapl_pmu->active_list);
>  
>  	local64_set(&event->hw.prev_count, rapl_read_counter(event));
>  
> -	pmu->n_active++;
> -	if (pmu->n_active == 1)
> -		rapl_start_hrtimer(pmu);
> +	rapl_pmu->n_active++;
> +	if (rapl_pmu->n_active == 1)
> +		rapl_start_hrtimer(rapl_pmu);
>  }
>  
>  static void rapl_pmu_event_start(struct perf_event *event, int mode)
>  {
> -	struct rapl_pmu *pmu = event->pmu_private;
> +	struct rapl_pmu *rapl_pmu = event->pmu_private;
>  	unsigned long flags;
>  
> -	raw_spin_lock_irqsave(&pmu->lock, flags);
> -	__rapl_pmu_event_start(pmu, event);
> -	raw_spin_unlock_irqrestore(&pmu->lock, flags);
> +	raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
> +	__rapl_pmu_event_start(rapl_pmu, event);
> +	raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
>  }
>  
>  static void rapl_pmu_event_stop(struct perf_event *event, int mode)
>  {
> -	struct rapl_pmu *pmu = event->pmu_private;
> +	struct rapl_pmu *rapl_pmu = event->pmu_private;
>  	struct hw_perf_event *hwc = &event->hw;
>  	unsigned long flags;
>  
> -	raw_spin_lock_irqsave(&pmu->lock, flags);
> +	raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
>  
>  	/* mark event as deactivated and stopped */
>  	if (!(hwc->state & PERF_HES_STOPPED)) {
> -		WARN_ON_ONCE(pmu->n_active <= 0);
> -		pmu->n_active--;
> -		if (pmu->n_active == 0)
> -			hrtimer_cancel(&pmu->hrtimer);
> +		WARN_ON_ONCE(rapl_pmu->n_active <= 0);
> +		rapl_pmu->n_active--;
> +		if (rapl_pmu->n_active == 0)
> +			hrtimer_cancel(&rapl_pmu->hrtimer);
>  
>  		list_del(&event->active_entry);
>  
> @@ -308,23 +308,23 @@ static void rapl_pmu_event_stop(struct perf_event *event, int mode)
>  		hwc->state |= PERF_HES_UPTODATE;
>  	}
>  
> -	raw_spin_unlock_irqrestore(&pmu->lock, flags);
> +	raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
>  }
>  
>  static int rapl_pmu_event_add(struct perf_event *event, int mode)
>  {
> -	struct rapl_pmu *pmu = event->pmu_private;
> +	struct rapl_pmu *rapl_pmu = event->pmu_private;
>  	struct hw_perf_event *hwc = &event->hw;
>  	unsigned long flags;
>  
> -	raw_spin_lock_irqsave(&pmu->lock, flags);
> +	raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
>  
>  	hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
>  
>  	if (mode & PERF_EF_START)
> -		__rapl_pmu_event_start(pmu, event);
> +		__rapl_pmu_event_start(rapl_pmu, event);
>  
> -	raw_spin_unlock_irqrestore(&pmu->lock, flags);
> +	raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
>  
>  	return 0;
>  }
> @@ -338,7 +338,7 @@ static int rapl_pmu_event_init(struct perf_event *event)
>  {
>  	u64 cfg = event->attr.config & RAPL_EVENT_MASK;
>  	int bit, rapl_pmu_idx, ret = 0;
> -	struct rapl_pmu *pmu;
> +	struct rapl_pmu *rapl_pmu;
>  
>  	/* only look at RAPL events */
>  	if (event->attr.type != rapl_pmus->pmu.type)
> @@ -370,10 +370,11 @@ static int rapl_pmu_event_init(struct perf_event *event)
>  		return -EINVAL;
>  
>  	/* must be done before validate_group */
> -	pmu = rapl_pmus->pmus[rapl_pmu_idx];
> -	if (!pmu)
> +	rapl_pmu = rapl_pmus->rapl_pmu[rapl_pmu_idx];
> +	if (!rapl_pmu)
>  		return -EINVAL;
> -	event->pmu_private = pmu;
> +
> +	event->pmu_private = rapl_pmu;
>  	event->hw.event_base = rapl_msrs[bit].msr;
>  	event->hw.config = cfg;
>  	event->hw.idx = bit;
> @@ -600,7 +601,7 @@ static void cleanup_rapl_pmus(void)
>  	int i;
>  
>  	for (i = 0; i < rapl_pmus->nr_rapl_pmu; i++)
> -		kfree(rapl_pmus->pmus[i]);
> +		kfree(rapl_pmus->rapl_pmu[i]);
>  	kfree(rapl_pmus);
>  }
>  
> @@ -615,27 +616,27 @@ static const struct attribute_group *rapl_attr_update[] = {
>  
>  static int __init init_rapl_pmu(void)
>  {
> -	struct rapl_pmu *pmu;
> +	struct rapl_pmu *rapl_pmu;
>  	int idx;
>  
>  	for (idx = 0; idx < rapl_pmus->nr_rapl_pmu; idx++) {
> -		pmu = kzalloc(sizeof(*pmu), GFP_KERNEL);
> -		if (!pmu)
> +		rapl_pmu = kzalloc(sizeof(*rapl_pmu), GFP_KERNEL);
> +		if (!rapl_pmu)
>  			goto free;
>  
> -		raw_spin_lock_init(&pmu->lock);
> -		INIT_LIST_HEAD(&pmu->active_list);
> -		pmu->pmu = &rapl_pmus->pmu;
> -		pmu->timer_interval = ms_to_ktime(rapl_timer_ms);
> -		rapl_hrtimer_init(pmu);
> +		raw_spin_lock_init(&rapl_pmu->lock);
> +		INIT_LIST_HEAD(&rapl_pmu->active_list);
> +		rapl_pmu->pmu = &rapl_pmus->pmu;
> +		rapl_pmu->timer_interval = ms_to_ktime(rapl_timer_ms);
> +		rapl_hrtimer_init(rapl_pmu);
>  
> -		rapl_pmus->pmus[idx] = pmu;
> +		rapl_pmus->rapl_pmu[idx] = rapl_pmu;
>  	}
>  
>  	return 0;
>  free:
>  	for (; idx > 0; idx--)
> -		kfree(rapl_pmus->pmus[idx - 1]);
> +		kfree(rapl_pmus->rapl_pmu[idx - 1]);
>  	return -ENOMEM;
>  }
>  
> @@ -649,7 +650,7 @@ static int __init init_rapl_pmus(void)
>  		rapl_pmu_scope = PERF_PMU_SCOPE_DIE;
>  	}
>  
> -	rapl_pmus = kzalloc(struct_size(rapl_pmus, pmus, nr_rapl_pmu), GFP_KERNEL);
> +	rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu, nr_rapl_pmu), GFP_KERNEL);
>  	if (!rapl_pmus)
>  		return -ENOMEM;
>  
> -- 
> 2.34.1
> 

Re: [PATCH v6 04/10] perf/x86/rapl: Rename rapl_pmu variables

[Zhang, Rui]

On Fri, 2024-10-25 at 11:13 +0000, Dhananjay Ugwekar wrote:
> Rename struct rapl_pmu variables from "pmu" to "rapl_pmu", to
> avoid any confusion between the variables of two different
> structs pmu and rapl_pmu. As rapl_pmu also contains a pointer to
> struct pmu, which leads to situations in code like pmu->pmu,
> which is needlessly confusing. Above scenario is replaced with
> much more readable rapl_pmu->pmu with this change.
> 
> Also rename "pmus" member in rapl_pmus struct, for same reason.
> 
> No functional change.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

Reviewed-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui


> ---
>  arch/x86/events/rapl.c | 91 +++++++++++++++++++++-------------------
> --
>  1 file changed, 46 insertions(+), 45 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index d20c5b1dd0ad..7387bca95018 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -129,7 +129,7 @@ struct rapl_pmu {
>  struct rapl_pmus {
>         struct pmu              pmu;
>         unsigned int            nr_rapl_pmu;
> -       struct rapl_pmu         *pmus[] __counted_by(nr_rapl_pmu);
> +       struct rapl_pmu         *rapl_pmu[]
> __counted_by(nr_rapl_pmu);
>  };
>  
>  enum rapl_unit_quirk {
> @@ -223,34 +223,34 @@ static void rapl_start_hrtimer(struct rapl_pmu
> *pmu)
>  
>  static enum hrtimer_restart rapl_hrtimer_handle(struct hrtimer
> *hrtimer)
>  {
> -       struct rapl_pmu *pmu = container_of(hrtimer, struct rapl_pmu,
> hrtimer);
> +       struct rapl_pmu *rapl_pmu = container_of(hrtimer, struct
> rapl_pmu, hrtimer);
>         struct perf_event *event;
>         unsigned long flags;
>  
> -       if (!pmu->n_active)
> +       if (!rapl_pmu->n_active)
>                 return HRTIMER_NORESTART;
>  
> -       raw_spin_lock_irqsave(&pmu->lock, flags);
> +       raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
>  
> -       list_for_each_entry(event, &pmu->active_list, active_entry)
> +       list_for_each_entry(event, &rapl_pmu->active_list,
> active_entry)
>                 rapl_event_update(event);
>  
> -       raw_spin_unlock_irqrestore(&pmu->lock, flags);
> +       raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
>  
> -       hrtimer_forward_now(hrtimer, pmu->timer_interval);
> +       hrtimer_forward_now(hrtimer, rapl_pmu->timer_interval);
>  
>         return HRTIMER_RESTART;
>  }
>  
> -static void rapl_hrtimer_init(struct rapl_pmu *pmu)
> +static void rapl_hrtimer_init(struct rapl_pmu *rapl_pmu)
>  {
> -       struct hrtimer *hr = &pmu->hrtimer;
> +       struct hrtimer *hr = &rapl_pmu->hrtimer;
>  
>         hrtimer_init(hr, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>         hr->function = rapl_hrtimer_handle;
>  }
>  
> -static void __rapl_pmu_event_start(struct rapl_pmu *pmu,
> +static void __rapl_pmu_event_start(struct rapl_pmu *rapl_pmu,
>                                    struct perf_event *event)
>  {
>         if (WARN_ON_ONCE(!(event->hw.state & PERF_HES_STOPPED)))
> @@ -258,39 +258,39 @@ static void __rapl_pmu_event_start(struct
> rapl_pmu *pmu,
>  
>         event->hw.state = 0;
>  
> -       list_add_tail(&event->active_entry, &pmu->active_list);
> +       list_add_tail(&event->active_entry, &rapl_pmu->active_list);
>  
>         local64_set(&event->hw.prev_count, rapl_read_counter(event));
>  
> -       pmu->n_active++;
> -       if (pmu->n_active == 1)
> -               rapl_start_hrtimer(pmu);
> +       rapl_pmu->n_active++;
> +       if (rapl_pmu->n_active == 1)
> +               rapl_start_hrtimer(rapl_pmu);
>  }
>  
>  static void rapl_pmu_event_start(struct perf_event *event, int mode)
>  {
> -       struct rapl_pmu *pmu = event->pmu_private;
> +       struct rapl_pmu *rapl_pmu = event->pmu_private;
>         unsigned long flags;
>  
> -       raw_spin_lock_irqsave(&pmu->lock, flags);
> -       __rapl_pmu_event_start(pmu, event);
> -       raw_spin_unlock_irqrestore(&pmu->lock, flags);
> +       raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
> +       __rapl_pmu_event_start(rapl_pmu, event);
> +       raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
>  }
>  
>  static void rapl_pmu_event_stop(struct perf_event *event, int mode)
>  {
> -       struct rapl_pmu *pmu = event->pmu_private;
> +       struct rapl_pmu *rapl_pmu = event->pmu_private;
>         struct hw_perf_event *hwc = &event->hw;
>         unsigned long flags;
>  
> -       raw_spin_lock_irqsave(&pmu->lock, flags);
> +       raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
>  
>         /* mark event as deactivated and stopped */
>         if (!(hwc->state & PERF_HES_STOPPED)) {
> -               WARN_ON_ONCE(pmu->n_active <= 0);
> -               pmu->n_active--;
> -               if (pmu->n_active == 0)
> -                       hrtimer_cancel(&pmu->hrtimer);
> +               WARN_ON_ONCE(rapl_pmu->n_active <= 0);
> +               rapl_pmu->n_active--;
> +               if (rapl_pmu->n_active == 0)
> +                       hrtimer_cancel(&rapl_pmu->hrtimer);
>  
>                 list_del(&event->active_entry);
>  
> @@ -308,23 +308,23 @@ static void rapl_pmu_event_stop(struct
> perf_event *event, int mode)
>                 hwc->state |= PERF_HES_UPTODATE;
>         }
>  
> -       raw_spin_unlock_irqrestore(&pmu->lock, flags);
> +       raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
>  }
>  
>  static int rapl_pmu_event_add(struct perf_event *event, int mode)
>  {
> -       struct rapl_pmu *pmu = event->pmu_private;
> +       struct rapl_pmu *rapl_pmu = event->pmu_private;
>         struct hw_perf_event *hwc = &event->hw;
>         unsigned long flags;
>  
> -       raw_spin_lock_irqsave(&pmu->lock, flags);
> +       raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
>  
>         hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
>  
>         if (mode & PERF_EF_START)
> -               __rapl_pmu_event_start(pmu, event);
> +               __rapl_pmu_event_start(rapl_pmu, event);
>  
> -       raw_spin_unlock_irqrestore(&pmu->lock, flags);
> +       raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
>  
>         return 0;
>  }
> @@ -338,7 +338,7 @@ static int rapl_pmu_event_init(struct perf_event
> *event)
>  {
>         u64 cfg = event->attr.config & RAPL_EVENT_MASK;
>         int bit, rapl_pmu_idx, ret = 0;
> -       struct rapl_pmu *pmu;
> +       struct rapl_pmu *rapl_pmu;
>  
>         /* only look at RAPL events */
>         if (event->attr.type != rapl_pmus->pmu.type)
> @@ -370,10 +370,11 @@ static int rapl_pmu_event_init(struct
> perf_event *event)
>                 return -EINVAL;
>  
>         /* must be done before validate_group */
> -       pmu = rapl_pmus->pmus[rapl_pmu_idx];
> -       if (!pmu)
> +       rapl_pmu = rapl_pmus->rapl_pmu[rapl_pmu_idx];
> +       if (!rapl_pmu)
>                 return -EINVAL;
> -       event->pmu_private = pmu;
> +
> +       event->pmu_private = rapl_pmu;
>         event->hw.event_base = rapl_msrs[bit].msr;
>         event->hw.config = cfg;
>         event->hw.idx = bit;
> @@ -600,7 +601,7 @@ static void cleanup_rapl_pmus(void)
>         int i;
>  
>         for (i = 0; i < rapl_pmus->nr_rapl_pmu; i++)
> -               kfree(rapl_pmus->pmus[i]);
> +               kfree(rapl_pmus->rapl_pmu[i]);
>         kfree(rapl_pmus);
>  }
>  
> @@ -615,27 +616,27 @@ static const struct attribute_group
> *rapl_attr_update[] = {
>  
>  static int __init init_rapl_pmu(void)
>  {
> -       struct rapl_pmu *pmu;
> +       struct rapl_pmu *rapl_pmu;
>         int idx;
>  
>         for (idx = 0; idx < rapl_pmus->nr_rapl_pmu; idx++) {
> -               pmu = kzalloc(sizeof(*pmu), GFP_KERNEL);
> -               if (!pmu)
> +               rapl_pmu = kzalloc(sizeof(*rapl_pmu), GFP_KERNEL);
> +               if (!rapl_pmu)
>                         goto free;
>  
> -               raw_spin_lock_init(&pmu->lock);
> -               INIT_LIST_HEAD(&pmu->active_list);
> -               pmu->pmu = &rapl_pmus->pmu;
> -               pmu->timer_interval = ms_to_ktime(rapl_timer_ms);
> -               rapl_hrtimer_init(pmu);
> +               raw_spin_lock_init(&rapl_pmu->lock);
> +               INIT_LIST_HEAD(&rapl_pmu->active_list);
> +               rapl_pmu->pmu = &rapl_pmus->pmu;
> +               rapl_pmu->timer_interval =
> ms_to_ktime(rapl_timer_ms);
> +               rapl_hrtimer_init(rapl_pmu);
>  
> -               rapl_pmus->pmus[idx] = pmu;
> +               rapl_pmus->rapl_pmu[idx] = rapl_pmu;
>         }
>  
>         return 0;
>  free:
>         for (; idx > 0; idx--)
> -               kfree(rapl_pmus->pmus[idx - 1]);
> +               kfree(rapl_pmus->rapl_pmu[idx - 1]);
>         return -ENOMEM;
>  }
>  
> @@ -649,7 +650,7 @@ static int __init init_rapl_pmus(void)
>                 rapl_pmu_scope = PERF_PMU_SCOPE_DIE;
>         }
>  
> -       rapl_pmus = kzalloc(struct_size(rapl_pmus, pmus,
> nr_rapl_pmu), GFP_KERNEL);
> +       rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu,
> nr_rapl_pmu), GFP_KERNEL);
>         if (!rapl_pmus)
>                 return -ENOMEM;
>  

[PATCH v6 05/10] perf/x86/rapl: Make rapl_model struct global

[Dhananjay Ugwekar]

Prepare for the addition of RAPL core energy counter support.

As there will always be just one rapl_model variable on a system, make it
global, to make it easier to access it from any function.

No functional change.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 arch/x86/events/rapl.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index 7387bca95018..447f62caa5f9 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -151,6 +151,7 @@ static struct rapl_pmus *rapl_pmus;
 static unsigned int rapl_cntr_mask;
 static u64 rapl_timer_ms;
 static struct perf_msr *rapl_msrs;
+static struct rapl_model *rapl_model;
 
 /*
  * Helper function to get the correct topology id according to the
@@ -536,18 +537,18 @@ static struct perf_msr amd_rapl_msrs[] = {
 	[PERF_RAPL_PSYS] = { 0, &rapl_events_psys_group,  NULL, false, 0 },
 };
 
-static int rapl_check_hw_unit(struct rapl_model *rm)
+static int rapl_check_hw_unit(void)
 {
 	u64 msr_rapl_power_unit_bits;
 	int i;
 
 	/* protect rdmsrl() to handle virtualization */
-	if (rdmsrl_safe(rm->msr_power_unit, &msr_rapl_power_unit_bits))
+	if (rdmsrl_safe(rapl_model->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;
 
-	switch (rm->unit_quirk) {
+	switch (rapl_model->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
@@ -792,21 +793,20 @@ MODULE_DEVICE_TABLE(x86cpu, rapl_model_match);
 static int __init rapl_pmu_init(void)
 {
 	const struct x86_cpu_id *id;
-	struct rapl_model *rm;
 	int ret;
 
 	id = x86_match_cpu(rapl_model_match);
 	if (!id)
 		return -ENODEV;
 
-	rm = (struct rapl_model *) id->driver_data;
+	rapl_model = (struct rapl_model *) id->driver_data;
 
-	rapl_msrs = rm->rapl_msrs;
+	rapl_msrs = rapl_model->rapl_msrs;
 
 	rapl_cntr_mask = perf_msr_probe(rapl_msrs, PERF_RAPL_MAX,
-					false, (void *) &rm->events);
+					false, (void *) &rapl_model->events);
 
-	ret = rapl_check_hw_unit(rm);
+	ret = rapl_check_hw_unit();
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH v6 05/10] perf/x86/rapl: Make rapl_model struct global

[Zhang, Rui]

On Fri, 2024-10-25 at 11:13 +0000, Dhananjay Ugwekar wrote:
> Prepare for the addition of RAPL core energy counter support.
> 
> As there will always be just one rapl_model variable on a system,
> make it
> global, to make it easier to access it from any function.
> 
> No functional change.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

Reviewed-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui

> ---
>  arch/x86/events/rapl.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index 7387bca95018..447f62caa5f9 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -151,6 +151,7 @@ static struct rapl_pmus *rapl_pmus;
>  static unsigned int rapl_cntr_mask;
>  static u64 rapl_timer_ms;
>  static struct perf_msr *rapl_msrs;
> +static struct rapl_model *rapl_model;
>  
>  /*
>   * Helper function to get the correct topology id according to the
> @@ -536,18 +537,18 @@ static struct perf_msr amd_rapl_msrs[] = {
>         [PERF_RAPL_PSYS] = { 0, &rapl_events_psys_group,  NULL,
> false, 0 },
>  };
>  
> -static int rapl_check_hw_unit(struct rapl_model *rm)
> +static int rapl_check_hw_unit(void)
>  {
>         u64 msr_rapl_power_unit_bits;
>         int i;
>  
>         /* protect rdmsrl() to handle virtualization */
> -       if (rdmsrl_safe(rm->msr_power_unit,
> &msr_rapl_power_unit_bits))
> +       if (rdmsrl_safe(rapl_model->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;
>  
> -       switch (rm->unit_quirk) {
> +       switch (rapl_model->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
> @@ -792,21 +793,20 @@ MODULE_DEVICE_TABLE(x86cpu, rapl_model_match);
>  static int __init rapl_pmu_init(void)
>  {
>         const struct x86_cpu_id *id;
> -       struct rapl_model *rm;
>         int ret;
>  
>         id = x86_match_cpu(rapl_model_match);
>         if (!id)
>                 return -ENODEV;
>  
> -       rm = (struct rapl_model *) id->driver_data;
> +       rapl_model = (struct rapl_model *) id->driver_data;
>  
> -       rapl_msrs = rm->rapl_msrs;
> +       rapl_msrs = rapl_model->rapl_msrs;
>  
>         rapl_cntr_mask = perf_msr_probe(rapl_msrs, PERF_RAPL_MAX,
> -                                       false, (void *) &rm->events);
> +                                       false, (void *) &rapl_model-
> >events);
>  
> -       ret = rapl_check_hw_unit(rm);
> +       ret = rapl_check_hw_unit();
>         if (ret)
>                 return ret;
>  

Re: [PATCH v6 05/10] perf/x86/rapl: Make rapl_model struct global

[Gautham R. Shenoy]

On Fri, Oct 25, 2024 at 11:13:43AM +0000, Dhananjay Ugwekar wrote:
> Prepare for the addition of RAPL core energy counter support.
> 
> As there will always be just one rapl_model variable on a system, make it
> global, to make it easier to access it from any function.
> 
> No functional change.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

LGTM.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

> ---
>  arch/x86/events/rapl.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index 7387bca95018..447f62caa5f9 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -151,6 +151,7 @@ static struct rapl_pmus *rapl_pmus;
>  static unsigned int rapl_cntr_mask;
>  static u64 rapl_timer_ms;
>  static struct perf_msr *rapl_msrs;
> +static struct rapl_model *rapl_model;
>  
>  /*
>   * Helper function to get the correct topology id according to the
> @@ -536,18 +537,18 @@ static struct perf_msr amd_rapl_msrs[] = {
>  	[PERF_RAPL_PSYS] = { 0, &rapl_events_psys_group,  NULL, false, 0 },
>  };
>  
> -static int rapl_check_hw_unit(struct rapl_model *rm)
> +static int rapl_check_hw_unit(void)
>  {
>  	u64 msr_rapl_power_unit_bits;
>  	int i;
>  
>  	/* protect rdmsrl() to handle virtualization */
> -	if (rdmsrl_safe(rm->msr_power_unit, &msr_rapl_power_unit_bits))
> +	if (rdmsrl_safe(rapl_model->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;
>  
> -	switch (rm->unit_quirk) {
> +	switch (rapl_model->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
> @@ -792,21 +793,20 @@ MODULE_DEVICE_TABLE(x86cpu, rapl_model_match);
>  static int __init rapl_pmu_init(void)
>  {
>  	const struct x86_cpu_id *id;
> -	struct rapl_model *rm;
>  	int ret;
>  
>  	id = x86_match_cpu(rapl_model_match);
>  	if (!id)
>  		return -ENODEV;
>  
> -	rm = (struct rapl_model *) id->driver_data;
> +	rapl_model = (struct rapl_model *) id->driver_data;
>  
> -	rapl_msrs = rm->rapl_msrs;
> +	rapl_msrs = rapl_model->rapl_msrs;
>  
>  	rapl_cntr_mask = perf_msr_probe(rapl_msrs, PERF_RAPL_MAX,
> -					false, (void *) &rm->events);
> +					false, (void *) &rapl_model->events);
>  
> -	ret = rapl_check_hw_unit(rm);
> +	ret = rapl_check_hw_unit();
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.34.1
> 

[PATCH v6 06/10] perf/x86/rapl: Add arguments to the init and cleanup functions

[Dhananjay Ugwekar]

Prepare for the addition of RAPL core energy counter support.

Add arguments to the init and cleanup functions, which will help in
initialization and cleaning up of two separate PMUs.

No functional change.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 arch/x86/events/rapl.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index 447f62caa5f9..bf6fee114294 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -597,7 +597,7 @@ static void __init rapl_advertise(void)
 	}
 }
 
-static void cleanup_rapl_pmus(void)
+static void cleanup_rapl_pmus(struct rapl_pmus *rapl_pmus)
 {
 	int i;
 
@@ -615,7 +615,7 @@ static const struct attribute_group *rapl_attr_update[] = {
 	NULL,
 };
 
-static int __init init_rapl_pmu(void)
+static int __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
 {
 	struct rapl_pmu *rapl_pmu;
 	int idx;
@@ -641,20 +641,20 @@ static int __init init_rapl_pmu(void)
 	return -ENOMEM;
 }
 
-static int __init init_rapl_pmus(void)
+static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr, int rapl_pmu_scope)
 {
 	int nr_rapl_pmu = topology_max_packages();
-	int rapl_pmu_scope = PERF_PMU_SCOPE_PKG;
+	struct rapl_pmus *rapl_pmus;
 
-	if (!rapl_pmu_is_pkg_scope()) {
-		nr_rapl_pmu *= topology_max_dies_per_package();
-		rapl_pmu_scope = PERF_PMU_SCOPE_DIE;
-	}
+	if (rapl_pmu_scope == PERF_PMU_SCOPE_DIE)
+		nr_rapl_pmu	*= topology_max_dies_per_package();
 
 	rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu, nr_rapl_pmu), GFP_KERNEL);
 	if (!rapl_pmus)
 		return -ENOMEM;
 
+	*rapl_pmus_ptr = rapl_pmus;
+
 	rapl_pmus->nr_rapl_pmu		= nr_rapl_pmu;
 	rapl_pmus->pmu.attr_groups	= rapl_attr_groups;
 	rapl_pmus->pmu.attr_update	= rapl_attr_update;
@@ -669,7 +669,7 @@ static int __init init_rapl_pmus(void)
 	rapl_pmus->pmu.module		= THIS_MODULE;
 	rapl_pmus->pmu.capabilities	= PERF_PMU_CAP_NO_EXCLUDE;
 
-	return init_rapl_pmu();
+	return init_rapl_pmu(rapl_pmus);
 }
 
 static struct rapl_model model_snb = {
@@ -793,8 +793,12 @@ MODULE_DEVICE_TABLE(x86cpu, rapl_model_match);
 static int __init rapl_pmu_init(void)
 {
 	const struct x86_cpu_id *id;
+	int rapl_pmu_scope = PERF_PMU_SCOPE_DIE;
 	int ret;
 
+	if (rapl_pmu_is_pkg_scope())
+		rapl_pmu_scope = PERF_PMU_SCOPE_PKG;
+
 	id = x86_match_cpu(rapl_model_match);
 	if (!id)
 		return -ENODEV;
@@ -810,7 +814,7 @@ static int __init rapl_pmu_init(void)
 	if (ret)
 		return ret;
 
-	ret = init_rapl_pmus();
+	ret = init_rapl_pmus(&rapl_pmus, rapl_pmu_scope);
 	if (ret)
 		return ret;
 
@@ -823,7 +827,7 @@ static int __init rapl_pmu_init(void)
 
 out:
 	pr_warn("Initialization failed (%d), disabled\n", ret);
-	cleanup_rapl_pmus();
+	cleanup_rapl_pmus(rapl_pmus);
 	return ret;
 }
 module_init(rapl_pmu_init);
@@ -831,6 +835,6 @@ module_init(rapl_pmu_init);
 static void __exit intel_rapl_exit(void)
 {
 	perf_pmu_unregister(&rapl_pmus->pmu);
-	cleanup_rapl_pmus();
+	cleanup_rapl_pmus(rapl_pmus);
 }
 module_exit(intel_rapl_exit);
-- 
2.34.1

Re: [PATCH v6 06/10] perf/x86/rapl: Add arguments to the init and cleanup functions

[Gautham R. Shenoy]

Hello Dhananjay,


On Fri, Oct 25, 2024 at 11:13:44AM +0000, Dhananjay Ugwekar wrote:
> Prepare for the addition of RAPL core energy counter support.
> 
> Add arguments to the init and cleanup functions, which will help in
> initialization and cleaning up of two separate PMUs.
> 
> No functional change.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

The patch looks good to me.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

> ---
>  arch/x86/events/rapl.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index 447f62caa5f9..bf6fee114294 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -597,7 +597,7 @@ static void __init rapl_advertise(void)
>  	}
>  }
>  
> -static void cleanup_rapl_pmus(void)
> +static void cleanup_rapl_pmus(struct rapl_pmus *rapl_pmus)
>  {
>  	int i;
>  
> @@ -615,7 +615,7 @@ static const struct attribute_group *rapl_attr_update[] = {
>  	NULL,
>  };
>  
> -static int __init init_rapl_pmu(void)
> +static int __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
>  {
>  	struct rapl_pmu *rapl_pmu;
>  	int idx;
> @@ -641,20 +641,20 @@ static int __init init_rapl_pmu(void)
>  	return -ENOMEM;
>  }
>  
> -static int __init init_rapl_pmus(void)
> +static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr, int rapl_pmu_scope)
>  {
>  	int nr_rapl_pmu = topology_max_packages();
> -	int rapl_pmu_scope = PERF_PMU_SCOPE_PKG;
> +	struct rapl_pmus *rapl_pmus;
>  
> -	if (!rapl_pmu_is_pkg_scope()) {
> -		nr_rapl_pmu *= topology_max_dies_per_package();
> -		rapl_pmu_scope = PERF_PMU_SCOPE_DIE;
> -	}
> +	if (rapl_pmu_scope == PERF_PMU_SCOPE_DIE)
> +		nr_rapl_pmu	*= topology_max_dies_per_package();
>  
>  	rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu, nr_rapl_pmu), GFP_KERNEL);
>  	if (!rapl_pmus)
>  		return -ENOMEM;
>  
> +	*rapl_pmus_ptr = rapl_pmus;
> +
>  	rapl_pmus->nr_rapl_pmu		= nr_rapl_pmu;
>  	rapl_pmus->pmu.attr_groups	= rapl_attr_groups;
>  	rapl_pmus->pmu.attr_update	= rapl_attr_update;
> @@ -669,7 +669,7 @@ static int __init init_rapl_pmus(void)
>  	rapl_pmus->pmu.module		= THIS_MODULE;
>  	rapl_pmus->pmu.capabilities	= PERF_PMU_CAP_NO_EXCLUDE;
>  
> -	return init_rapl_pmu();
> +	return init_rapl_pmu(rapl_pmus);
>  }
>  
>  static struct rapl_model model_snb = {
> @@ -793,8 +793,12 @@ MODULE_DEVICE_TABLE(x86cpu, rapl_model_match);
>  static int __init rapl_pmu_init(void)
>  {
>  	const struct x86_cpu_id *id;
> +	int rapl_pmu_scope = PERF_PMU_SCOPE_DIE;
>  	int ret;
>  
> +	if (rapl_pmu_is_pkg_scope())
> +		rapl_pmu_scope = PERF_PMU_SCOPE_PKG;
> +
>  	id = x86_match_cpu(rapl_model_match);
>  	if (!id)
>  		return -ENODEV;
> @@ -810,7 +814,7 @@ static int __init rapl_pmu_init(void)
>  	if (ret)
>  		return ret;
>  
> -	ret = init_rapl_pmus();
> +	ret = init_rapl_pmus(&rapl_pmus, rapl_pmu_scope);
>  	if (ret)
>  		return ret;
>  
> @@ -823,7 +827,7 @@ static int __init rapl_pmu_init(void)
>  
>  out:
>  	pr_warn("Initialization failed (%d), disabled\n", ret);
> -	cleanup_rapl_pmus();
> +	cleanup_rapl_pmus(rapl_pmus);
>  	return ret;
>  }
>  module_init(rapl_pmu_init);
> @@ -831,6 +835,6 @@ module_init(rapl_pmu_init);
>  static void __exit intel_rapl_exit(void)
>  {
>  	perf_pmu_unregister(&rapl_pmus->pmu);
> -	cleanup_rapl_pmus();
> +	cleanup_rapl_pmus(rapl_pmus);
>  }
>  module_exit(intel_rapl_exit);
> -- 
> 2.34.1
> 

Re: [PATCH v6 06/10] perf/x86/rapl: Add arguments to the init and cleanup functions

[Zhang, Rui]

On Fri, 2024-10-25 at 11:13 +0000, Dhananjay Ugwekar wrote:
> Prepare for the addition of RAPL core energy counter support.
> 
> Add arguments to the init and cleanup functions, which will help in
> initialization and cleaning up of two separate PMUs.
> 
> No functional change.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

Reviewed-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui


> ---
>  arch/x86/events/rapl.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index 447f62caa5f9..bf6fee114294 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -597,7 +597,7 @@ static void __init rapl_advertise(void)
>         }
>  }
>  
> -static void cleanup_rapl_pmus(void)
> +static void cleanup_rapl_pmus(struct rapl_pmus *rapl_pmus)
>  {
>         int i;
>  
> @@ -615,7 +615,7 @@ static const struct attribute_group
> *rapl_attr_update[] = {
>         NULL,
>  };
>  
> -static int __init init_rapl_pmu(void)
> +static int __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
>  {
>         struct rapl_pmu *rapl_pmu;
>         int idx;
> @@ -641,20 +641,20 @@ static int __init init_rapl_pmu(void)
>         return -ENOMEM;
>  }
>  
> -static int __init init_rapl_pmus(void)
> +static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr,
> int rapl_pmu_scope)
>  {
>         int nr_rapl_pmu = topology_max_packages();
> -       int rapl_pmu_scope = PERF_PMU_SCOPE_PKG;
> +       struct rapl_pmus *rapl_pmus;
>  
> -       if (!rapl_pmu_is_pkg_scope()) {
> -               nr_rapl_pmu *= topology_max_dies_per_package();
> -               rapl_pmu_scope = PERF_PMU_SCOPE_DIE;
> -       }
> +       if (rapl_pmu_scope == PERF_PMU_SCOPE_DIE)
> +               nr_rapl_pmu     *= topology_max_dies_per_package();
>  
>         rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu,
> nr_rapl_pmu), GFP_KERNEL);
>         if (!rapl_pmus)
>                 return -ENOMEM;
>  
> +       *rapl_pmus_ptr = rapl_pmus;
> +
>         rapl_pmus->nr_rapl_pmu          = nr_rapl_pmu;
>         rapl_pmus->pmu.attr_groups      = rapl_attr_groups;
>         rapl_pmus->pmu.attr_update      = rapl_attr_update;
> @@ -669,7 +669,7 @@ static int __init init_rapl_pmus(void)
>         rapl_pmus->pmu.module           = THIS_MODULE;
>         rapl_pmus->pmu.capabilities     = PERF_PMU_CAP_NO_EXCLUDE;
>  
> -       return init_rapl_pmu();
> +       return init_rapl_pmu(rapl_pmus);
>  }
>  
>  static struct rapl_model model_snb = {
> @@ -793,8 +793,12 @@ MODULE_DEVICE_TABLE(x86cpu, rapl_model_match);
>  static int __init rapl_pmu_init(void)
>  {
>         const struct x86_cpu_id *id;
> +       int rapl_pmu_scope = PERF_PMU_SCOPE_DIE;
>         int ret;
>  
> +       if (rapl_pmu_is_pkg_scope())
> +               rapl_pmu_scope = PERF_PMU_SCOPE_PKG;
> +
>         id = x86_match_cpu(rapl_model_match);
>         if (!id)
>                 return -ENODEV;
> @@ -810,7 +814,7 @@ static int __init rapl_pmu_init(void)
>         if (ret)
>                 return ret;
>  
> -       ret = init_rapl_pmus();
> +       ret = init_rapl_pmus(&rapl_pmus, rapl_pmu_scope);
>         if (ret)
>                 return ret;
>  
> @@ -823,7 +827,7 @@ static int __init rapl_pmu_init(void)
>  
>  out:
>         pr_warn("Initialization failed (%d), disabled\n", ret);
> -       cleanup_rapl_pmus();
> +       cleanup_rapl_pmus(rapl_pmus);
>         return ret;
>  }
>  module_init(rapl_pmu_init);
> @@ -831,6 +835,6 @@ module_init(rapl_pmu_init);
>  static void __exit intel_rapl_exit(void)
>  {
>         perf_pmu_unregister(&rapl_pmus->pmu);
> -       cleanup_rapl_pmus();
> +       cleanup_rapl_pmus(rapl_pmus);
>  }
>  module_exit(intel_rapl_exit);

[PATCH v6 07/10] perf/x86/rapl: Modify the generic variable names to *_pkg*

[Dhananjay Ugwekar]

Prepare for the addition of RAPL core energy counter support.

Replace the generic names with *_pkg*, to later on differentiate between 
the scopes of the two different PMUs and their variables.

No functional change.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 arch/x86/events/rapl.c | 118 ++++++++++++++++++++---------------------
 1 file changed, 59 insertions(+), 59 deletions(-)

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index bf6fee114294..ae8b450caa9b 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -70,18 +70,18 @@ MODULE_LICENSE("GPL");
 /*
  * RAPL energy status counters
  */
-enum perf_rapl_events {
+enum perf_rapl_pkg_events {
 	PERF_RAPL_PP0 = 0,		/* all cores */
 	PERF_RAPL_PKG,			/* entire package */
 	PERF_RAPL_RAM,			/* DRAM */
 	PERF_RAPL_PP1,			/* gpu */
 	PERF_RAPL_PSYS,			/* psys */
 
-	PERF_RAPL_MAX,
-	NR_RAPL_DOMAINS = PERF_RAPL_MAX,
+	PERF_RAPL_PKG_EVENTS_MAX,
+	NR_RAPL_PKG_DOMAINS = PERF_RAPL_PKG_EVENTS_MAX,
 };
 
-static const char *const rapl_domain_names[NR_RAPL_DOMAINS] __initconst = {
+static const char *const rapl_pkg_domain_names[NR_RAPL_PKG_DOMAINS] __initconst = {
 	"pp0-core",
 	"package",
 	"dram",
@@ -112,7 +112,7 @@ static struct perf_pmu_events_attr event_attr_##v = {				\
  *	     considered as either pkg-scope or die-scope, and we are considering
  *	     them as die-scope.
  */
-#define rapl_pmu_is_pkg_scope()				\
+#define rapl_pkg_pmu_is_pkg_scope()				\
 	(boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||	\
 	 boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
 
@@ -139,16 +139,16 @@ enum rapl_unit_quirk {
 };
 
 struct rapl_model {
-	struct perf_msr *rapl_msrs;
-	unsigned long	events;
+	struct perf_msr *rapl_pkg_msrs;
+	unsigned long	pkg_events;
 	unsigned int	msr_power_unit;
 	enum rapl_unit_quirk	unit_quirk;
 };
 
  /* 1/2^hw_unit Joule */
-static int rapl_hw_unit[NR_RAPL_DOMAINS] __read_mostly;
-static struct rapl_pmus *rapl_pmus;
-static unsigned int rapl_cntr_mask;
+static int rapl_pkg_hw_unit[NR_RAPL_PKG_DOMAINS] __read_mostly;
+static struct rapl_pmus *rapl_pmus_pkg;
+static unsigned int rapl_pkg_cntr_mask;
 static u64 rapl_timer_ms;
 static struct perf_msr *rapl_msrs;
 static struct rapl_model *rapl_model;
@@ -159,7 +159,7 @@ static struct rapl_model *rapl_model;
  */
 static inline unsigned int get_rapl_pmu_idx(int cpu)
 {
-	return rapl_pmu_is_pkg_scope() ? topology_logical_package_id(cpu) :
+	return rapl_pkg_pmu_is_pkg_scope() ? topology_logical_package_id(cpu) :
 					 topology_logical_die_id(cpu);
 }
 
@@ -172,7 +172,7 @@ static inline u64 rapl_read_counter(struct perf_event *event)
 
 static inline u64 rapl_scale(u64 v, int cfg)
 {
-	if (cfg > NR_RAPL_DOMAINS) {
+	if (cfg > NR_RAPL_PKG_DOMAINS) {
 		pr_warn("Invalid domain %d, failed to scale data\n", cfg);
 		return v;
 	}
@@ -182,7 +182,7 @@ static inline u64 rapl_scale(u64 v, int cfg)
 	 * or use ldexp(count, -32).
 	 * Watts = Joules/Time delta
 	 */
-	return v << (32 - rapl_hw_unit[cfg - 1]);
+	return v << (32 - rapl_pkg_hw_unit[cfg - 1]);
 }
 
 static u64 rapl_event_update(struct perf_event *event)
@@ -342,7 +342,7 @@ static int rapl_pmu_event_init(struct perf_event *event)
 	struct rapl_pmu *rapl_pmu;
 
 	/* only look at RAPL events */
-	if (event->attr.type != rapl_pmus->pmu.type)
+	if (event->attr.type != rapl_pmus_pkg->pmu.type)
 		return -ENOENT;
 
 	/* check only supported bits are set */
@@ -352,14 +352,14 @@ static int rapl_pmu_event_init(struct perf_event *event)
 	if (event->cpu < 0)
 		return -EINVAL;
 
-	if (!cfg || cfg >= NR_RAPL_DOMAINS + 1)
+	if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
 		return -EINVAL;
 
-	cfg = array_index_nospec((long)cfg, NR_RAPL_DOMAINS + 1);
+	cfg = array_index_nospec((long)cfg, NR_RAPL_PKG_DOMAINS + 1);
 	bit = cfg - 1;
 
 	/* check event supported */
-	if (!(rapl_cntr_mask & (1 << bit)))
+	if (!(rapl_pkg_cntr_mask & (1 << bit)))
 		return -EINVAL;
 
 	/* unsupported modes and filters */
@@ -367,11 +367,11 @@ static int rapl_pmu_event_init(struct perf_event *event)
 		return -EINVAL;
 
 	rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
-	if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
+	if (rapl_pmu_idx >= rapl_pmus_pkg->nr_rapl_pmu)
 		return -EINVAL;
 
 	/* must be done before validate_group */
-	rapl_pmu = rapl_pmus->rapl_pmu[rapl_pmu_idx];
+	rapl_pmu = rapl_pmus_pkg->rapl_pmu[rapl_pmu_idx];
 	if (!rapl_pmu)
 		return -EINVAL;
 
@@ -525,11 +525,11 @@ static struct perf_msr intel_rapl_spr_msrs[] = {
 };
 
 /*
- * Force to PERF_RAPL_MAX size due to:
- * - perf_msr_probe(PERF_RAPL_MAX)
+ * Force to PERF_RAPL_PKG_EVENTS_MAX size due to:
+ * - perf_msr_probe(PERF_RAPL_PKG_EVENTS_MAX)
  * - want to use same event codes across both architectures
  */
-static struct perf_msr amd_rapl_msrs[] = {
+static struct perf_msr amd_rapl_pkg_msrs[] = {
 	[PERF_RAPL_PP0]  = { 0, &rapl_events_cores_group, NULL, false, 0 },
 	[PERF_RAPL_PKG]  = { MSR_AMD_PKG_ENERGY_STATUS,  &rapl_events_pkg_group,   test_msr, false, RAPL_MSR_MASK },
 	[PERF_RAPL_RAM]  = { 0, &rapl_events_ram_group,   NULL, false, 0 },
@@ -545,8 +545,8 @@ static int rapl_check_hw_unit(void)
 	/* protect rdmsrl() to handle virtualization */
 	if (rdmsrl_safe(rapl_model->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;
+	for (i = 0; i < NR_RAPL_PKG_DOMAINS; i++)
+		rapl_pkg_hw_unit[i] = (msr_rapl_power_unit_bits >> 8) & 0x1FULL;
 
 	switch (rapl_model->unit_quirk) {
 	/*
@@ -556,11 +556,11 @@ static int rapl_check_hw_unit(void)
 	 * of 2. Datasheet, September 2014, Reference Number: 330784-001 "
 	 */
 	case RAPL_UNIT_QUIRK_INTEL_HSW:
-		rapl_hw_unit[PERF_RAPL_RAM] = 16;
+		rapl_pkg_hw_unit[PERF_RAPL_RAM] = 16;
 		break;
 	/* SPR uses a fixed energy unit for Psys domain. */
 	case RAPL_UNIT_QUIRK_INTEL_SPR:
-		rapl_hw_unit[PERF_RAPL_PSYS] = 0;
+		rapl_pkg_hw_unit[PERF_RAPL_PSYS] = 0;
 		break;
 	default:
 		break;
@@ -575,9 +575,9 @@ static int rapl_check_hw_unit(void)
 	 * if hw unit is 32, then we use 2 ms 1/200/2
 	 */
 	rapl_timer_ms = 2;
-	if (rapl_hw_unit[0] < 32) {
+	if (rapl_pkg_hw_unit[0] < 32) {
 		rapl_timer_ms = (1000 / (2 * 100));
-		rapl_timer_ms *= (1ULL << (32 - rapl_hw_unit[0] - 1));
+		rapl_timer_ms *= (1ULL << (32 - rapl_pkg_hw_unit[0] - 1));
 	}
 	return 0;
 }
@@ -587,12 +587,12 @@ static void __init rapl_advertise(void)
 	int i;
 
 	pr_info("API unit is 2^-32 Joules, %d fixed counters, %llu ms ovfl timer\n",
-		hweight32(rapl_cntr_mask), rapl_timer_ms);
+		hweight32(rapl_pkg_cntr_mask), rapl_timer_ms);
 
-	for (i = 0; i < NR_RAPL_DOMAINS; i++) {
-		if (rapl_cntr_mask & (1 << i)) {
+	for (i = 0; i < NR_RAPL_PKG_DOMAINS; i++) {
+		if (rapl_pkg_cntr_mask & (1 << i)) {
 			pr_info("hw unit of domain %s 2^-%d Joules\n",
-				rapl_domain_names[i], rapl_hw_unit[i]);
+				rapl_pkg_domain_names[i], rapl_pkg_hw_unit[i]);
 		}
 	}
 }
@@ -673,71 +673,71 @@ static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr, int rapl_pmu_
 }
 
 static struct rapl_model model_snb = {
-	.events		= BIT(PERF_RAPL_PP0) |
+	.pkg_events	= BIT(PERF_RAPL_PP0) |
 			  BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_PP1),
 	.msr_power_unit = MSR_RAPL_POWER_UNIT,
-	.rapl_msrs      = intel_rapl_msrs,
+	.rapl_pkg_msrs	= intel_rapl_msrs,
 };
 
 static struct rapl_model model_snbep = {
-	.events		= BIT(PERF_RAPL_PP0) |
+	.pkg_events	= BIT(PERF_RAPL_PP0) |
 			  BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_RAM),
 	.msr_power_unit = MSR_RAPL_POWER_UNIT,
-	.rapl_msrs      = intel_rapl_msrs,
+	.rapl_pkg_msrs	= intel_rapl_msrs,
 };
 
 static struct rapl_model model_hsw = {
-	.events		= BIT(PERF_RAPL_PP0) |
+	.pkg_events	= BIT(PERF_RAPL_PP0) |
 			  BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_RAM) |
 			  BIT(PERF_RAPL_PP1),
 	.msr_power_unit = MSR_RAPL_POWER_UNIT,
-	.rapl_msrs      = intel_rapl_msrs,
+	.rapl_pkg_msrs	= intel_rapl_msrs,
 };
 
 static struct rapl_model model_hsx = {
-	.events		= BIT(PERF_RAPL_PP0) |
+	.pkg_events	= BIT(PERF_RAPL_PP0) |
 			  BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_RAM),
 	.unit_quirk	= RAPL_UNIT_QUIRK_INTEL_HSW,
 	.msr_power_unit = MSR_RAPL_POWER_UNIT,
-	.rapl_msrs      = intel_rapl_msrs,
+	.rapl_pkg_msrs	= intel_rapl_msrs,
 };
 
 static struct rapl_model model_knl = {
-	.events		= BIT(PERF_RAPL_PKG) |
+	.pkg_events	= BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_RAM),
 	.unit_quirk	= RAPL_UNIT_QUIRK_INTEL_HSW,
 	.msr_power_unit = MSR_RAPL_POWER_UNIT,
-	.rapl_msrs      = intel_rapl_msrs,
+	.rapl_pkg_msrs	= intel_rapl_msrs,
 };
 
 static struct rapl_model model_skl = {
-	.events		= BIT(PERF_RAPL_PP0) |
+	.pkg_events	= BIT(PERF_RAPL_PP0) |
 			  BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_RAM) |
 			  BIT(PERF_RAPL_PP1) |
 			  BIT(PERF_RAPL_PSYS),
 	.msr_power_unit = MSR_RAPL_POWER_UNIT,
-	.rapl_msrs      = intel_rapl_msrs,
+	.rapl_pkg_msrs      = intel_rapl_msrs,
 };
 
 static struct rapl_model model_spr = {
-	.events		= BIT(PERF_RAPL_PP0) |
+	.pkg_events	= BIT(PERF_RAPL_PP0) |
 			  BIT(PERF_RAPL_PKG) |
 			  BIT(PERF_RAPL_RAM) |
 			  BIT(PERF_RAPL_PSYS),
 	.unit_quirk	= RAPL_UNIT_QUIRK_INTEL_SPR,
 	.msr_power_unit = MSR_RAPL_POWER_UNIT,
-	.rapl_msrs      = intel_rapl_spr_msrs,
+	.rapl_pkg_msrs	= intel_rapl_spr_msrs,
 };
 
 static struct rapl_model model_amd_hygon = {
-	.events		= BIT(PERF_RAPL_PKG),
+	.pkg_events	= BIT(PERF_RAPL_PKG),
 	.msr_power_unit = MSR_AMD_RAPL_POWER_UNIT,
-	.rapl_msrs      = amd_rapl_msrs,
+	.rapl_pkg_msrs	= amd_rapl_pkg_msrs,
 };
 
 static const struct x86_cpu_id rapl_model_match[] __initconst = {
@@ -793,11 +793,11 @@ MODULE_DEVICE_TABLE(x86cpu, rapl_model_match);
 static int __init rapl_pmu_init(void)
 {
 	const struct x86_cpu_id *id;
-	int rapl_pmu_scope = PERF_PMU_SCOPE_DIE;
+	int rapl_pkg_pmu_scope = PERF_PMU_SCOPE_DIE;
 	int ret;
 
-	if (rapl_pmu_is_pkg_scope())
-		rapl_pmu_scope = PERF_PMU_SCOPE_PKG;
+	if (rapl_pkg_pmu_is_pkg_scope())
+		rapl_pkg_pmu_scope = PERF_PMU_SCOPE_PKG;
 
 	id = x86_match_cpu(rapl_model_match);
 	if (!id)
@@ -805,20 +805,20 @@ static int __init rapl_pmu_init(void)
 
 	rapl_model = (struct rapl_model *) id->driver_data;
 
-	rapl_msrs = rapl_model->rapl_msrs;
+	rapl_msrs = rapl_model->rapl_pkg_msrs;
 
-	rapl_cntr_mask = perf_msr_probe(rapl_msrs, PERF_RAPL_MAX,
-					false, (void *) &rapl_model->events);
+	rapl_pkg_cntr_mask = perf_msr_probe(rapl_msrs, PERF_RAPL_PKG_EVENTS_MAX,
+					false, (void *) &rapl_model->pkg_events);
 
 	ret = rapl_check_hw_unit();
 	if (ret)
 		return ret;
 
-	ret = init_rapl_pmus(&rapl_pmus, rapl_pmu_scope);
+	ret = init_rapl_pmus(&rapl_pmus_pkg, rapl_pkg_pmu_scope);
 	if (ret)
 		return ret;
 
-	ret = perf_pmu_register(&rapl_pmus->pmu, "power", -1);
+	ret = perf_pmu_register(&rapl_pmus_pkg->pmu, "power", -1);
 	if (ret)
 		goto out;
 
@@ -827,14 +827,14 @@ static int __init rapl_pmu_init(void)
 
 out:
 	pr_warn("Initialization failed (%d), disabled\n", ret);
-	cleanup_rapl_pmus(rapl_pmus);
+	cleanup_rapl_pmus(rapl_pmus_pkg);
 	return ret;
 }
 module_init(rapl_pmu_init);
 
 static void __exit intel_rapl_exit(void)
 {
-	perf_pmu_unregister(&rapl_pmus->pmu);
-	cleanup_rapl_pmus(rapl_pmus);
+	perf_pmu_unregister(&rapl_pmus_pkg->pmu);
+	cleanup_rapl_pmus(rapl_pmus_pkg);
 }
 module_exit(intel_rapl_exit);
-- 
2.34.1

Re: [PATCH v6 07/10] perf/x86/rapl: Modify the generic variable names to *_pkg*

[Gautham R. Shenoy]

On Fri, Oct 25, 2024 at 11:13:45AM +0000, Dhananjay Ugwekar wrote:
> Prepare for the addition of RAPL core energy counter support.
> 
> Replace the generic names with *_pkg*, to later on differentiate between 
> the scopes of the two different PMUs and their variables.
> 
> No functional change.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

Looks good to me, even though rapl_pkg_pmu_is_pkg_scope() is a bit
mouthful. But I cannot think of an alternative.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.



> ---
>  arch/x86/events/rapl.c | 118 ++++++++++++++++++++---------------------
>  1 file changed, 59 insertions(+), 59 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index bf6fee114294..ae8b450caa9b 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -70,18 +70,18 @@ MODULE_LICENSE("GPL");
>  /*
>   * RAPL energy status counters
>   */
> -enum perf_rapl_events {
> +enum perf_rapl_pkg_events {
>  	PERF_RAPL_PP0 = 0,		/* all cores */
>  	PERF_RAPL_PKG,			/* entire package */
>  	PERF_RAPL_RAM,			/* DRAM */
>  	PERF_RAPL_PP1,			/* gpu */
>  	PERF_RAPL_PSYS,			/* psys */
>  
> -	PERF_RAPL_MAX,
> -	NR_RAPL_DOMAINS = PERF_RAPL_MAX,
> +	PERF_RAPL_PKG_EVENTS_MAX,
> +	NR_RAPL_PKG_DOMAINS = PERF_RAPL_PKG_EVENTS_MAX,
>  };
>  
> -static const char *const rapl_domain_names[NR_RAPL_DOMAINS] __initconst = {
> +static const char *const rapl_pkg_domain_names[NR_RAPL_PKG_DOMAINS] __initconst = {
>  	"pp0-core",
>  	"package",
>  	"dram",
> @@ -112,7 +112,7 @@ static struct perf_pmu_events_attr event_attr_##v = {				\
>   *	     considered as either pkg-scope or die-scope, and we are considering
>   *	     them as die-scope.
>   */
> -#define rapl_pmu_is_pkg_scope()				\
> +#define rapl_pkg_pmu_is_pkg_scope()				\
>  	(boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||	\
>  	 boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
>  
> @@ -139,16 +139,16 @@ enum rapl_unit_quirk {
>  };
>  
>  struct rapl_model {
> -	struct perf_msr *rapl_msrs;
> -	unsigned long	events;
> +	struct perf_msr *rapl_pkg_msrs;
> +	unsigned long	pkg_events;
>  	unsigned int	msr_power_unit;
>  	enum rapl_unit_quirk	unit_quirk;
>  };
>  
>   /* 1/2^hw_unit Joule */
> -static int rapl_hw_unit[NR_RAPL_DOMAINS] __read_mostly;
> -static struct rapl_pmus *rapl_pmus;
> -static unsigned int rapl_cntr_mask;
> +static int rapl_pkg_hw_unit[NR_RAPL_PKG_DOMAINS] __read_mostly;
> +static struct rapl_pmus *rapl_pmus_pkg;
> +static unsigned int rapl_pkg_cntr_mask;
>  static u64 rapl_timer_ms;
>  static struct perf_msr *rapl_msrs;
>  static struct rapl_model *rapl_model;
> @@ -159,7 +159,7 @@ static struct rapl_model *rapl_model;
>   */
>  static inline unsigned int get_rapl_pmu_idx(int cpu)
>  {
> -	return rapl_pmu_is_pkg_scope() ? topology_logical_package_id(cpu) :
> +	return rapl_pkg_pmu_is_pkg_scope() ? topology_logical_package_id(cpu) :
>  					 topology_logical_die_id(cpu);
>  }
>  
> @@ -172,7 +172,7 @@ static inline u64 rapl_read_counter(struct perf_event *event)
>  
>  static inline u64 rapl_scale(u64 v, int cfg)
>  {
> -	if (cfg > NR_RAPL_DOMAINS) {
> +	if (cfg > NR_RAPL_PKG_DOMAINS) {
>  		pr_warn("Invalid domain %d, failed to scale data\n", cfg);
>  		return v;
>  	}
> @@ -182,7 +182,7 @@ static inline u64 rapl_scale(u64 v, int cfg)
>  	 * or use ldexp(count, -32).
>  	 * Watts = Joules/Time delta
>  	 */
> -	return v << (32 - rapl_hw_unit[cfg - 1]);
> +	return v << (32 - rapl_pkg_hw_unit[cfg - 1]);
>  }
>  
>  static u64 rapl_event_update(struct perf_event *event)
> @@ -342,7 +342,7 @@ static int rapl_pmu_event_init(struct perf_event *event)
>  	struct rapl_pmu *rapl_pmu;
>  
>  	/* only look at RAPL events */
> -	if (event->attr.type != rapl_pmus->pmu.type)
> +	if (event->attr.type != rapl_pmus_pkg->pmu.type)
>  		return -ENOENT;
>  
>  	/* check only supported bits are set */
> @@ -352,14 +352,14 @@ static int rapl_pmu_event_init(struct perf_event *event)
>  	if (event->cpu < 0)
>  		return -EINVAL;
>  
> -	if (!cfg || cfg >= NR_RAPL_DOMAINS + 1)
> +	if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
>  		return -EINVAL;
>  
> -	cfg = array_index_nospec((long)cfg, NR_RAPL_DOMAINS + 1);
> +	cfg = array_index_nospec((long)cfg, NR_RAPL_PKG_DOMAINS + 1);
>  	bit = cfg - 1;
>  
>  	/* check event supported */
> -	if (!(rapl_cntr_mask & (1 << bit)))
> +	if (!(rapl_pkg_cntr_mask & (1 << bit)))
>  		return -EINVAL;
>  
>  	/* unsupported modes and filters */
> @@ -367,11 +367,11 @@ static int rapl_pmu_event_init(struct perf_event *event)
>  		return -EINVAL;
>  
>  	rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
> -	if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
> +	if (rapl_pmu_idx >= rapl_pmus_pkg->nr_rapl_pmu)
>  		return -EINVAL;
>  
>  	/* must be done before validate_group */
> -	rapl_pmu = rapl_pmus->rapl_pmu[rapl_pmu_idx];
> +	rapl_pmu = rapl_pmus_pkg->rapl_pmu[rapl_pmu_idx];
>  	if (!rapl_pmu)
>  		return -EINVAL;
>  
> @@ -525,11 +525,11 @@ static struct perf_msr intel_rapl_spr_msrs[] = {
>  };
>  
>  /*
> - * Force to PERF_RAPL_MAX size due to:
> - * - perf_msr_probe(PERF_RAPL_MAX)
> + * Force to PERF_RAPL_PKG_EVENTS_MAX size due to:
> + * - perf_msr_probe(PERF_RAPL_PKG_EVENTS_MAX)
>   * - want to use same event codes across both architectures
>   */
> -static struct perf_msr amd_rapl_msrs[] = {
> +static struct perf_msr amd_rapl_pkg_msrs[] = {
>  	[PERF_RAPL_PP0]  = { 0, &rapl_events_cores_group, NULL, false, 0 },
>  	[PERF_RAPL_PKG]  = { MSR_AMD_PKG_ENERGY_STATUS,  &rapl_events_pkg_group,   test_msr, false, RAPL_MSR_MASK },
>  	[PERF_RAPL_RAM]  = { 0, &rapl_events_ram_group,   NULL, false, 0 },
> @@ -545,8 +545,8 @@ static int rapl_check_hw_unit(void)
>  	/* protect rdmsrl() to handle virtualization */
>  	if (rdmsrl_safe(rapl_model->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;
> +	for (i = 0; i < NR_RAPL_PKG_DOMAINS; i++)
> +		rapl_pkg_hw_unit[i] = (msr_rapl_power_unit_bits >> 8) & 0x1FULL;
>  
>  	switch (rapl_model->unit_quirk) {
>  	/*
> @@ -556,11 +556,11 @@ static int rapl_check_hw_unit(void)
>  	 * of 2. Datasheet, September 2014, Reference Number: 330784-001 "
>  	 */
>  	case RAPL_UNIT_QUIRK_INTEL_HSW:
> -		rapl_hw_unit[PERF_RAPL_RAM] = 16;
> +		rapl_pkg_hw_unit[PERF_RAPL_RAM] = 16;
>  		break;
>  	/* SPR uses a fixed energy unit for Psys domain. */
>  	case RAPL_UNIT_QUIRK_INTEL_SPR:
> -		rapl_hw_unit[PERF_RAPL_PSYS] = 0;
> +		rapl_pkg_hw_unit[PERF_RAPL_PSYS] = 0;
>  		break;
>  	default:
>  		break;
> @@ -575,9 +575,9 @@ static int rapl_check_hw_unit(void)
>  	 * if hw unit is 32, then we use 2 ms 1/200/2
>  	 */
>  	rapl_timer_ms = 2;
> -	if (rapl_hw_unit[0] < 32) {
> +	if (rapl_pkg_hw_unit[0] < 32) {
>  		rapl_timer_ms = (1000 / (2 * 100));
> -		rapl_timer_ms *= (1ULL << (32 - rapl_hw_unit[0] - 1));
> +		rapl_timer_ms *= (1ULL << (32 - rapl_pkg_hw_unit[0] - 1));
>  	}
>  	return 0;
>  }
> @@ -587,12 +587,12 @@ static void __init rapl_advertise(void)
>  	int i;
>  
>  	pr_info("API unit is 2^-32 Joules, %d fixed counters, %llu ms ovfl timer\n",
> -		hweight32(rapl_cntr_mask), rapl_timer_ms);
> +		hweight32(rapl_pkg_cntr_mask), rapl_timer_ms);
>  
> -	for (i = 0; i < NR_RAPL_DOMAINS; i++) {
> -		if (rapl_cntr_mask & (1 << i)) {
> +	for (i = 0; i < NR_RAPL_PKG_DOMAINS; i++) {
> +		if (rapl_pkg_cntr_mask & (1 << i)) {
>  			pr_info("hw unit of domain %s 2^-%d Joules\n",
> -				rapl_domain_names[i], rapl_hw_unit[i]);
> +				rapl_pkg_domain_names[i], rapl_pkg_hw_unit[i]);
>  		}
>  	}
>  }
> @@ -673,71 +673,71 @@ static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr, int rapl_pmu_
>  }
>  
>  static struct rapl_model model_snb = {
> -	.events		= BIT(PERF_RAPL_PP0) |
> +	.pkg_events	= BIT(PERF_RAPL_PP0) |
>  			  BIT(PERF_RAPL_PKG) |
>  			  BIT(PERF_RAPL_PP1),
>  	.msr_power_unit = MSR_RAPL_POWER_UNIT,
> -	.rapl_msrs      = intel_rapl_msrs,
> +	.rapl_pkg_msrs	= intel_rapl_msrs,
>  };
>  
>  static struct rapl_model model_snbep = {
> -	.events		= BIT(PERF_RAPL_PP0) |
> +	.pkg_events	= BIT(PERF_RAPL_PP0) |
>  			  BIT(PERF_RAPL_PKG) |
>  			  BIT(PERF_RAPL_RAM),
>  	.msr_power_unit = MSR_RAPL_POWER_UNIT,
> -	.rapl_msrs      = intel_rapl_msrs,
> +	.rapl_pkg_msrs	= intel_rapl_msrs,
>  };
>  
>  static struct rapl_model model_hsw = {
> -	.events		= BIT(PERF_RAPL_PP0) |
> +	.pkg_events	= BIT(PERF_RAPL_PP0) |
>  			  BIT(PERF_RAPL_PKG) |
>  			  BIT(PERF_RAPL_RAM) |
>  			  BIT(PERF_RAPL_PP1),
>  	.msr_power_unit = MSR_RAPL_POWER_UNIT,
> -	.rapl_msrs      = intel_rapl_msrs,
> +	.rapl_pkg_msrs	= intel_rapl_msrs,
>  };
>  
>  static struct rapl_model model_hsx = {
> -	.events		= BIT(PERF_RAPL_PP0) |
> +	.pkg_events	= BIT(PERF_RAPL_PP0) |
>  			  BIT(PERF_RAPL_PKG) |
>  			  BIT(PERF_RAPL_RAM),
>  	.unit_quirk	= RAPL_UNIT_QUIRK_INTEL_HSW,
>  	.msr_power_unit = MSR_RAPL_POWER_UNIT,
> -	.rapl_msrs      = intel_rapl_msrs,
> +	.rapl_pkg_msrs	= intel_rapl_msrs,
>  };
>  
>  static struct rapl_model model_knl = {
> -	.events		= BIT(PERF_RAPL_PKG) |
> +	.pkg_events	= BIT(PERF_RAPL_PKG) |
>  			  BIT(PERF_RAPL_RAM),
>  	.unit_quirk	= RAPL_UNIT_QUIRK_INTEL_HSW,
>  	.msr_power_unit = MSR_RAPL_POWER_UNIT,
> -	.rapl_msrs      = intel_rapl_msrs,
> +	.rapl_pkg_msrs	= intel_rapl_msrs,
>  };
>  
>  static struct rapl_model model_skl = {
> -	.events		= BIT(PERF_RAPL_PP0) |
> +	.pkg_events	= BIT(PERF_RAPL_PP0) |
>  			  BIT(PERF_RAPL_PKG) |
>  			  BIT(PERF_RAPL_RAM) |
>  			  BIT(PERF_RAPL_PP1) |
>  			  BIT(PERF_RAPL_PSYS),
>  	.msr_power_unit = MSR_RAPL_POWER_UNIT,
> -	.rapl_msrs      = intel_rapl_msrs,
> +	.rapl_pkg_msrs      = intel_rapl_msrs,
>  };
>  
>  static struct rapl_model model_spr = {
> -	.events		= BIT(PERF_RAPL_PP0) |
> +	.pkg_events	= BIT(PERF_RAPL_PP0) |
>  			  BIT(PERF_RAPL_PKG) |
>  			  BIT(PERF_RAPL_RAM) |
>  			  BIT(PERF_RAPL_PSYS),
>  	.unit_quirk	= RAPL_UNIT_QUIRK_INTEL_SPR,
>  	.msr_power_unit = MSR_RAPL_POWER_UNIT,
> -	.rapl_msrs      = intel_rapl_spr_msrs,
> +	.rapl_pkg_msrs	= intel_rapl_spr_msrs,
>  };
>  
>  static struct rapl_model model_amd_hygon = {
> -	.events		= BIT(PERF_RAPL_PKG),
> +	.pkg_events	= BIT(PERF_RAPL_PKG),
>  	.msr_power_unit = MSR_AMD_RAPL_POWER_UNIT,
> -	.rapl_msrs      = amd_rapl_msrs,
> +	.rapl_pkg_msrs	= amd_rapl_pkg_msrs,
>  };
>  
>  static const struct x86_cpu_id rapl_model_match[] __initconst = {
> @@ -793,11 +793,11 @@ MODULE_DEVICE_TABLE(x86cpu, rapl_model_match);
>  static int __init rapl_pmu_init(void)
>  {
>  	const struct x86_cpu_id *id;
> -	int rapl_pmu_scope = PERF_PMU_SCOPE_DIE;
> +	int rapl_pkg_pmu_scope = PERF_PMU_SCOPE_DIE;
>  	int ret;
>  
> -	if (rapl_pmu_is_pkg_scope())
> -		rapl_pmu_scope = PERF_PMU_SCOPE_PKG;
> +	if (rapl_pkg_pmu_is_pkg_scope())
> +		rapl_pkg_pmu_scope = PERF_PMU_SCOPE_PKG;
>  
>  	id = x86_match_cpu(rapl_model_match);
>  	if (!id)
> @@ -805,20 +805,20 @@ static int __init rapl_pmu_init(void)
>  
>  	rapl_model = (struct rapl_model *) id->driver_data;
>  
> -	rapl_msrs = rapl_model->rapl_msrs;
> +	rapl_msrs = rapl_model->rapl_pkg_msrs;
>  
> -	rapl_cntr_mask = perf_msr_probe(rapl_msrs, PERF_RAPL_MAX,
> -					false, (void *) &rapl_model->events);
> +	rapl_pkg_cntr_mask = perf_msr_probe(rapl_msrs, PERF_RAPL_PKG_EVENTS_MAX,
> +					false, (void *) &rapl_model->pkg_events);
>  
>  	ret = rapl_check_hw_unit();
>  	if (ret)
>  		return ret;
>  
> -	ret = init_rapl_pmus(&rapl_pmus, rapl_pmu_scope);
> +	ret = init_rapl_pmus(&rapl_pmus_pkg, rapl_pkg_pmu_scope);
>  	if (ret)
>  		return ret;
>  
> -	ret = perf_pmu_register(&rapl_pmus->pmu, "power", -1);
> +	ret = perf_pmu_register(&rapl_pmus_pkg->pmu, "power", -1);
>  	if (ret)
>  		goto out;
>  
> @@ -827,14 +827,14 @@ static int __init rapl_pmu_init(void)
>  
>  out:
>  	pr_warn("Initialization failed (%d), disabled\n", ret);
> -	cleanup_rapl_pmus(rapl_pmus);
> +	cleanup_rapl_pmus(rapl_pmus_pkg);
>  	return ret;
>  }
>  module_init(rapl_pmu_init);
>  
>  static void __exit intel_rapl_exit(void)
>  {
> -	perf_pmu_unregister(&rapl_pmus->pmu);
> -	cleanup_rapl_pmus(rapl_pmus);
> +	perf_pmu_unregister(&rapl_pmus_pkg->pmu);
> +	cleanup_rapl_pmus(rapl_pmus_pkg);
>  }
>  module_exit(intel_rapl_exit);
> -- 
> 2.34.1
> 

Re: [PATCH v6 07/10] perf/x86/rapl: Modify the generic variable names to *_pkg*

[Zhang, Rui]

On Fri, 2024-10-25 at 11:13 +0000, Dhananjay Ugwekar wrote:
> Prepare for the addition of RAPL core energy counter support.
> 
> Replace the generic names with *_pkg*, to later on differentiate
> between 
> the scopes of the two different PMUs and their variables.
> 
> No functional change.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

Reviewed-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui

> ---
>  arch/x86/events/rapl.c | 118 ++++++++++++++++++++-------------------
> --
>  1 file changed, 59 insertions(+), 59 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index bf6fee114294..ae8b450caa9b 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -70,18 +70,18 @@ MODULE_LICENSE("GPL");
>  /*
>   * RAPL energy status counters
>   */
> -enum perf_rapl_events {
> +enum perf_rapl_pkg_events {
>         PERF_RAPL_PP0 = 0,              /* all cores */
>         PERF_RAPL_PKG,                  /* entire package */
>         PERF_RAPL_RAM,                  /* DRAM */
>         PERF_RAPL_PP1,                  /* gpu */
>         PERF_RAPL_PSYS,                 /* psys */
>  
> -       PERF_RAPL_MAX,
> -       NR_RAPL_DOMAINS = PERF_RAPL_MAX,
> +       PERF_RAPL_PKG_EVENTS_MAX,
> +       NR_RAPL_PKG_DOMAINS = PERF_RAPL_PKG_EVENTS_MAX,
>  };
>  
> -static const char *const rapl_domain_names[NR_RAPL_DOMAINS]
> __initconst = {
> +static const char *const rapl_pkg_domain_names[NR_RAPL_PKG_DOMAINS]
> __initconst = {
>         "pp0-core",
>         "package",
>         "dram",
> @@ -112,7 +112,7 @@ static struct perf_pmu_events_attr event_attr_##v
> = {                               \
>   *          considered as either pkg-scope or die-scope, and we are
> considering
>   *          them as die-scope.
>   */
> -#define rapl_pmu_is_pkg_scope()                                \
> +#define rapl_pkg_pmu_is_pkg_scope()                            \
>         (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||  \
>          boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
>  
> @@ -139,16 +139,16 @@ enum rapl_unit_quirk {
>  };
>  
>  struct rapl_model {
> -       struct perf_msr *rapl_msrs;
> -       unsigned long   events;
> +       struct perf_msr *rapl_pkg_msrs;
> +       unsigned long   pkg_events;
>         unsigned int    msr_power_unit;
>         enum rapl_unit_quirk    unit_quirk;
>  };
>  
>   /* 1/2^hw_unit Joule */
> -static int rapl_hw_unit[NR_RAPL_DOMAINS] __read_mostly;
> -static struct rapl_pmus *rapl_pmus;
> -static unsigned int rapl_cntr_mask;
> +static int rapl_pkg_hw_unit[NR_RAPL_PKG_DOMAINS] __read_mostly;
> +static struct rapl_pmus *rapl_pmus_pkg;
> +static unsigned int rapl_pkg_cntr_mask;
>  static u64 rapl_timer_ms;
>  static struct perf_msr *rapl_msrs;
>  static struct rapl_model *rapl_model;
> @@ -159,7 +159,7 @@ static struct rapl_model *rapl_model;
>   */
>  static inline unsigned int get_rapl_pmu_idx(int cpu)
>  {
> -       return rapl_pmu_is_pkg_scope() ?
> topology_logical_package_id(cpu) :
> +       return rapl_pkg_pmu_is_pkg_scope() ?
> topology_logical_package_id(cpu) :
>                                         
> topology_logical_die_id(cpu);
>  }
>  
> @@ -172,7 +172,7 @@ static inline u64 rapl_read_counter(struct
> perf_event *event)
>  
>  static inline u64 rapl_scale(u64 v, int cfg)
>  {
> -       if (cfg > NR_RAPL_DOMAINS) {
> +       if (cfg > NR_RAPL_PKG_DOMAINS) {
>                 pr_warn("Invalid domain %d, failed to scale data\n",
> cfg);
>                 return v;
>         }
> @@ -182,7 +182,7 @@ static inline u64 rapl_scale(u64 v, int cfg)
>          * or use ldexp(count, -32).
>          * Watts = Joules/Time delta
>          */
> -       return v << (32 - rapl_hw_unit[cfg - 1]);
> +       return v << (32 - rapl_pkg_hw_unit[cfg - 1]);
>  }
>  
>  static u64 rapl_event_update(struct perf_event *event)
> @@ -342,7 +342,7 @@ static int rapl_pmu_event_init(struct perf_event
> *event)
>         struct rapl_pmu *rapl_pmu;
>  
>         /* only look at RAPL events */
> -       if (event->attr.type != rapl_pmus->pmu.type)
> +       if (event->attr.type != rapl_pmus_pkg->pmu.type)
>                 return -ENOENT;
>  
>         /* check only supported bits are set */
> @@ -352,14 +352,14 @@ static int rapl_pmu_event_init(struct
> perf_event *event)
>         if (event->cpu < 0)
>                 return -EINVAL;
>  
> -       if (!cfg || cfg >= NR_RAPL_DOMAINS + 1)
> +       if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
>                 return -EINVAL;
>  
> -       cfg = array_index_nospec((long)cfg, NR_RAPL_DOMAINS + 1);
> +       cfg = array_index_nospec((long)cfg, NR_RAPL_PKG_DOMAINS + 1);
>         bit = cfg - 1;
>  
>         /* check event supported */
> -       if (!(rapl_cntr_mask & (1 << bit)))
> +       if (!(rapl_pkg_cntr_mask & (1 << bit)))
>                 return -EINVAL;
>  
>         /* unsupported modes and filters */
> @@ -367,11 +367,11 @@ static int rapl_pmu_event_init(struct
> perf_event *event)
>                 return -EINVAL;
>  
>         rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
> -       if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
> +       if (rapl_pmu_idx >= rapl_pmus_pkg->nr_rapl_pmu)
>                 return -EINVAL;
>  
>         /* must be done before validate_group */
> -       rapl_pmu = rapl_pmus->rapl_pmu[rapl_pmu_idx];
> +       rapl_pmu = rapl_pmus_pkg->rapl_pmu[rapl_pmu_idx];
>         if (!rapl_pmu)
>                 return -EINVAL;
>  
> @@ -525,11 +525,11 @@ static struct perf_msr intel_rapl_spr_msrs[] =
> {
>  };
>  
>  /*
> - * Force to PERF_RAPL_MAX size due to:
> - * - perf_msr_probe(PERF_RAPL_MAX)
> + * Force to PERF_RAPL_PKG_EVENTS_MAX size due to:
> + * - perf_msr_probe(PERF_RAPL_PKG_EVENTS_MAX)
>   * - want to use same event codes across both architectures
>   */
> -static struct perf_msr amd_rapl_msrs[] = {
> +static struct perf_msr amd_rapl_pkg_msrs[] = {
>         [PERF_RAPL_PP0]  = { 0, &rapl_events_cores_group, NULL,
> false, 0 },
>         [PERF_RAPL_PKG]  = { MSR_AMD_PKG_ENERGY_STATUS, 
> &rapl_events_pkg_group,   test_msr, false, RAPL_MSR_MASK },
>         [PERF_RAPL_RAM]  = { 0, &rapl_events_ram_group,   NULL,
> false, 0 },
> @@ -545,8 +545,8 @@ static int rapl_check_hw_unit(void)
>         /* protect rdmsrl() to handle virtualization */
>         if (rdmsrl_safe(rapl_model->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;
> +       for (i = 0; i < NR_RAPL_PKG_DOMAINS; i++)
> +               rapl_pkg_hw_unit[i] = (msr_rapl_power_unit_bits >> 8)
> & 0x1FULL;
>  
>         switch (rapl_model->unit_quirk) {
>         /*
> @@ -556,11 +556,11 @@ static int rapl_check_hw_unit(void)
>          * of 2. Datasheet, September 2014, Reference Number: 330784-
> 001 "
>          */
>         case RAPL_UNIT_QUIRK_INTEL_HSW:
> -               rapl_hw_unit[PERF_RAPL_RAM] = 16;
> +               rapl_pkg_hw_unit[PERF_RAPL_RAM] = 16;
>                 break;
>         /* SPR uses a fixed energy unit for Psys domain. */
>         case RAPL_UNIT_QUIRK_INTEL_SPR:
> -               rapl_hw_unit[PERF_RAPL_PSYS] = 0;
> +               rapl_pkg_hw_unit[PERF_RAPL_PSYS] = 0;
>                 break;
>         default:
>                 break;
> @@ -575,9 +575,9 @@ static int rapl_check_hw_unit(void)
>          * if hw unit is 32, then we use 2 ms 1/200/2
>          */
>         rapl_timer_ms = 2;
> -       if (rapl_hw_unit[0] < 32) {
> +       if (rapl_pkg_hw_unit[0] < 32) {
>                 rapl_timer_ms = (1000 / (2 * 100));
> -               rapl_timer_ms *= (1ULL << (32 - rapl_hw_unit[0] -
> 1));
> +               rapl_timer_ms *= (1ULL << (32 - rapl_pkg_hw_unit[0] -
> 1));
>         }
>         return 0;
>  }
> @@ -587,12 +587,12 @@ static void __init rapl_advertise(void)
>         int i;
>  
>         pr_info("API unit is 2^-32 Joules, %d fixed counters, %llu ms
> ovfl timer\n",
> -               hweight32(rapl_cntr_mask), rapl_timer_ms);
> +               hweight32(rapl_pkg_cntr_mask), rapl_timer_ms);
>  
> -       for (i = 0; i < NR_RAPL_DOMAINS; i++) {
> -               if (rapl_cntr_mask & (1 << i)) {
> +       for (i = 0; i < NR_RAPL_PKG_DOMAINS; i++) {
> +               if (rapl_pkg_cntr_mask & (1 << i)) {
>                         pr_info("hw unit of domain %s 2^-%d
> Joules\n",
> -                               rapl_domain_names[i],
> rapl_hw_unit[i]);
> +                               rapl_pkg_domain_names[i],
> rapl_pkg_hw_unit[i]);
>                 }
>         }
>  }
> @@ -673,71 +673,71 @@ static int __init init_rapl_pmus(struct
> rapl_pmus **rapl_pmus_ptr, int rapl_pmu_
>  }
>  
>  static struct rapl_model model_snb = {
> -       .events         = BIT(PERF_RAPL_PP0) |
> +       .pkg_events     = BIT(PERF_RAPL_PP0) |
>                           BIT(PERF_RAPL_PKG) |
>                           BIT(PERF_RAPL_PP1),
>         .msr_power_unit = MSR_RAPL_POWER_UNIT,
> -       .rapl_msrs      = intel_rapl_msrs,
> +       .rapl_pkg_msrs  = intel_rapl_msrs,
>  };
>  
>  static struct rapl_model model_snbep = {
> -       .events         = BIT(PERF_RAPL_PP0) |
> +       .pkg_events     = BIT(PERF_RAPL_PP0) |
>                           BIT(PERF_RAPL_PKG) |
>                           BIT(PERF_RAPL_RAM),
>         .msr_power_unit = MSR_RAPL_POWER_UNIT,
> -       .rapl_msrs      = intel_rapl_msrs,
> +       .rapl_pkg_msrs  = intel_rapl_msrs,
>  };
>  
>  static struct rapl_model model_hsw = {
> -       .events         = BIT(PERF_RAPL_PP0) |
> +       .pkg_events     = BIT(PERF_RAPL_PP0) |
>                           BIT(PERF_RAPL_PKG) |
>                           BIT(PERF_RAPL_RAM) |
>                           BIT(PERF_RAPL_PP1),
>         .msr_power_unit = MSR_RAPL_POWER_UNIT,
> -       .rapl_msrs      = intel_rapl_msrs,
> +       .rapl_pkg_msrs  = intel_rapl_msrs,
>  };
>  
>  static struct rapl_model model_hsx = {
> -       .events         = BIT(PERF_RAPL_PP0) |
> +       .pkg_events     = BIT(PERF_RAPL_PP0) |
>                           BIT(PERF_RAPL_PKG) |
>                           BIT(PERF_RAPL_RAM),
>         .unit_quirk     = RAPL_UNIT_QUIRK_INTEL_HSW,
>         .msr_power_unit = MSR_RAPL_POWER_UNIT,
> -       .rapl_msrs      = intel_rapl_msrs,
> +       .rapl_pkg_msrs  = intel_rapl_msrs,
>  };
>  
>  static struct rapl_model model_knl = {
> -       .events         = BIT(PERF_RAPL_PKG) |
> +       .pkg_events     = BIT(PERF_RAPL_PKG) |
>                           BIT(PERF_RAPL_RAM),
>         .unit_quirk     = RAPL_UNIT_QUIRK_INTEL_HSW,
>         .msr_power_unit = MSR_RAPL_POWER_UNIT,
> -       .rapl_msrs      = intel_rapl_msrs,
> +       .rapl_pkg_msrs  = intel_rapl_msrs,
>  };
>  
>  static struct rapl_model model_skl = {
> -       .events         = BIT(PERF_RAPL_PP0) |
> +       .pkg_events     = BIT(PERF_RAPL_PP0) |
>                           BIT(PERF_RAPL_PKG) |
>                           BIT(PERF_RAPL_RAM) |
>                           BIT(PERF_RAPL_PP1) |
>                           BIT(PERF_RAPL_PSYS),
>         .msr_power_unit = MSR_RAPL_POWER_UNIT,
> -       .rapl_msrs      = intel_rapl_msrs,
> +       .rapl_pkg_msrs      = intel_rapl_msrs,
>  };
>  
>  static struct rapl_model model_spr = {
> -       .events         = BIT(PERF_RAPL_PP0) |
> +       .pkg_events     = BIT(PERF_RAPL_PP0) |
>                           BIT(PERF_RAPL_PKG) |
>                           BIT(PERF_RAPL_RAM) |
>                           BIT(PERF_RAPL_PSYS),
>         .unit_quirk     = RAPL_UNIT_QUIRK_INTEL_SPR,
>         .msr_power_unit = MSR_RAPL_POWER_UNIT,
> -       .rapl_msrs      = intel_rapl_spr_msrs,
> +       .rapl_pkg_msrs  = intel_rapl_spr_msrs,
>  };
>  
>  static struct rapl_model model_amd_hygon = {
> -       .events         = BIT(PERF_RAPL_PKG),
> +       .pkg_events     = BIT(PERF_RAPL_PKG),
>         .msr_power_unit = MSR_AMD_RAPL_POWER_UNIT,
> -       .rapl_msrs      = amd_rapl_msrs,
> +       .rapl_pkg_msrs  = amd_rapl_pkg_msrs,
>  };
>  
>  static const struct x86_cpu_id rapl_model_match[] __initconst = {
> @@ -793,11 +793,11 @@ MODULE_DEVICE_TABLE(x86cpu, rapl_model_match);
>  static int __init rapl_pmu_init(void)
>  {
>         const struct x86_cpu_id *id;
> -       int rapl_pmu_scope = PERF_PMU_SCOPE_DIE;
> +       int rapl_pkg_pmu_scope = PERF_PMU_SCOPE_DIE;
>         int ret;
>  
> -       if (rapl_pmu_is_pkg_scope())
> -               rapl_pmu_scope = PERF_PMU_SCOPE_PKG;
> +       if (rapl_pkg_pmu_is_pkg_scope())
> +               rapl_pkg_pmu_scope = PERF_PMU_SCOPE_PKG;
>  
>         id = x86_match_cpu(rapl_model_match);
>         if (!id)
> @@ -805,20 +805,20 @@ static int __init rapl_pmu_init(void)
>  
>         rapl_model = (struct rapl_model *) id->driver_data;
>  
> -       rapl_msrs = rapl_model->rapl_msrs;
> +       rapl_msrs = rapl_model->rapl_pkg_msrs;
>  
> -       rapl_cntr_mask = perf_msr_probe(rapl_msrs, PERF_RAPL_MAX,
> -                                       false, (void *) &rapl_model-
> >events);
> +       rapl_pkg_cntr_mask = perf_msr_probe(rapl_msrs,
> PERF_RAPL_PKG_EVENTS_MAX,
> +                                       false, (void *) &rapl_model-
> >pkg_events);
>  
>         ret = rapl_check_hw_unit();
>         if (ret)
>                 return ret;
>  
> -       ret = init_rapl_pmus(&rapl_pmus, rapl_pmu_scope);
> +       ret = init_rapl_pmus(&rapl_pmus_pkg, rapl_pkg_pmu_scope);
>         if (ret)
>                 return ret;
>  
> -       ret = perf_pmu_register(&rapl_pmus->pmu, "power", -1);
> +       ret = perf_pmu_register(&rapl_pmus_pkg->pmu, "power", -1);
>         if (ret)
>                 goto out;
>  
> @@ -827,14 +827,14 @@ static int __init rapl_pmu_init(void)
>  
>  out:
>         pr_warn("Initialization failed (%d), disabled\n", ret);
> -       cleanup_rapl_pmus(rapl_pmus);
> +       cleanup_rapl_pmus(rapl_pmus_pkg);
>         return ret;
>  }
>  module_init(rapl_pmu_init);
>  
>  static void __exit intel_rapl_exit(void)
>  {
> -       perf_pmu_unregister(&rapl_pmus->pmu);
> -       cleanup_rapl_pmus(rapl_pmus);
> +       perf_pmu_unregister(&rapl_pmus_pkg->pmu);
> +       cleanup_rapl_pmus(rapl_pmus_pkg);
>  }
>  module_exit(intel_rapl_exit);

[PATCH v6 08/10] perf/x86/rapl: Remove the global variable rapl_msrs

[Dhananjay Ugwekar]

Prepare for the addition of RAPL core energy counter support.

After making the rapl_model struct global, the rapl_msrs global
variable isn't needed, so remove it.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 arch/x86/events/rapl.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index ae8b450caa9b..e80b62cf9abc 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -150,7 +150,6 @@ static int rapl_pkg_hw_unit[NR_RAPL_PKG_DOMAINS] __read_mostly;
 static struct rapl_pmus *rapl_pmus_pkg;
 static unsigned int rapl_pkg_cntr_mask;
 static u64 rapl_timer_ms;
-static struct perf_msr *rapl_msrs;
 static struct rapl_model *rapl_model;
 
 /*
@@ -376,7 +375,7 @@ static int rapl_pmu_event_init(struct perf_event *event)
 		return -EINVAL;
 
 	event->pmu_private = rapl_pmu;
-	event->hw.event_base = rapl_msrs[bit].msr;
+	event->hw.event_base = rapl_model->rapl_pkg_msrs[bit].msr;
 	event->hw.config = cfg;
 	event->hw.idx = bit;
 
@@ -805,9 +804,7 @@ static int __init rapl_pmu_init(void)
 
 	rapl_model = (struct rapl_model *) id->driver_data;
 
-	rapl_msrs = rapl_model->rapl_pkg_msrs;
-
-	rapl_pkg_cntr_mask = perf_msr_probe(rapl_msrs, PERF_RAPL_PKG_EVENTS_MAX,
+	rapl_pkg_cntr_mask = perf_msr_probe(rapl_model->rapl_pkg_msrs, PERF_RAPL_PKG_EVENTS_MAX,
 					false, (void *) &rapl_model->pkg_events);
 
 	ret = rapl_check_hw_unit();
-- 
2.34.1

Re: [PATCH v6 08/10] perf/x86/rapl: Remove the global variable rapl_msrs

[Gautham R. Shenoy]

On Fri, Oct 25, 2024 at 11:13:46AM +0000, Dhananjay Ugwekar wrote:
> Prepare for the addition of RAPL core energy counter support.
> 
> After making the rapl_model struct global, the rapl_msrs global
> variable isn't needed, so remove it.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

> ---
>  arch/x86/events/rapl.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index ae8b450caa9b..e80b62cf9abc 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -150,7 +150,6 @@ static int rapl_pkg_hw_unit[NR_RAPL_PKG_DOMAINS] __read_mostly;
>  static struct rapl_pmus *rapl_pmus_pkg;
>  static unsigned int rapl_pkg_cntr_mask;
>  static u64 rapl_timer_ms;
> -static struct perf_msr *rapl_msrs;
>  static struct rapl_model *rapl_model;
>  
>  /*
> @@ -376,7 +375,7 @@ static int rapl_pmu_event_init(struct perf_event *event)
>  		return -EINVAL;
>  
>  	event->pmu_private = rapl_pmu;
> -	event->hw.event_base = rapl_msrs[bit].msr;
> +	event->hw.event_base = rapl_model->rapl_pkg_msrs[bit].msr;
>  	event->hw.config = cfg;
>  	event->hw.idx = bit;
>  
> @@ -805,9 +804,7 @@ static int __init rapl_pmu_init(void)
>  
>  	rapl_model = (struct rapl_model *) id->driver_data;
>  
> -	rapl_msrs = rapl_model->rapl_pkg_msrs;
> -
> -	rapl_pkg_cntr_mask = perf_msr_probe(rapl_msrs, PERF_RAPL_PKG_EVENTS_MAX,
> +	rapl_pkg_cntr_mask = perf_msr_probe(rapl_model->rapl_pkg_msrs, PERF_RAPL_PKG_EVENTS_MAX,
>  					false, (void *) &rapl_model->pkg_events);
>  
>  	ret = rapl_check_hw_unit();
> -- 
> 2.34.1
> 

Re: [PATCH v6 08/10] perf/x86/rapl: Remove the global variable rapl_msrs

[Zhang, Rui]

On Fri, 2024-10-25 at 11:13 +0000, Dhananjay Ugwekar wrote:
> Prepare for the addition of RAPL core energy counter support.
> 
> After making the rapl_model struct global, the rapl_msrs global
> variable isn't needed, so remove it.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

Reviewed-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui

> ---
>  arch/x86/events/rapl.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index ae8b450caa9b..e80b62cf9abc 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -150,7 +150,6 @@ static int rapl_pkg_hw_unit[NR_RAPL_PKG_DOMAINS]
> __read_mostly;
>  static struct rapl_pmus *rapl_pmus_pkg;
>  static unsigned int rapl_pkg_cntr_mask;
>  static u64 rapl_timer_ms;
> -static struct perf_msr *rapl_msrs;
>  static struct rapl_model *rapl_model;
>  
>  /*
> @@ -376,7 +375,7 @@ static int rapl_pmu_event_init(struct perf_event
> *event)
>                 return -EINVAL;
>  
>         event->pmu_private = rapl_pmu;
> -       event->hw.event_base = rapl_msrs[bit].msr;
> +       event->hw.event_base = rapl_model->rapl_pkg_msrs[bit].msr;
>         event->hw.config = cfg;
>         event->hw.idx = bit;
>  
> @@ -805,9 +804,7 @@ static int __init rapl_pmu_init(void)
>  
>         rapl_model = (struct rapl_model *) id->driver_data;
>  
> -       rapl_msrs = rapl_model->rapl_pkg_msrs;
> -
> -       rapl_pkg_cntr_mask = perf_msr_probe(rapl_msrs,
> PERF_RAPL_PKG_EVENTS_MAX,
> +       rapl_pkg_cntr_mask = perf_msr_probe(rapl_model-
> >rapl_pkg_msrs, PERF_RAPL_PKG_EVENTS_MAX,
>                                         false, (void *) &rapl_model-
> >pkg_events);
>  
>         ret = rapl_check_hw_unit();

[PATCH v6 09/10] perf/x86/rapl: Move the cntr_mask to rapl_pmus struct

[Dhananjay Ugwekar]

Prepare for the addition of RAPL core energy counter support.

Move cntr_mask to rapl_pmus struct instead of adding a new global
cntr_mask for the new RAPL power_core PMU. This will also ensure that
the second "core_cntr_mask" is only created if needed (i.e. in case of
AMD CPUs).

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 arch/x86/events/rapl.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index e80b62cf9abc..d3acc70a3d31 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -129,6 +129,7 @@ struct rapl_pmu {
 struct rapl_pmus {
 	struct pmu		pmu;
 	unsigned int		nr_rapl_pmu;
+	unsigned int		cntr_mask;
 	struct rapl_pmu		*rapl_pmu[] __counted_by(nr_rapl_pmu);
 };
 
@@ -148,7 +149,6 @@ struct rapl_model {
  /* 1/2^hw_unit Joule */
 static int rapl_pkg_hw_unit[NR_RAPL_PKG_DOMAINS] __read_mostly;
 static struct rapl_pmus *rapl_pmus_pkg;
-static unsigned int rapl_pkg_cntr_mask;
 static u64 rapl_timer_ms;
 static struct rapl_model *rapl_model;
 
@@ -358,7 +358,7 @@ static int rapl_pmu_event_init(struct perf_event *event)
 	bit = cfg - 1;
 
 	/* check event supported */
-	if (!(rapl_pkg_cntr_mask & (1 << bit)))
+	if (!(rapl_pmus_pkg->cntr_mask & (1 << bit)))
 		return -EINVAL;
 
 	/* unsupported modes and filters */
@@ -586,10 +586,10 @@ static void __init rapl_advertise(void)
 	int i;
 
 	pr_info("API unit is 2^-32 Joules, %d fixed counters, %llu ms ovfl timer\n",
-		hweight32(rapl_pkg_cntr_mask), rapl_timer_ms);
+		hweight32(rapl_pmus_pkg->cntr_mask), rapl_timer_ms);
 
 	for (i = 0; i < NR_RAPL_PKG_DOMAINS; i++) {
-		if (rapl_pkg_cntr_mask & (1 << i)) {
+		if (rapl_pmus_pkg->cntr_mask & (1 << i)) {
 			pr_info("hw unit of domain %s 2^-%d Joules\n",
 				rapl_pkg_domain_names[i], rapl_pkg_hw_unit[i]);
 		}
@@ -804,9 +804,6 @@ static int __init rapl_pmu_init(void)
 
 	rapl_model = (struct rapl_model *) id->driver_data;
 
-	rapl_pkg_cntr_mask = perf_msr_probe(rapl_model->rapl_pkg_msrs, PERF_RAPL_PKG_EVENTS_MAX,
-					false, (void *) &rapl_model->pkg_events);
-
 	ret = rapl_check_hw_unit();
 	if (ret)
 		return ret;
@@ -815,6 +812,10 @@ static int __init rapl_pmu_init(void)
 	if (ret)
 		return ret;
 
+	rapl_pmus_pkg->cntr_mask = perf_msr_probe(rapl_model->rapl_pkg_msrs,
+						  PERF_RAPL_PKG_EVENTS_MAX, false,
+						  (void *) &rapl_model->pkg_events);
+
 	ret = perf_pmu_register(&rapl_pmus_pkg->pmu, "power", -1);
 	if (ret)
 		goto out;
-- 
2.34.1

Re: [PATCH v6 09/10] perf/x86/rapl: Move the cntr_mask to rapl_pmus struct

[Gautham R. Shenoy]

On Fri, Oct 25, 2024 at 11:13:47AM +0000, Dhananjay Ugwekar wrote:
> Prepare for the addition of RAPL core energy counter support.
> 
> Move cntr_mask to rapl_pmus struct instead of adding a new global
> cntr_mask for the new RAPL power_core PMU. This will also ensure that
> the second "core_cntr_mask" is only created if needed (i.e. in case of
> AMD CPUs).
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

LGTM.
Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
--
Thanks and Regards
gautham.

> ---
>  arch/x86/events/rapl.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index e80b62cf9abc..d3acc70a3d31 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -129,6 +129,7 @@ struct rapl_pmu {
>  struct rapl_pmus {
>  	struct pmu		pmu;
>  	unsigned int		nr_rapl_pmu;
> +	unsigned int		cntr_mask;
>  	struct rapl_pmu		*rapl_pmu[] __counted_by(nr_rapl_pmu);
>  };
>  
> @@ -148,7 +149,6 @@ struct rapl_model {
>   /* 1/2^hw_unit Joule */
>  static int rapl_pkg_hw_unit[NR_RAPL_PKG_DOMAINS] __read_mostly;
>  static struct rapl_pmus *rapl_pmus_pkg;
> -static unsigned int rapl_pkg_cntr_mask;
>  static u64 rapl_timer_ms;
>  static struct rapl_model *rapl_model;
>  
> @@ -358,7 +358,7 @@ static int rapl_pmu_event_init(struct perf_event *event)
>  	bit = cfg - 1;
>  
>  	/* check event supported */
> -	if (!(rapl_pkg_cntr_mask & (1 << bit)))
> +	if (!(rapl_pmus_pkg->cntr_mask & (1 << bit)))
>  		return -EINVAL;
>  
>  	/* unsupported modes and filters */
> @@ -586,10 +586,10 @@ static void __init rapl_advertise(void)
>  	int i;
>  
>  	pr_info("API unit is 2^-32 Joules, %d fixed counters, %llu ms ovfl timer\n",
> -		hweight32(rapl_pkg_cntr_mask), rapl_timer_ms);
> +		hweight32(rapl_pmus_pkg->cntr_mask), rapl_timer_ms);
>  
>  	for (i = 0; i < NR_RAPL_PKG_DOMAINS; i++) {
> -		if (rapl_pkg_cntr_mask & (1 << i)) {
> +		if (rapl_pmus_pkg->cntr_mask & (1 << i)) {
>  			pr_info("hw unit of domain %s 2^-%d Joules\n",
>  				rapl_pkg_domain_names[i], rapl_pkg_hw_unit[i]);
>  		}
> @@ -804,9 +804,6 @@ static int __init rapl_pmu_init(void)
>  
>  	rapl_model = (struct rapl_model *) id->driver_data;
>  
> -	rapl_pkg_cntr_mask = perf_msr_probe(rapl_model->rapl_pkg_msrs, PERF_RAPL_PKG_EVENTS_MAX,
> -					false, (void *) &rapl_model->pkg_events);
> -
>  	ret = rapl_check_hw_unit();
>  	if (ret)
>  		return ret;
> @@ -815,6 +812,10 @@ static int __init rapl_pmu_init(void)
>  	if (ret)
>  		return ret;
>  
> +	rapl_pmus_pkg->cntr_mask = perf_msr_probe(rapl_model->rapl_pkg_msrs,
> +						  PERF_RAPL_PKG_EVENTS_MAX, false,
> +						  (void *) &rapl_model->pkg_events);
> +
>  	ret = perf_pmu_register(&rapl_pmus_pkg->pmu, "power", -1);
>  	if (ret)
>  		goto out;
> -- 
> 2.34.1
> 

Re: [PATCH v6 09/10] perf/x86/rapl: Move the cntr_mask to rapl_pmus struct

[Zhang, Rui]

On Fri, 2024-10-25 at 11:13 +0000, Dhananjay Ugwekar wrote:
> Prepare for the addition of RAPL core energy counter support.
> 
> Move cntr_mask to rapl_pmus struct instead of adding a new global
> cntr_mask for the new RAPL power_core PMU. This will also ensure that
> the second "core_cntr_mask" is only created if needed (i.e. in case
> of
> AMD CPUs).
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

Reviewed-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui

> ---
>  arch/x86/events/rapl.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index e80b62cf9abc..d3acc70a3d31 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -129,6 +129,7 @@ struct rapl_pmu {
>  struct rapl_pmus {
>         struct pmu              pmu;
>         unsigned int            nr_rapl_pmu;
> +       unsigned int            cntr_mask;
>         struct rapl_pmu         *rapl_pmu[]
> __counted_by(nr_rapl_pmu);
>  };
>  
> @@ -148,7 +149,6 @@ struct rapl_model {
>   /* 1/2^hw_unit Joule */
>  static int rapl_pkg_hw_unit[NR_RAPL_PKG_DOMAINS] __read_mostly;
>  static struct rapl_pmus *rapl_pmus_pkg;
> -static unsigned int rapl_pkg_cntr_mask;
>  static u64 rapl_timer_ms;
>  static struct rapl_model *rapl_model;
>  
> @@ -358,7 +358,7 @@ static int rapl_pmu_event_init(struct perf_event
> *event)
>         bit = cfg - 1;
>  
>         /* check event supported */
> -       if (!(rapl_pkg_cntr_mask & (1 << bit)))
> +       if (!(rapl_pmus_pkg->cntr_mask & (1 << bit)))
>                 return -EINVAL;
>  
>         /* unsupported modes and filters */
> @@ -586,10 +586,10 @@ static void __init rapl_advertise(void)
>         int i;
>  
>         pr_info("API unit is 2^-32 Joules, %d fixed counters, %llu ms
> ovfl timer\n",
> -               hweight32(rapl_pkg_cntr_mask), rapl_timer_ms);
> +               hweight32(rapl_pmus_pkg->cntr_mask), rapl_timer_ms);
>  
>         for (i = 0; i < NR_RAPL_PKG_DOMAINS; i++) {
> -               if (rapl_pkg_cntr_mask & (1 << i)) {
> +               if (rapl_pmus_pkg->cntr_mask & (1 << i)) {
>                         pr_info("hw unit of domain %s 2^-%d
> Joules\n",
>                                 rapl_pkg_domain_names[i],
> rapl_pkg_hw_unit[i]);
>                 }
> @@ -804,9 +804,6 @@ static int __init rapl_pmu_init(void)
>  
>         rapl_model = (struct rapl_model *) id->driver_data;
>  
> -       rapl_pkg_cntr_mask = perf_msr_probe(rapl_model-
> >rapl_pkg_msrs, PERF_RAPL_PKG_EVENTS_MAX,
> -                                       false, (void *) &rapl_model-
> >pkg_events);
> -
>         ret = rapl_check_hw_unit();
>         if (ret)
>                 return ret;
> @@ -815,6 +812,10 @@ static int __init rapl_pmu_init(void)
>         if (ret)
>                 return ret;
>  
> +       rapl_pmus_pkg->cntr_mask = perf_msr_probe(rapl_model-
> >rapl_pkg_msrs,
> +                                                
> PERF_RAPL_PKG_EVENTS_MAX, false,
> +                                                 (void *)
> &rapl_model->pkg_events);
> +
>         ret = perf_pmu_register(&rapl_pmus_pkg->pmu, "power", -1);
>         if (ret)
>                 goto out;

[PATCH v6 10/10] perf/x86/rapl: Add core energy counter support for AMD CPUs

[Dhananjay Ugwekar]

Add a new "power_core" PMU and "energy-core" event for monitoring 
energy consumption by each individual core. The existing energy-cores 
event aggregates the energy consumption of CPU cores at the package level. 
This new event aligns with the AMD's per-core energy counters.

Tested the package level and core level PMU counters with workloads
pinned to different CPUs.

Results with workload pinned to CPU 4 in core 4 on an AMD Zen4 Genoa
machine:

$ sudo perf stat --per-core -e power_core/energy-core/ -- taskset -c 4 stress-ng --matrix 1 --timeout 5s
stress-ng: info:  [21250] setting to a 5 second run per stressor
stress-ng: info:  [21250] dispatching hogs: 1 matrix
stress-ng: info:  [21250] successful run completed in 5.00s

 Performance counter stats for 'system wide':

S0-D0-C0              1               0.00 Joules power_core/energy-core/
S0-D0-C1              1               0.00 Joules power_core/energy-core/
S0-D0-C2              1               0.00 Joules power_core/energy-core/
S0-D0-C3              1               0.00 Joules power_core/energy-core/
S0-D0-C4              1               8.43 Joules power_core/energy-core/
S0-D0-C5              1               0.00 Joules power_core/energy-core/
S0-D0-C6              1               0.00 Joules power_core/energy-core/
S0-D0-C7              1               0.00 Joules power_core/energy-core/
S0-D1-C8              1               0.00 Joules power_core/energy-core/
S0-D1-C9              1               0.00 Joules power_core/energy-core/
S0-D1-C10             1               0.00 Joules power_core/energy-core/

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
v6 changes:
* Replaced the if-ladder in get_rapl_pmu_idx() with a switch case. (Gautham)
* Added the error if condition in init_rapl_pmus().
* Modify the PMU name from "power_per_core" to "power_core" and event name
  from "event_per_core" to "event_core"
---
 arch/x86/events/rapl.c | 182 +++++++++++++++++++++++++++++++++--------
 1 file changed, 150 insertions(+), 32 deletions(-)

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index d3acc70a3d31..57cf15a19b7c 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -39,6 +39,10 @@
  *	  event: rapl_energy_psys
  *    perf code: 0x5
  *
+ *  core counter: consumption of a single physical core
+ *	  event: rapl_energy_core (power_core PMU)
+ *    perf code: 0x1
+ *
  * We manage those counters as free running (read-only). They may be
  * use simultaneously by other tools, such as turbostat.
  *
@@ -81,6 +85,10 @@ enum perf_rapl_pkg_events {
 	NR_RAPL_PKG_DOMAINS = PERF_RAPL_PKG_EVENTS_MAX,
 };
 
+#define PERF_RAPL_CORE			0		/* single core */
+#define PERF_RAPL_CORE_EVENTS_MAX	1
+#define NR_RAPL_CORE_DOMAINS		PERF_RAPL_CORE_EVENTS_MAX
+
 static const char *const rapl_pkg_domain_names[NR_RAPL_PKG_DOMAINS] __initconst = {
 	"pp0-core",
 	"package",
@@ -89,6 +97,8 @@ static const char *const rapl_pkg_domain_names[NR_RAPL_PKG_DOMAINS] __initconst
 	"psys",
 };
 
+static const char *const rapl_core_domain_name __initconst = "core";
+
 /*
  * event code: LSB 8 bits, passed in attr->config
  * any other bit is reserved
@@ -141,14 +151,18 @@ enum rapl_unit_quirk {
 
 struct rapl_model {
 	struct perf_msr *rapl_pkg_msrs;
+	struct perf_msr *rapl_core_msrs;
 	unsigned long	pkg_events;
+	unsigned long	core_events;
 	unsigned int	msr_power_unit;
 	enum rapl_unit_quirk	unit_quirk;
 };
 
  /* 1/2^hw_unit Joule */
 static int rapl_pkg_hw_unit[NR_RAPL_PKG_DOMAINS] __read_mostly;
+static int rapl_core_hw_unit __read_mostly;
 static struct rapl_pmus *rapl_pmus_pkg;
+static struct rapl_pmus *rapl_pmus_core;
 static u64 rapl_timer_ms;
 static struct rapl_model *rapl_model;
 
@@ -156,10 +170,18 @@ static struct rapl_model *rapl_model;
  * Helper function to get the correct topology id according to the
  * RAPL PMU scope.
  */
-static inline unsigned int get_rapl_pmu_idx(int cpu)
+static inline unsigned int get_rapl_pmu_idx(int cpu, int scope)
 {
-	return rapl_pkg_pmu_is_pkg_scope() ? topology_logical_package_id(cpu) :
-					 topology_logical_die_id(cpu);
+	switch (scope) {
+	case PERF_PMU_SCOPE_PKG:
+		return topology_logical_package_id(cpu);
+	case PERF_PMU_SCOPE_DIE:
+		return topology_logical_die_id(cpu);
+	case PERF_PMU_SCOPE_CORE:
+		return topology_logical_core_id(cpu);
+	default:
+		return -EINVAL;
+	}
 }
 
 static inline u64 rapl_read_counter(struct perf_event *event)
@@ -169,19 +191,20 @@ static inline u64 rapl_read_counter(struct perf_event *event)
 	return raw;
 }
 
-static inline u64 rapl_scale(u64 v, int cfg)
+static inline u64 rapl_scale(u64 v, struct perf_event *event)
 {
-	if (cfg > NR_RAPL_PKG_DOMAINS) {
-		pr_warn("Invalid domain %d, failed to scale data\n", cfg);
-		return v;
-	}
+	int hw_unit = rapl_pkg_hw_unit[event->hw.config - 1];
+
+	if (event->pmu->scope == PERF_PMU_SCOPE_CORE)
+		hw_unit = rapl_core_hw_unit;
+
 	/*
 	 * scale delta to smallest unit (1/2^32)
 	 * users must then scale back: count * 1/(1e9*2^32) to get Joules
 	 * or use ldexp(count, -32).
 	 * Watts = Joules/Time delta
 	 */
-	return v << (32 - rapl_pkg_hw_unit[cfg - 1]);
+	return v << (32 - hw_unit);
 }
 
 static u64 rapl_event_update(struct perf_event *event)
@@ -208,7 +231,7 @@ static u64 rapl_event_update(struct perf_event *event)
 	delta = (new_raw_count << shift) - (prev_raw_count << shift);
 	delta >>= shift;
 
-	sdelta = rapl_scale(delta, event->hw.config);
+	sdelta = rapl_scale(delta, event);
 
 	local64_add(sdelta, &event->count);
 
@@ -337,12 +360,13 @@ static void rapl_pmu_event_del(struct perf_event *event, int flags)
 static int rapl_pmu_event_init(struct perf_event *event)
 {
 	u64 cfg = event->attr.config & RAPL_EVENT_MASK;
-	int bit, rapl_pmu_idx, ret = 0;
+	int bit, rapl_pmus_scope, rapl_pmu_idx, ret = 0;
 	struct rapl_pmu *rapl_pmu;
+	struct rapl_pmus *rapl_pmus;
 
-	/* only look at RAPL events */
-	if (event->attr.type != rapl_pmus_pkg->pmu.type)
-		return -ENOENT;
+	/* unsupported modes and filters */
+	if (event->attr.sample_period) /* no sampling */
+		return -EINVAL;
 
 	/* check only supported bits are set */
 	if (event->attr.config & ~RAPL_EVENT_MASK)
@@ -351,31 +375,49 @@ static int rapl_pmu_event_init(struct perf_event *event)
 	if (event->cpu < 0)
 		return -EINVAL;
 
-	if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
+	rapl_pmus = container_of(event->pmu, struct rapl_pmus, pmu);
+	if (!rapl_pmus)
 		return -EINVAL;
-
-	cfg = array_index_nospec((long)cfg, NR_RAPL_PKG_DOMAINS + 1);
-	bit = cfg - 1;
-
-	/* check event supported */
-	if (!(rapl_pmus_pkg->cntr_mask & (1 << bit)))
+	rapl_pmus_scope = rapl_pmus->pmu.scope;
+
+	if (rapl_pmus_scope == PERF_PMU_SCOPE_PKG || rapl_pmus_scope == PERF_PMU_SCOPE_DIE) {
+		/* only look at RAPL package events */
+		if (event->attr.type != rapl_pmus_pkg->pmu.type)
+			return -ENOENT;
+
+		cfg = array_index_nospec((long)cfg, NR_RAPL_PKG_DOMAINS + 1);
+		if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
+			return -EINVAL;
+
+		bit = cfg - 1;
+		event->hw.event_base = rapl_model->rapl_pkg_msrs[bit].msr;
+	} else if (rapl_pmus_scope == PERF_PMU_SCOPE_CORE) {
+		/* only look at RAPL core events */
+		if (event->attr.type != rapl_pmus_core->pmu.type)
+			return -ENOENT;
+
+		cfg = array_index_nospec((long)cfg, NR_RAPL_CORE_DOMAINS + 1);
+		if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
+			return -EINVAL;
+
+		bit = cfg - 1;
+		event->hw.event_base = rapl_model->rapl_core_msrs[bit].msr;
+	} else
 		return -EINVAL;
 
-	/* unsupported modes and filters */
-	if (event->attr.sample_period) /* no sampling */
+	/* check event supported */
+	if (!(rapl_pmus->cntr_mask & (1 << bit)))
 		return -EINVAL;
 
-	rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
-	if (rapl_pmu_idx >= rapl_pmus_pkg->nr_rapl_pmu)
+	rapl_pmu_idx = get_rapl_pmu_idx(event->cpu, rapl_pmus_scope);
+	if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
 		return -EINVAL;
-
 	/* must be done before validate_group */
-	rapl_pmu = rapl_pmus_pkg->rapl_pmu[rapl_pmu_idx];
+	rapl_pmu = rapl_pmus->rapl_pmu[rapl_pmu_idx];
 	if (!rapl_pmu)
 		return -EINVAL;
 
 	event->pmu_private = rapl_pmu;
-	event->hw.event_base = rapl_model->rapl_pkg_msrs[bit].msr;
 	event->hw.config = cfg;
 	event->hw.idx = bit;
 
@@ -392,12 +434,14 @@ RAPL_EVENT_ATTR_STR(energy-pkg  ,   rapl_pkg, "event=0x02");
 RAPL_EVENT_ATTR_STR(energy-ram  ,   rapl_ram, "event=0x03");
 RAPL_EVENT_ATTR_STR(energy-gpu  ,   rapl_gpu, "event=0x04");
 RAPL_EVENT_ATTR_STR(energy-psys,   rapl_psys, "event=0x05");
+RAPL_EVENT_ATTR_STR(energy-core,   rapl_core, "event=0x01");
 
 RAPL_EVENT_ATTR_STR(energy-cores.unit, rapl_cores_unit, "Joules");
 RAPL_EVENT_ATTR_STR(energy-pkg.unit  ,   rapl_pkg_unit, "Joules");
 RAPL_EVENT_ATTR_STR(energy-ram.unit  ,   rapl_ram_unit, "Joules");
 RAPL_EVENT_ATTR_STR(energy-gpu.unit  ,   rapl_gpu_unit, "Joules");
 RAPL_EVENT_ATTR_STR(energy-psys.unit,   rapl_psys_unit, "Joules");
+RAPL_EVENT_ATTR_STR(energy-core.unit,   rapl_core_unit, "Joules");
 
 /*
  * we compute in 0.23 nJ increments regardless of MSR
@@ -407,6 +451,7 @@ RAPL_EVENT_ATTR_STR(energy-pkg.scale,     rapl_pkg_scale, "2.3283064365386962890
 RAPL_EVENT_ATTR_STR(energy-ram.scale,     rapl_ram_scale, "2.3283064365386962890625e-10");
 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");
+RAPL_EVENT_ATTR_STR(energy-core.scale,   rapl_core_scale, "2.3283064365386962890625e-10");
 
 /*
  * There are no default events, but we need to create
@@ -439,6 +484,12 @@ static const struct attribute_group *rapl_attr_groups[] = {
 	NULL,
 };
 
+static const struct attribute_group *rapl_core_attr_groups[] = {
+	&rapl_pmu_format_group,
+	&rapl_pmu_events_group,
+	NULL,
+};
+
 static struct attribute *rapl_events_cores[] = {
 	EVENT_PTR(rapl_cores),
 	EVENT_PTR(rapl_cores_unit),
@@ -499,6 +550,18 @@ static struct attribute_group rapl_events_psys_group = {
 	.attrs = rapl_events_psys,
 };
 
+static struct attribute *rapl_events_core[] = {
+	EVENT_PTR(rapl_core),
+	EVENT_PTR(rapl_core_unit),
+	EVENT_PTR(rapl_core_scale),
+	NULL,
+};
+
+static struct attribute_group rapl_events_core_group = {
+	.name  = "events",
+	.attrs = rapl_events_core,
+};
+
 static bool test_msr(int idx, void *data)
 {
 	return test_bit(idx, (unsigned long *) data);
@@ -536,6 +599,11 @@ static struct perf_msr amd_rapl_pkg_msrs[] = {
 	[PERF_RAPL_PSYS] = { 0, &rapl_events_psys_group,  NULL, false, 0 },
 };
 
+static struct perf_msr amd_rapl_core_msrs[] = {
+	[PERF_RAPL_CORE] = { MSR_AMD_CORE_ENERGY_STATUS, &rapl_events_core_group,
+				 test_msr, false, RAPL_MSR_MASK },
+};
+
 static int rapl_check_hw_unit(void)
 {
 	u64 msr_rapl_power_unit_bits;
@@ -547,6 +615,8 @@ static int rapl_check_hw_unit(void)
 	for (i = 0; i < NR_RAPL_PKG_DOMAINS; i++)
 		rapl_pkg_hw_unit[i] = (msr_rapl_power_unit_bits >> 8) & 0x1FULL;
 
+	rapl_core_hw_unit = (msr_rapl_power_unit_bits >> 8) & 0x1FULL;
+
 	switch (rapl_model->unit_quirk) {
 	/*
 	 * DRAM domain on HSW server and KNL has fixed energy unit which can be
@@ -565,7 +635,6 @@ static int rapl_check_hw_unit(void)
 		break;
 	}
 
-
 	/*
 	 * Calculate the timer rate:
 	 * Use reference of 200W for scaling the timeout to avoid counter
@@ -584,9 +653,13 @@ static int rapl_check_hw_unit(void)
 static void __init rapl_advertise(void)
 {
 	int i;
+	int num_counters = hweight32(rapl_pmus_pkg->cntr_mask);
+
+	if (rapl_pmus_core)
+		num_counters += hweight32(rapl_pmus_core->cntr_mask);
 
 	pr_info("API unit is 2^-32 Joules, %d fixed counters, %llu ms ovfl timer\n",
-		hweight32(rapl_pmus_pkg->cntr_mask), rapl_timer_ms);
+		num_counters, rapl_timer_ms);
 
 	for (i = 0; i < NR_RAPL_PKG_DOMAINS; i++) {
 		if (rapl_pmus_pkg->cntr_mask & (1 << i)) {
@@ -594,6 +667,10 @@ static void __init rapl_advertise(void)
 				rapl_pkg_domain_names[i], rapl_pkg_hw_unit[i]);
 		}
 	}
+
+	if (rapl_pmus_core && (rapl_pmus_core->cntr_mask & (1 << PERF_RAPL_CORE)))
+		pr_info("hw unit of domain %s 2^-%d Joules\n",
+			rapl_core_domain_name, rapl_core_hw_unit);
 }
 
 static void cleanup_rapl_pmus(struct rapl_pmus *rapl_pmus)
@@ -614,6 +691,10 @@ static const struct attribute_group *rapl_attr_update[] = {
 	NULL,
 };
 
+static const struct attribute_group *rapl_core_attr_update[] = {
+	&rapl_events_core_group,
+};
+
 static int __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
 {
 	struct rapl_pmu *rapl_pmu;
@@ -640,13 +721,22 @@ static int __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
 	return -ENOMEM;
 }
 
-static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr, int rapl_pmu_scope)
+static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr, int rapl_pmu_scope,
+				 const struct attribute_group **rapl_attr_groups,
+				 const struct attribute_group **rapl_attr_update)
 {
 	int nr_rapl_pmu = topology_max_packages();
 	struct rapl_pmus *rapl_pmus;
 
+	/*
+	 * rapl_pmu_scope must be either PKG, DIE or CORE
+	 */
 	if (rapl_pmu_scope == PERF_PMU_SCOPE_DIE)
 		nr_rapl_pmu	*= topology_max_dies_per_package();
+	else if (rapl_pmu_scope == PERF_PMU_SCOPE_CORE)
+		nr_rapl_pmu	*= topology_num_cores_per_package();
+	else if (rapl_pmu_scope != PERF_PMU_SCOPE_PKG)
+		return -EINVAL;
 
 	rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu, nr_rapl_pmu), GFP_KERNEL);
 	if (!rapl_pmus)
@@ -735,8 +825,10 @@ static struct rapl_model model_spr = {
 
 static struct rapl_model model_amd_hygon = {
 	.pkg_events	= BIT(PERF_RAPL_PKG),
+	.core_events	= BIT(PERF_RAPL_CORE),
 	.msr_power_unit = MSR_AMD_RAPL_POWER_UNIT,
 	.rapl_pkg_msrs	= amd_rapl_pkg_msrs,
+	.rapl_core_msrs	= amd_rapl_core_msrs,
 };
 
 static const struct x86_cpu_id rapl_model_match[] __initconst = {
@@ -808,7 +900,8 @@ static int __init rapl_pmu_init(void)
 	if (ret)
 		return ret;
 
-	ret = init_rapl_pmus(&rapl_pmus_pkg, rapl_pkg_pmu_scope);
+	ret = init_rapl_pmus(&rapl_pmus_pkg, rapl_pkg_pmu_scope, rapl_attr_groups,
+			     rapl_attr_update);
 	if (ret)
 		return ret;
 
@@ -820,6 +913,27 @@ static int __init rapl_pmu_init(void)
 	if (ret)
 		goto out;
 
+	if (rapl_model->core_events) {
+		ret = init_rapl_pmus(&rapl_pmus_core, PERF_PMU_SCOPE_CORE,
+				     rapl_core_attr_groups,
+				     rapl_core_attr_update);
+		if (ret) {
+			pr_warn("power-core PMU initialization failed (%d)\n", ret);
+			goto core_init_failed;
+		}
+
+		rapl_pmus_core->cntr_mask = perf_msr_probe(rapl_model->rapl_core_msrs,
+						     PERF_RAPL_CORE_EVENTS_MAX, false,
+						     (void *) &rapl_model->core_events);
+
+		ret = perf_pmu_register(&rapl_pmus_core->pmu, "power_core", -1);
+		if (ret) {
+			pr_warn("power-core PMU registration failed (%d)\n", ret);
+			cleanup_rapl_pmus(rapl_pmus_core);
+		}
+	}
+
+core_init_failed:
 	rapl_advertise();
 	return 0;
 
@@ -832,6 +946,10 @@ module_init(rapl_pmu_init);
 
 static void __exit intel_rapl_exit(void)
 {
+	if (rapl_pmus_core) {
+		perf_pmu_unregister(&rapl_pmus_core->pmu);
+		cleanup_rapl_pmus(rapl_pmus_core);
+	}
 	perf_pmu_unregister(&rapl_pmus_pkg->pmu);
 	cleanup_rapl_pmus(rapl_pmus_pkg);
 }
-- 
2.34.1

Re: [PATCH v6 10/10] perf/x86/rapl: Add core energy counter support for AMD CPUs

[Gautham R. Shenoy]

Hello Dhananjay,


On Fri, Oct 25, 2024 at 11:13:48AM +0000, Dhananjay Ugwekar wrote:
> Add a new "power_core" PMU and "energy-core" event for monitoring 
> energy consumption by each individual core. The existing energy-cores 
> event aggregates the energy consumption of CPU cores at the package level. 
> This new event aligns with the AMD's per-core energy counters.
> 
> Tested the package level and core level PMU counters with workloads
> pinned to different CPUs.
> 
> Results with workload pinned to CPU 4 in core 4 on an AMD Zen4 Genoa
> machine:
> 
> $ sudo perf stat --per-core -e power_core/energy-core/ -- taskset -c 4 stress-ng --matrix 1 --timeout 5s
> stress-ng: info:  [21250] setting to a 5 second run per stressor
> stress-ng: info:  [21250] dispatching hogs: 1 matrix
> stress-ng: info:  [21250] successful run completed in 5.00s
> 
>  Performance counter stats for 'system wide':
> 
> S0-D0-C0              1               0.00 Joules power_core/energy-core/
> S0-D0-C1              1               0.00 Joules power_core/energy-core/
> S0-D0-C2              1               0.00 Joules power_core/energy-core/
> S0-D0-C3              1               0.00 Joules power_core/energy-core/
> S0-D0-C4              1               8.43 Joules power_core/energy-core/
> S0-D0-C5              1               0.00 Joules power_core/energy-core/
> S0-D0-C6              1               0.00 Joules power_core/energy-core/
> S0-D0-C7              1               0.00 Joules power_core/energy-core/
> S0-D1-C8              1               0.00 Joules power_core/energy-core/
> S0-D1-C9              1               0.00 Joules power_core/energy-core/
> S0-D1-C10             1               0.00 Joules power_core/energy-core/
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

Looks like my response to this patch never made it to the list.

The patch looks good to me.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.



> ---
> v6 changes:
> * Replaced the if-ladder in get_rapl_pmu_idx() with a switch case. (Gautham)
> * Added the error if condition in init_rapl_pmus().
> * Modify the PMU name from "power_per_core" to "power_core" and event name
>   from "event_per_core" to "event_core"
> ---
>  arch/x86/events/rapl.c | 182 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 150 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index d3acc70a3d31..57cf15a19b7c 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -39,6 +39,10 @@
>   *	  event: rapl_energy_psys
>   *    perf code: 0x5
>   *
> + *  core counter: consumption of a single physical core
> + *	  event: rapl_energy_core (power_core PMU)
> + *    perf code: 0x1
> + *
>   * We manage those counters as free running (read-only). They may be
>   * use simultaneously by other tools, such as turbostat.
>   *
> @@ -81,6 +85,10 @@ enum perf_rapl_pkg_events {
>  	NR_RAPL_PKG_DOMAINS = PERF_RAPL_PKG_EVENTS_MAX,
>  };
>  
> +#define PERF_RAPL_CORE			0		/* single core */
> +#define PERF_RAPL_CORE_EVENTS_MAX	1
> +#define NR_RAPL_CORE_DOMAINS		PERF_RAPL_CORE_EVENTS_MAX
> +
>  static const char *const rapl_pkg_domain_names[NR_RAPL_PKG_DOMAINS] __initconst = {
>  	"pp0-core",
>  	"package",
> @@ -89,6 +97,8 @@ static const char *const rapl_pkg_domain_names[NR_RAPL_PKG_DOMAINS] __initconst
>  	"psys",
>  };
>  
> +static const char *const rapl_core_domain_name __initconst = "core";
> +
>  /*
>   * event code: LSB 8 bits, passed in attr->config
>   * any other bit is reserved
> @@ -141,14 +151,18 @@ enum rapl_unit_quirk {
>  
>  struct rapl_model {
>  	struct perf_msr *rapl_pkg_msrs;
> +	struct perf_msr *rapl_core_msrs;
>  	unsigned long	pkg_events;
> +	unsigned long	core_events;
>  	unsigned int	msr_power_unit;
>  	enum rapl_unit_quirk	unit_quirk;
>  };
>  
>   /* 1/2^hw_unit Joule */
>  static int rapl_pkg_hw_unit[NR_RAPL_PKG_DOMAINS] __read_mostly;
> +static int rapl_core_hw_unit __read_mostly;
>  static struct rapl_pmus *rapl_pmus_pkg;
> +static struct rapl_pmus *rapl_pmus_core;
>  static u64 rapl_timer_ms;
>  static struct rapl_model *rapl_model;
>  
> @@ -156,10 +170,18 @@ static struct rapl_model *rapl_model;
>   * Helper function to get the correct topology id according to the
>   * RAPL PMU scope.
>   */
> -static inline unsigned int get_rapl_pmu_idx(int cpu)
> +static inline unsigned int get_rapl_pmu_idx(int cpu, int scope)
>  {
> -	return rapl_pkg_pmu_is_pkg_scope() ? topology_logical_package_id(cpu) :
> -					 topology_logical_die_id(cpu);
> +	switch (scope) {
> +	case PERF_PMU_SCOPE_PKG:
> +		return topology_logical_package_id(cpu);
> +	case PERF_PMU_SCOPE_DIE:
> +		return topology_logical_die_id(cpu);
> +	case PERF_PMU_SCOPE_CORE:
> +		return topology_logical_core_id(cpu);
> +	default:
> +		return -EINVAL;
> +	}
>  }
>  
>  static inline u64 rapl_read_counter(struct perf_event *event)
> @@ -169,19 +191,20 @@ static inline u64 rapl_read_counter(struct perf_event *event)
>  	return raw;
>  }
>  
> -static inline u64 rapl_scale(u64 v, int cfg)
> +static inline u64 rapl_scale(u64 v, struct perf_event *event)
>  {
> -	if (cfg > NR_RAPL_PKG_DOMAINS) {
> -		pr_warn("Invalid domain %d, failed to scale data\n", cfg);
> -		return v;
> -	}
> +	int hw_unit = rapl_pkg_hw_unit[event->hw.config - 1];
> +
> +	if (event->pmu->scope == PERF_PMU_SCOPE_CORE)
> +		hw_unit = rapl_core_hw_unit;
> +
>  	/*
>  	 * scale delta to smallest unit (1/2^32)
>  	 * users must then scale back: count * 1/(1e9*2^32) to get Joules
>  	 * or use ldexp(count, -32).
>  	 * Watts = Joules/Time delta
>  	 */
> -	return v << (32 - rapl_pkg_hw_unit[cfg - 1]);
> +	return v << (32 - hw_unit);
>  }
>  
>  static u64 rapl_event_update(struct perf_event *event)
> @@ -208,7 +231,7 @@ static u64 rapl_event_update(struct perf_event *event)
>  	delta = (new_raw_count << shift) - (prev_raw_count << shift);
>  	delta >>= shift;
>  
> -	sdelta = rapl_scale(delta, event->hw.config);
> +	sdelta = rapl_scale(delta, event);
>  
>  	local64_add(sdelta, &event->count);
>  
> @@ -337,12 +360,13 @@ static void rapl_pmu_event_del(struct perf_event *event, int flags)
>  static int rapl_pmu_event_init(struct perf_event *event)
>  {
>  	u64 cfg = event->attr.config & RAPL_EVENT_MASK;
> -	int bit, rapl_pmu_idx, ret = 0;
> +	int bit, rapl_pmus_scope, rapl_pmu_idx, ret = 0;
>  	struct rapl_pmu *rapl_pmu;
> +	struct rapl_pmus *rapl_pmus;
>  
> -	/* only look at RAPL events */
> -	if (event->attr.type != rapl_pmus_pkg->pmu.type)
> -		return -ENOENT;
> +	/* unsupported modes and filters */
> +	if (event->attr.sample_period) /* no sampling */
> +		return -EINVAL;
>  
>  	/* check only supported bits are set */
>  	if (event->attr.config & ~RAPL_EVENT_MASK)
> @@ -351,31 +375,49 @@ static int rapl_pmu_event_init(struct perf_event *event)
>  	if (event->cpu < 0)
>  		return -EINVAL;
>  
> -	if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
> +	rapl_pmus = container_of(event->pmu, struct rapl_pmus, pmu);
> +	if (!rapl_pmus)
>  		return -EINVAL;
> -
> -	cfg = array_index_nospec((long)cfg, NR_RAPL_PKG_DOMAINS + 1);
> -	bit = cfg - 1;
> -
> -	/* check event supported */
> -	if (!(rapl_pmus_pkg->cntr_mask & (1 << bit)))
> +	rapl_pmus_scope = rapl_pmus->pmu.scope;
> +
> +	if (rapl_pmus_scope == PERF_PMU_SCOPE_PKG || rapl_pmus_scope == PERF_PMU_SCOPE_DIE) {
> +		/* only look at RAPL package events */
> +		if (event->attr.type != rapl_pmus_pkg->pmu.type)
> +			return -ENOENT;
> +
> +		cfg = array_index_nospec((long)cfg, NR_RAPL_PKG_DOMAINS + 1);
> +		if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
> +			return -EINVAL;
> +
> +		bit = cfg - 1;
> +		event->hw.event_base = rapl_model->rapl_pkg_msrs[bit].msr;
> +	} else if (rapl_pmus_scope == PERF_PMU_SCOPE_CORE) {
> +		/* only look at RAPL core events */
> +		if (event->attr.type != rapl_pmus_core->pmu.type)
> +			return -ENOENT;
> +
> +		cfg = array_index_nospec((long)cfg, NR_RAPL_CORE_DOMAINS + 1);
> +		if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
> +			return -EINVAL;
> +
> +		bit = cfg - 1;
> +		event->hw.event_base = rapl_model->rapl_core_msrs[bit].msr;
> +	} else
>  		return -EINVAL;
>  
> -	/* unsupported modes and filters */
> -	if (event->attr.sample_period) /* no sampling */
> +	/* check event supported */
> +	if (!(rapl_pmus->cntr_mask & (1 << bit)))
>  		return -EINVAL;
>  
> -	rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
> -	if (rapl_pmu_idx >= rapl_pmus_pkg->nr_rapl_pmu)
> +	rapl_pmu_idx = get_rapl_pmu_idx(event->cpu, rapl_pmus_scope);
> +	if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
>  		return -EINVAL;
> -
>  	/* must be done before validate_group */
> -	rapl_pmu = rapl_pmus_pkg->rapl_pmu[rapl_pmu_idx];
> +	rapl_pmu = rapl_pmus->rapl_pmu[rapl_pmu_idx];
>  	if (!rapl_pmu)
>  		return -EINVAL;
>  
>  	event->pmu_private = rapl_pmu;
> -	event->hw.event_base = rapl_model->rapl_pkg_msrs[bit].msr;
>  	event->hw.config = cfg;
>  	event->hw.idx = bit;
>  
> @@ -392,12 +434,14 @@ RAPL_EVENT_ATTR_STR(energy-pkg  ,   rapl_pkg, "event=0x02");
>  RAPL_EVENT_ATTR_STR(energy-ram  ,   rapl_ram, "event=0x03");
>  RAPL_EVENT_ATTR_STR(energy-gpu  ,   rapl_gpu, "event=0x04");
>  RAPL_EVENT_ATTR_STR(energy-psys,   rapl_psys, "event=0x05");
> +RAPL_EVENT_ATTR_STR(energy-core,   rapl_core, "event=0x01");
>  
>  RAPL_EVENT_ATTR_STR(energy-cores.unit, rapl_cores_unit, "Joules");
>  RAPL_EVENT_ATTR_STR(energy-pkg.unit  ,   rapl_pkg_unit, "Joules");
>  RAPL_EVENT_ATTR_STR(energy-ram.unit  ,   rapl_ram_unit, "Joules");
>  RAPL_EVENT_ATTR_STR(energy-gpu.unit  ,   rapl_gpu_unit, "Joules");
>  RAPL_EVENT_ATTR_STR(energy-psys.unit,   rapl_psys_unit, "Joules");
> +RAPL_EVENT_ATTR_STR(energy-core.unit,   rapl_core_unit, "Joules");
>  
>  /*
>   * we compute in 0.23 nJ increments regardless of MSR
> @@ -407,6 +451,7 @@ RAPL_EVENT_ATTR_STR(energy-pkg.scale,     rapl_pkg_scale, "2.3283064365386962890
>  RAPL_EVENT_ATTR_STR(energy-ram.scale,     rapl_ram_scale, "2.3283064365386962890625e-10");
>  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");
> +RAPL_EVENT_ATTR_STR(energy-core.scale,   rapl_core_scale, "2.3283064365386962890625e-10");
>  
>  /*
>   * There are no default events, but we need to create
> @@ -439,6 +484,12 @@ static const struct attribute_group *rapl_attr_groups[] = {
>  	NULL,
>  };
>  
> +static const struct attribute_group *rapl_core_attr_groups[] = {
> +	&rapl_pmu_format_group,
> +	&rapl_pmu_events_group,
> +	NULL,
> +};
> +
>  static struct attribute *rapl_events_cores[] = {
>  	EVENT_PTR(rapl_cores),
>  	EVENT_PTR(rapl_cores_unit),
> @@ -499,6 +550,18 @@ static struct attribute_group rapl_events_psys_group = {
>  	.attrs = rapl_events_psys,
>  };
>  
> +static struct attribute *rapl_events_core[] = {
> +	EVENT_PTR(rapl_core),
> +	EVENT_PTR(rapl_core_unit),
> +	EVENT_PTR(rapl_core_scale),
> +	NULL,
> +};
> +
> +static struct attribute_group rapl_events_core_group = {
> +	.name  = "events",
> +	.attrs = rapl_events_core,
> +};
> +
>  static bool test_msr(int idx, void *data)
>  {
>  	return test_bit(idx, (unsigned long *) data);
> @@ -536,6 +599,11 @@ static struct perf_msr amd_rapl_pkg_msrs[] = {
>  	[PERF_RAPL_PSYS] = { 0, &rapl_events_psys_group,  NULL, false, 0 },
>  };
>  
> +static struct perf_msr amd_rapl_core_msrs[] = {
> +	[PERF_RAPL_CORE] = { MSR_AMD_CORE_ENERGY_STATUS, &rapl_events_core_group,
> +				 test_msr, false, RAPL_MSR_MASK },
> +};
> +
>  static int rapl_check_hw_unit(void)
>  {
>  	u64 msr_rapl_power_unit_bits;
> @@ -547,6 +615,8 @@ static int rapl_check_hw_unit(void)
>  	for (i = 0; i < NR_RAPL_PKG_DOMAINS; i++)
>  		rapl_pkg_hw_unit[i] = (msr_rapl_power_unit_bits >> 8) & 0x1FULL;
>  
> +	rapl_core_hw_unit = (msr_rapl_power_unit_bits >> 8) & 0x1FULL;
> +
>  	switch (rapl_model->unit_quirk) {
>  	/*
>  	 * DRAM domain on HSW server and KNL has fixed energy unit which can be
> @@ -565,7 +635,6 @@ static int rapl_check_hw_unit(void)
>  		break;
>  	}
>  
> -
>  	/*
>  	 * Calculate the timer rate:
>  	 * Use reference of 200W for scaling the timeout to avoid counter
> @@ -584,9 +653,13 @@ static int rapl_check_hw_unit(void)
>  static void __init rapl_advertise(void)
>  {
>  	int i;
> +	int num_counters = hweight32(rapl_pmus_pkg->cntr_mask);
> +
> +	if (rapl_pmus_core)
> +		num_counters += hweight32(rapl_pmus_core->cntr_mask);
>  
>  	pr_info("API unit is 2^-32 Joules, %d fixed counters, %llu ms ovfl timer\n",
> -		hweight32(rapl_pmus_pkg->cntr_mask), rapl_timer_ms);
> +		num_counters, rapl_timer_ms);
>  
>  	for (i = 0; i < NR_RAPL_PKG_DOMAINS; i++) {
>  		if (rapl_pmus_pkg->cntr_mask & (1 << i)) {
> @@ -594,6 +667,10 @@ static void __init rapl_advertise(void)
>  				rapl_pkg_domain_names[i], rapl_pkg_hw_unit[i]);
>  		}
>  	}
> +
> +	if (rapl_pmus_core && (rapl_pmus_core->cntr_mask & (1 << PERF_RAPL_CORE)))
> +		pr_info("hw unit of domain %s 2^-%d Joules\n",
> +			rapl_core_domain_name, rapl_core_hw_unit);
>  }
>  
>  static void cleanup_rapl_pmus(struct rapl_pmus *rapl_pmus)
> @@ -614,6 +691,10 @@ static const struct attribute_group *rapl_attr_update[] = {
>  	NULL,
>  };
>  
> +static const struct attribute_group *rapl_core_attr_update[] = {
> +	&rapl_events_core_group,
> +};
> +
>  static int __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
>  {
>  	struct rapl_pmu *rapl_pmu;
> @@ -640,13 +721,22 @@ static int __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
>  	return -ENOMEM;
>  }
>  
> -static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr, int rapl_pmu_scope)
> +static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr, int rapl_pmu_scope,
> +				 const struct attribute_group **rapl_attr_groups,
> +				 const struct attribute_group **rapl_attr_update)
>  {
>  	int nr_rapl_pmu = topology_max_packages();
>  	struct rapl_pmus *rapl_pmus;
>  
> +	/*
> +	 * rapl_pmu_scope must be either PKG, DIE or CORE
> +	 */
>  	if (rapl_pmu_scope == PERF_PMU_SCOPE_DIE)
>  		nr_rapl_pmu	*= topology_max_dies_per_package();
> +	else if (rapl_pmu_scope == PERF_PMU_SCOPE_CORE)
> +		nr_rapl_pmu	*= topology_num_cores_per_package();
> +	else if (rapl_pmu_scope != PERF_PMU_SCOPE_PKG)
> +		return -EINVAL;
>  
>  	rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu, nr_rapl_pmu), GFP_KERNEL);
>  	if (!rapl_pmus)
> @@ -735,8 +825,10 @@ static struct rapl_model model_spr = {
>  
>  static struct rapl_model model_amd_hygon = {
>  	.pkg_events	= BIT(PERF_RAPL_PKG),
> +	.core_events	= BIT(PERF_RAPL_CORE),
>  	.msr_power_unit = MSR_AMD_RAPL_POWER_UNIT,
>  	.rapl_pkg_msrs	= amd_rapl_pkg_msrs,
> +	.rapl_core_msrs	= amd_rapl_core_msrs,
>  };
>  
>  static const struct x86_cpu_id rapl_model_match[] __initconst = {
> @@ -808,7 +900,8 @@ static int __init rapl_pmu_init(void)
>  	if (ret)
>  		return ret;
>  
> -	ret = init_rapl_pmus(&rapl_pmus_pkg, rapl_pkg_pmu_scope);
> +	ret = init_rapl_pmus(&rapl_pmus_pkg, rapl_pkg_pmu_scope, rapl_attr_groups,
> +			     rapl_attr_update);
>  	if (ret)
>  		return ret;
>  
> @@ -820,6 +913,27 @@ static int __init rapl_pmu_init(void)
>  	if (ret)
>  		goto out;
>  
> +	if (rapl_model->core_events) {
> +		ret = init_rapl_pmus(&rapl_pmus_core, PERF_PMU_SCOPE_CORE,
> +				     rapl_core_attr_groups,
> +				     rapl_core_attr_update);
> +		if (ret) {
> +			pr_warn("power-core PMU initialization failed (%d)\n", ret);
> +			goto core_init_failed;
> +		}
> +
> +		rapl_pmus_core->cntr_mask = perf_msr_probe(rapl_model->rapl_core_msrs,
> +						     PERF_RAPL_CORE_EVENTS_MAX, false,
> +						     (void *) &rapl_model->core_events);
> +
> +		ret = perf_pmu_register(&rapl_pmus_core->pmu, "power_core", -1);
> +		if (ret) {
> +			pr_warn("power-core PMU registration failed (%d)\n", ret);
> +			cleanup_rapl_pmus(rapl_pmus_core);
> +		}
> +	}
> +
> +core_init_failed:
>  	rapl_advertise();
>  	return 0;
>  
> @@ -832,6 +946,10 @@ module_init(rapl_pmu_init);
>  
>  static void __exit intel_rapl_exit(void)
>  {
> +	if (rapl_pmus_core) {
> +		perf_pmu_unregister(&rapl_pmus_core->pmu);
> +		cleanup_rapl_pmus(rapl_pmus_core);
> +	}
>  	perf_pmu_unregister(&rapl_pmus_pkg->pmu);
>  	cleanup_rapl_pmus(rapl_pmus_pkg);
>  }
> -- 
> 2.34.1
>