linux-perf-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/9] Add per-core RAPL energy counter support for AMD CPUs
@ 2024-09-13 15:21 Dhananjay Ugwekar
  2024-09-13 15:21 ` [PATCH v5 1/9] x86/topology: Introduce topology_logical_core_id() Dhananjay Ugwekar
                   ` (5 more replies)
  0 siblings, 6 replies; 18+ messages in thread
From: Dhananjay Ugwekar @ 2024-09-13 15:21 UTC (permalink / raw)
  To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
	jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen,
	x86, hpa, rui.zhang, oleksandr
  Cc: eranian, gautham.shenoy, ravi.bangoria, linux-perf-users,
	linux-kernel, 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_per_core" PMU alongside the existing
"power" PMU, which will be responsible for collecting the new
"energy-per-core" event.

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

Results with workload pinned to CPU 1 in Core 1 on an AMD Zen4 Genoa 
machine:

$ perf stat -a --per-core -e power_per_core/energy-per-core/ -- sleep 1

 Performance counter stats for 'system wide':

S0-D0-C0         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C1         1          5.72 Joules power_per_core/energy-per-core/
S0-D0-C2         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C3         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C4         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C5         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C6         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C7         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C8         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C9         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C10        1          0.02 Joules power_per_core/energy-per-core/

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

v5 changes:
* Rebase on top of Kan Liang's "PMU scope" patchset [2]
* rapl_cntr_mask moved to rapl_pmus struct in patch 8
* Patch 1 from v4 is merged separately, so removed from this series
* Add an extra argument "scope" in patch 5 to the init functions
* Add an new patch 2, which removes the cpu_to_rapl_pmu() function

Base: tip/perf/core(currently has just 1-5 patches from [2]) + patch 6 from [2] + 
      diff [3] + patch 7 from [2] + revert [4] + apply [5] 

[1]: https://lore.kernel.org/lkml/3e766f0e-37d4-0f82-3868-31b14228868d@linux.intel.com/
[2]: https://lore.kernel.org/all/20240802151643.1691631-1-kan.liang@linux.intel.com/
[3]: https://lore.kernel.org/all/8c09633c-5bf2-48a2-91a6-a0af9b9f2e8c@linux.intel.com/
[4]: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=perf/core&id=8d72eba1cf8cecd76a2b4c1dd7673c2dc775f514
[5]: https://lore.kernel.org/all/20240910085504.204814-1-Dhananjay.Ugwekar@amd.com/

Dhananjay Ugwekar (8):
  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 cleanup and init 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 per-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                | 406 ++++++++++++++++----------
 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, 266 insertions(+), 148 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v5 1/9] x86/topology: Introduce topology_logical_core_id()
  2024-09-13 15:21 [PATCH v5 0/9] Add per-core RAPL energy counter support for AMD CPUs Dhananjay Ugwekar
@ 2024-09-13 15:21 ` Dhananjay Ugwekar
  2024-09-13 15:21 ` [PATCH v5 2/9] perf/x86/rapl: Remove the cpu_to_rapl_pmu() function Dhananjay Ugwekar
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: Dhananjay Ugwekar @ 2024-09-13 15:21 UTC (permalink / raw)
  To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
	jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen,
	x86, hpa, rui.zhang, oleksandr
  Cc: eranian, gautham.shenoy, ravi.bangoria, linux-perf-users,
	linux-kernel, K Prateek Nayak, 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 per-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>
Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
Reviewed-by: Zhang Rui <rui.zhang@intel.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 a75a07f4931f..5a59713ec62b 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 abe3a8f22cbd..2a6dbf965d92 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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v5 2/9] perf/x86/rapl: Remove the cpu_to_rapl_pmu() function
  2024-09-13 15:21 [PATCH v5 0/9] Add per-core RAPL energy counter support for AMD CPUs Dhananjay Ugwekar
  2024-09-13 15:21 ` [PATCH v5 1/9] x86/topology: Introduce topology_logical_core_id() Dhananjay Ugwekar
@ 2024-09-13 15:21 ` Dhananjay Ugwekar
  2024-09-13 15:21 ` [PATCH v5 3/9] perf/x86/rapl: Rename rapl_pmu variables Dhananjay Ugwekar
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: Dhananjay Ugwekar @ 2024-09-13 15:21 UTC (permalink / raw)
  To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
	jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen,
	x86, hpa, rui.zhang, oleksandr
  Cc: eranian, gautham.shenoy, ravi.bangoria, linux-perf-users,
	linux-kernel, Dhananjay Ugwekar

Preparation for the addition of per-core RAPL energy counter support for
AMD CPUs. Post which, one cpu might be mapped to more than one rapl_pmu
(package/die one or per-core one), also makes sense to use the
get_rapl_pmu_idx macro which is anyway used to index into the
rapl_pmus->pmus[] array.

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

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index ce04dc5844c4..b997899233c3 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;
@@ -623,12 +616,16 @@ static const struct attribute_group *rapl_attr_update[] = {
 static void __init init_rapl_pmu(void)
 {
 	struct rapl_pmu *pmu;
-	int cpu;
+	int cpu, rapl_pmu_idx;
 
 	cpus_read_lock();
 
 	for_each_cpu(cpu, cpu_online_mask) {
-		pmu = cpu_to_rapl_pmu(cpu);
+		rapl_pmu_idx = get_rapl_pmu_idx(cpu);
+		if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
+			continue;
+
+		pmu = rapl_pmus->pmus[rapl_pmu_idx];
 		if (pmu)
 			continue;
 		pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
@@ -640,7 +637,7 @@ static void __init init_rapl_pmu(void)
 		pmu->timer_interval = ms_to_ktime(rapl_timer_ms);
 		rapl_hrtimer_init(pmu);
 
-		rapl_pmus->pmus[get_rapl_pmu_idx(cpu)] = pmu;
+		rapl_pmus->pmus[rapl_pmu_idx] = pmu;
 	}
 
 	cpus_read_unlock();
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v5 3/9] perf/x86/rapl: Rename rapl_pmu variables
  2024-09-13 15:21 [PATCH v5 0/9] Add per-core RAPL energy counter support for AMD CPUs Dhananjay Ugwekar
  2024-09-13 15:21 ` [PATCH v5 1/9] x86/topology: Introduce topology_logical_core_id() Dhananjay Ugwekar
  2024-09-13 15:21 ` [PATCH v5 2/9] perf/x86/rapl: Remove the cpu_to_rapl_pmu() function Dhananjay Ugwekar
@ 2024-09-13 15:21 ` Dhananjay Ugwekar
  2024-09-13 15:47 ` [PATCH v5 4/9] perf/x86/rapl: Make rapl_model struct global Dhananjay Ugwekar
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: Dhananjay Ugwekar @ 2024-09-13 15:21 UTC (permalink / raw)
  To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
	jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen,
	x86, hpa, rui.zhang, oleksandr
  Cc: eranian, gautham.shenoy, ravi.bangoria, linux-perf-users,
	linux-kernel, 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 | 93 +++++++++++++++++++++---------------------
 1 file changed, 47 insertions(+), 46 deletions(-)

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index b997899233c3..a2f8c77fe629 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -116,7 +116,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,7 +616,7 @@ static const struct attribute_group *rapl_attr_update[] = {
 
 static void __init init_rapl_pmu(void)
 {
-	struct rapl_pmu *pmu;
+	struct rapl_pmu *rapl_pmu;
 	int cpu, rapl_pmu_idx;
 
 	cpus_read_lock();
@@ -625,19 +626,19 @@ static void __init init_rapl_pmu(void)
 		if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
 			continue;
 
-		pmu = rapl_pmus->pmus[rapl_pmu_idx];
-		if (pmu)
+		rapl_pmu = rapl_pmus->rapl_pmu[rapl_pmu_idx];
+		if (rapl_pmu)
 			continue;
-		pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
-		if (!pmu)
+		rapl_pmu = kzalloc_node(sizeof(*rapl_pmu), GFP_KERNEL, cpu_to_node(cpu));
+		if (!rapl_pmu)
 			continue;
-		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[rapl_pmu_idx] = pmu;
+		rapl_pmus->rapl_pmu[rapl_pmu_idx] = rapl_pmu;
 	}
 
 	cpus_read_unlock();
@@ -653,7 +654,7 @@ static int __init init_rapl_pmus(void)
 		rapl_pmu_scope		= PERF_PMU_SCOPE_PKG;
 	}
 
-	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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v5 4/9] perf/x86/rapl: Make rapl_model struct global
  2024-09-13 15:21 [PATCH v5 0/9] Add per-core RAPL energy counter support for AMD CPUs Dhananjay Ugwekar
                   ` (2 preceding siblings ...)
  2024-09-13 15:21 ` [PATCH v5 3/9] perf/x86/rapl: Rename rapl_pmu variables Dhananjay Ugwekar
@ 2024-09-13 15:47 ` Dhananjay Ugwekar
  2024-09-13 15:47   ` [PATCH v5 5/9] perf/x86/rapl: Add arguments to the cleanup and init functions Dhananjay Ugwekar
                     ` (4 more replies)
  2024-09-13 21:18 ` [PATCH v5 0/9] Add per-core RAPL " Oleksandr Natalenko
  2024-10-08  6:08 ` Zhang, Rui
  5 siblings, 5 replies; 18+ messages in thread
From: Dhananjay Ugwekar @ 2024-09-13 15:47 UTC (permalink / raw)
  To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
	jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen,
	x86, hpa, rui.zhang, oleksandr
  Cc: eranian, gautham.shenoy, ravi.bangoria, linux-perf-users,
	linux-kernel, Dhananjay Ugwekar

Preparation for per-core energy counter support addition for AMD CPUs.

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 a2f8c77fe629..46a6aeb61541 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -138,6 +138,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;
 
 /*
  * RAPL Package energy counter scope:
@@ -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
@@ -798,21 +799,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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v5 5/9] perf/x86/rapl: Add arguments to the cleanup and init functions
  2024-09-13 15:47 ` [PATCH v5 4/9] perf/x86/rapl: Make rapl_model struct global Dhananjay Ugwekar
@ 2024-09-13 15:47   ` Dhananjay Ugwekar
  2024-09-13 15:47   ` [PATCH v5 6/9] perf/x86/rapl: Modify the generic variable names to *_pkg* Dhananjay Ugwekar
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 18+ messages in thread
From: Dhananjay Ugwekar @ 2024-09-13 15:47 UTC (permalink / raw)
  To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
	jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen,
	x86, hpa, rui.zhang, oleksandr
  Cc: eranian, gautham.shenoy, ravi.bangoria, linux-perf-users,
	linux-kernel, Dhananjay Ugwekar

Prep for per-core RAPL PMU addition.

No functional change.

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

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index 46a6aeb61541..3bb1da8f96c3 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 void __init init_rapl_pmu(void)
+static void __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
 {
 	struct rapl_pmu *rapl_pmu;
 	int cpu, rapl_pmu_idx;
@@ -645,20 +645,22 @@ static void __init init_rapl_pmu(void)
 	cpus_read_unlock();
 }
 
-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() * topology_max_dies_per_package();
-	int rapl_pmu_scope = PERF_PMU_SCOPE_DIE;
+	int nr_rapl_pmu;
+	struct rapl_pmus *rapl_pmus;
 
-	if (rapl_pmu_is_pkg_scope()) {
-		nr_rapl_pmu		= topology_max_packages();
-		rapl_pmu_scope		= PERF_PMU_SCOPE_PKG;
-	}
+	if (rapl_pmu_scope == PERF_PMU_SCOPE_PKG)
+		nr_rapl_pmu	= topology_max_packages();
+	else
+		nr_rapl_pmu	= topology_max_packages() * 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;
@@ -673,7 +675,7 @@ static int __init init_rapl_pmus(void)
 	rapl_pmus->pmu.module		= THIS_MODULE;
 	rapl_pmus->pmu.capabilities	= PERF_PMU_CAP_NO_EXCLUDE;
 
-	init_rapl_pmu();
+	init_rapl_pmu(rapl_pmus);
 
 	return 0;
 }
@@ -799,8 +801,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;
@@ -816,7 +822,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;
 
@@ -829,7 +835,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);
@@ -837,6 +843,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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v5 6/9] perf/x86/rapl: Modify the generic variable names to *_pkg*
  2024-09-13 15:47 ` [PATCH v5 4/9] perf/x86/rapl: Make rapl_model struct global Dhananjay Ugwekar
  2024-09-13 15:47   ` [PATCH v5 5/9] perf/x86/rapl: Add arguments to the cleanup and init functions Dhananjay Ugwekar
@ 2024-09-13 15:47   ` Dhananjay Ugwekar
  2024-09-13 15:47   ` [PATCH v5 7/9] perf/x86/rapl: Remove the global variable rapl_msrs Dhananjay Ugwekar
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 18+ messages in thread
From: Dhananjay Ugwekar @ 2024-09-13 15:47 UTC (permalink / raw)
  To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
	jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen,
	x86, hpa, rui.zhang, oleksandr
  Cc: eranian, gautham.shenoy, ravi.bangoria, linux-perf-users,
	linux-kernel, Dhananjay Ugwekar

Prep for addition of power_per_core PMU to handle core scope energy
consumption for AMD CPUs.

Replace the generic names with *_pkg*, to 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 3bb1da8f96c3..023d208966bc 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",
@@ -126,16 +126,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;
@@ -149,7 +149,7 @@ static struct rapl_model *rapl_model;
  *	     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)
 
@@ -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]);
 		}
 	}
 }
@@ -681,71 +681,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 = {
@@ -801,11 +801,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)
@@ -813,20 +813,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;
 
@@ -835,14 +835,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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v5 7/9] perf/x86/rapl: Remove the global variable rapl_msrs
  2024-09-13 15:47 ` [PATCH v5 4/9] perf/x86/rapl: Make rapl_model struct global Dhananjay Ugwekar
  2024-09-13 15:47   ` [PATCH v5 5/9] perf/x86/rapl: Add arguments to the cleanup and init functions Dhananjay Ugwekar
  2024-09-13 15:47   ` [PATCH v5 6/9] perf/x86/rapl: Modify the generic variable names to *_pkg* Dhananjay Ugwekar
@ 2024-09-13 15:47   ` Dhananjay Ugwekar
  2024-09-13 15:48   ` [PATCH v5 8/9] perf/x86/rapl: Move the cntr_mask to rapl_pmus struct Dhananjay Ugwekar
  2024-09-13 15:48   ` [PATCH v5 9/9] perf/x86/rapl: Add per-core energy counter support for AMD CPUs Dhananjay Ugwekar
  4 siblings, 0 replies; 18+ messages in thread
From: Dhananjay Ugwekar @ 2024-09-13 15:47 UTC (permalink / raw)
  To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
	jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen,
	x86, hpa, rui.zhang, oleksandr
  Cc: eranian, gautham.shenoy, ravi.bangoria, linux-perf-users,
	linux-kernel, Dhananjay Ugwekar

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

Also it will be cleaner when new per-core scope PMU is added. As we will
need to maintain two rapl_msrs array(one for per-core scope and one for
package scope PMU), inside the rapl_model struct.

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 023d208966bc..bfd8bbcc1421 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -137,7 +137,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;
 
@@ -813,9 +812,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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v5 8/9] perf/x86/rapl: Move the cntr_mask to rapl_pmus struct
  2024-09-13 15:47 ` [PATCH v5 4/9] perf/x86/rapl: Make rapl_model struct global Dhananjay Ugwekar
                     ` (2 preceding siblings ...)
  2024-09-13 15:47   ` [PATCH v5 7/9] perf/x86/rapl: Remove the global variable rapl_msrs Dhananjay Ugwekar
@ 2024-09-13 15:48   ` Dhananjay Ugwekar
  2024-09-13 15:48   ` [PATCH v5 9/9] perf/x86/rapl: Add per-core energy counter support for AMD CPUs Dhananjay Ugwekar
  4 siblings, 0 replies; 18+ messages in thread
From: Dhananjay Ugwekar @ 2024-09-13 15:48 UTC (permalink / raw)
  To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
	jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen,
	x86, hpa, rui.zhang, oleksandr
  Cc: eranian, gautham.shenoy, ravi.bangoria, linux-perf-users,
	linux-kernel, Dhananjay Ugwekar

Preparation for the addition of per-core RAPL energy counter for AMD
CPUs.

Moving cntr_mask to rapl_pmus struct instead of adding a new global
cntr_mask for the per-core RAPL energy counter, will ensure that the
"per_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 bfd8bbcc1421..7e9cb01f749a 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -116,6 +116,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);
 };
 
@@ -135,7 +136,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]);
 		}
@@ -812,9 +812,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;
@@ -823,6 +820,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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v5 9/9] perf/x86/rapl: Add per-core energy counter support for AMD CPUs
  2024-09-13 15:47 ` [PATCH v5 4/9] perf/x86/rapl: Make rapl_model struct global Dhananjay Ugwekar
                     ` (3 preceding siblings ...)
  2024-09-13 15:48   ` [PATCH v5 8/9] perf/x86/rapl: Move the cntr_mask to rapl_pmus struct Dhananjay Ugwekar
@ 2024-09-13 15:48   ` Dhananjay Ugwekar
  2024-10-07  6:41     ` Gautham R. Shenoy
  4 siblings, 1 reply; 18+ messages in thread
From: Dhananjay Ugwekar @ 2024-09-13 15:48 UTC (permalink / raw)
  To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
	jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen,
	x86, hpa, rui.zhang, oleksandr
  Cc: eranian, gautham.shenoy, ravi.bangoria, linux-perf-users,
	linux-kernel, Dhananjay Ugwekar

Add a new "power_per_core" PMU and "energy-per-core" event for
monitoring energy consumption by each core. The existing energy-cores
event aggregates the energy consumption 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 1 in core 1 on a AMD Zen4 Genoa
machine:

$ perf stat -a --per-core -e power_per_core/energy-per-core/ sleep 1

 Performance counter stats for 'system wide':

S0-D0-C0         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C1         1          5.72 Joules power_per_core/energy-per-core/
S0-D0-C2         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C3         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C4         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C5         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C6         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C7         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C8         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C9         1          0.02 Joules power_per_core/energy-per-core/
S0-D0-C10        1          0.02 Joules power_per_core/energy-per-core/

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
v5 changes:
* Pass an extra "scope" argument to the get_rapl_pmu_idx() to
  support the per-core PMU as well.
* Pass "perf_event" pointer argument to rapl_scale(), to use the 
  corresponding hw_unit value (pkg or core) to scale.
* Move around the code in rapl_pmu_event_init() to consolidate the PMU
  specific code to single if block.
* Removed the sanity check in rapl_scale() as we have already
  bound-checked "cfg" in rapl_pmu_event_init()
---
 arch/x86/events/rapl.c | 178 +++++++++++++++++++++++++++++++++--------
 1 file changed, 143 insertions(+), 35 deletions(-)

diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index 7e9cb01f749a..6514ea67cf0c 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -39,6 +39,10 @@
  *	  event: rapl_energy_psys
  *    perf code: 0x5
  *
+ *  per_core counter: consumption of a single physical core
+ *	  event: rapl_energy_per_core (power_per_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_PER_CORE		0		/* per-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 = "per-core";
+
 /*
  * event code: LSB 8 bits, passed in attr->config
  * any other bit is reserved
@@ -128,14 +138,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,14 @@ 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);
+	if (scope == PERF_PMU_SCOPE_PKG)
+		return topology_logical_package_id(cpu);
+	else if (scope == PERF_PMU_SCOPE_DIE)
+		return topology_logical_die_id(cpu);
+	else
+		return topology_logical_core_id(cpu);
 }
 
 static inline u64 rapl_read_counter(struct perf_event *event)
@@ -169,19 +187,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 +227,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 +356,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 +371,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 per-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 +430,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-per-core,   rapl_per_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-per-core.unit,   rapl_per_core_unit, "Joules");
 
 /*
  * we compute in 0.23 nJ increments regardless of MSR
@@ -407,6 +447,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-per-core.scale,   rapl_per_core_scale, "2.3283064365386962890625e-10");
 
 /*
  * There are no default events, but we need to create
@@ -439,6 +480,12 @@ static const struct attribute_group *rapl_attr_groups[] = {
 	NULL,
 };
 
+static const struct attribute_group *rapl_per_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 +546,18 @@ static struct attribute_group rapl_events_psys_group = {
 	.attrs = rapl_events_psys,
 };
 
+static struct attribute *rapl_events_per_core[] = {
+	EVENT_PTR(rapl_per_core),
+	EVENT_PTR(rapl_per_core_unit),
+	EVENT_PTR(rapl_per_core_scale),
+	NULL,
+};
+
+static struct attribute_group rapl_events_per_core_group = {
+	.name  = "events",
+	.attrs = rapl_events_per_core,
+};
+
 static bool test_msr(int idx, void *data)
 {
 	return test_bit(idx, (unsigned long *) data);
@@ -536,6 +595,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_PER_CORE] = { MSR_AMD_CORE_ENERGY_STATUS, &rapl_events_per_core_group,
+				 test_msr, false, RAPL_MSR_MASK },
+};
+
 static int rapl_check_hw_unit(void)
 {
 	u64 msr_rapl_power_unit_bits;
@@ -547,6 +611,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 +631,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 +649,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 +663,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_PER_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 +687,10 @@ static const struct attribute_group *rapl_attr_update[] = {
 	NULL,
 };
 
+static const struct attribute_group *rapl_per_core_attr_update[] = {
+	&rapl_events_per_core_group,
+};
+
 static void __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
 {
 	struct rapl_pmu *rapl_pmu;
@@ -622,10 +699,9 @@ static void __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
 	cpus_read_lock();
 
 	for_each_cpu(cpu, cpu_online_mask) {
-		rapl_pmu_idx = get_rapl_pmu_idx(cpu);
+		rapl_pmu_idx = get_rapl_pmu_idx(cpu, rapl_pmus->pmu.scope);
 		if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
 			continue;
-
 		rapl_pmu = rapl_pmus->rapl_pmu[rapl_pmu_idx];
 		if (rapl_pmu)
 			continue;
@@ -644,15 +720,19 @@ static void __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
 	cpus_read_unlock();
 }
 
-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;
 	struct rapl_pmus *rapl_pmus;
 
 	if (rapl_pmu_scope == PERF_PMU_SCOPE_PKG)
 		nr_rapl_pmu	= topology_max_packages();
-	else
+	else if (rapl_pmu_scope == PERF_PMU_SCOPE_DIE)
 		nr_rapl_pmu	= topology_max_packages() * topology_max_dies_per_package();
+	else
+		nr_rapl_pmu	= topology_max_packages() * topology_num_cores_per_package();
 
 	rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu, nr_rapl_pmu), GFP_KERNEL);
 	if (!rapl_pmus)
@@ -743,8 +823,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_PER_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 = {
@@ -816,7 +898,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;
 
@@ -828,6 +911,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_per_core_attr_groups,
+				     rapl_per_core_attr_update);
+		if (ret) {
+			pr_warn("Per-core PMU initialization failed (%d)\n", ret);
+			goto per_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_per_core", -1);
+		if (ret) {
+			pr_warn("Per-core PMU registration failed (%d)\n", ret);
+			cleanup_rapl_pmus(rapl_pmus_core);
+		}
+	}
+
+per_core_init_failed:
 	rapl_advertise();
 	return 0;
 
@@ -840,6 +944,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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v5 0/9] Add per-core RAPL energy counter support for AMD CPUs
  2024-09-13 15:21 [PATCH v5 0/9] Add per-core RAPL energy counter support for AMD CPUs Dhananjay Ugwekar
                   ` (3 preceding siblings ...)
  2024-09-13 15:47 ` [PATCH v5 4/9] perf/x86/rapl: Make rapl_model struct global Dhananjay Ugwekar
@ 2024-09-13 21:18 ` Oleksandr Natalenko
  2024-09-16  4:18   ` Dhananjay Ugwekar
  2024-10-08  6:08 ` Zhang, Rui
  5 siblings, 1 reply; 18+ messages in thread
From: Oleksandr Natalenko @ 2024-09-13 21:18 UTC (permalink / raw)
  To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
	jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen,
	x86, hpa, rui.zhang, Dhananjay Ugwekar
  Cc: eranian, gautham.shenoy, ravi.bangoria, linux-perf-users,
	linux-kernel, Dhananjay Ugwekar

[-- Attachment #1: Type: text/plain, Size: 5756 bytes --]

Hello.

On pátek 13. září 2024 17:21:40, SELČ Dhananjay Ugwekar wrote:
> 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_per_core" PMU alongside the existing
> "power" PMU, which will be responsible for collecting the new
> "energy-per-core" event.
> 
> Tested the package level and core level PMU counters with workloads
> pinned to different CPUs.
> 
> Results with workload pinned to CPU 1 in Core 1 on an AMD Zen4 Genoa 
> machine:
> 
> $ perf stat -a --per-core -e power_per_core/energy-per-core/ -- sleep 1
> 
>  Performance counter stats for 'system wide':
> 
> S0-D0-C0         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C1         1          5.72 Joules power_per_core/energy-per-core/
> S0-D0-C2         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C3         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C4         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C5         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C6         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C7         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C8         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C9         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C10        1          0.02 Joules power_per_core/energy-per-core/
> 
> v4 Link: https://lore.kernel.org/all/20240711102436.4432-1-Dhananjay.Ugwekar@amd.com/
> 
> v5 changes:
> * Rebase on top of Kan Liang's "PMU scope" patchset [2]
> * rapl_cntr_mask moved to rapl_pmus struct in patch 8
> * Patch 1 from v4 is merged separately, so removed from this series
> * Add an extra argument "scope" in patch 5 to the init functions
> * Add an new patch 2, which removes the cpu_to_rapl_pmu() function
> 
> Base: tip/perf/core(currently has just 1-5 patches from [2]) + patch 6 from [2] + 
>       diff [3] + patch 7 from [2] + revert [4] + apply [5] 
> 
> [1]: https://lore.kernel.org/lkml/3e766f0e-37d4-0f82-3868-31b14228868d@linux.intel.com/
> [2]: https://lore.kernel.org/all/20240802151643.1691631-1-kan.liang@linux.intel.com/
> [3]: https://lore.kernel.org/all/8c09633c-5bf2-48a2-91a6-a0af9b9f2e8c@linux.intel.com/
> [4]: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=perf/core&id=8d72eba1cf8cecd76a2b4c1dd7673c2dc775f514
> [5]: https://lore.kernel.org/all/20240910085504.204814-1-Dhananjay.Ugwekar@amd.com/
> 
> Dhananjay Ugwekar (8):
>   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 cleanup and init 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 per-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                | 406 ++++++++++++++++----------
>  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, 266 insertions(+), 148 deletions(-)

With v6.11-rc7 + all the mentioned preparatory patches and this series:

$ taskset -c 9 dd if=/dev/zero of=/dev/null &

$ sudo perf stat -a --per-core -e power_per_core/energy-per-core/ sleep 5

 Performance counter stats for 'system wide':

S0-D0-C0              1               3,79 Joules power_per_core/energy-per-core/
S0-D0-C1              1               5,65 Joules power_per_core/energy-per-core/
S0-D0-C2              1               1,26 Joules power_per_core/energy-per-core/
S0-D0-C3              1               3,18 Joules power_per_core/energy-per-core/
S0-D0-C4              1               2,06 Joules power_per_core/energy-per-core/
S0-D0-C5              1               3,51 Joules power_per_core/energy-per-core/
S0-D0-C6              1               0,77 Joules power_per_core/energy-per-core/
S0-D0-C7              1               0,55 Joules power_per_core/energy-per-core/
S0-D0-C8              1               1,65 Joules power_per_core/energy-per-core/
S0-D0-C9              1              47,85 Joules power_per_core/energy-per-core/
S0-D0-C10             1               2,49 Joules power_per_core/energy-per-core/
S0-D0-C11             1              11,85 Joules power_per_core/energy-per-core/
S0-D0-C12             1               1,75 Joules power_per_core/energy-per-core/
S0-D0-C13             1               0,74 Joules power_per_core/energy-per-core/
S0-D0-C14             1               2,58 Joules power_per_core/energy-per-core/
S0-D0-C15             1               4,67 Joules power_per_core/energy-per-core/

       5,003391425 seconds time elapsed

on the following CPU:

AMD Ryzen 9 5950X 16-Core Processor

If this behaves as expected, please add:

Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>

Thank you.

-- 
Oleksandr Natalenko (post-factum)

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v5 0/9] Add per-core RAPL energy counter support for AMD CPUs
  2024-09-13 21:18 ` [PATCH v5 0/9] Add per-core RAPL " Oleksandr Natalenko
@ 2024-09-16  4:18   ` Dhananjay Ugwekar
  2024-09-16  6:49     ` Oleksandr Natalenko
  0 siblings, 1 reply; 18+ messages in thread
From: Dhananjay Ugwekar @ 2024-09-16  4:18 UTC (permalink / raw)
  To: Oleksandr Natalenko, peterz, mingo, acme, namhyung, mark.rutland,
	alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang,
	tglx, bp, dave.hansen, x86, hpa, rui.zhang
  Cc: eranian, gautham.shenoy, ravi.bangoria, linux-perf-users,
	linux-kernel

Hello Oleksandr,

On 9/14/2024 2:48 AM, Oleksandr Natalenko wrote:
> Hello.
> 
> On pátek 13. září 2024 17:21:40, SELČ Dhananjay Ugwekar wrote:
>> 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_per_core" PMU alongside the existing
>> "power" PMU, which will be responsible for collecting the new
>> "energy-per-core" event.
>>
>> Tested the package level and core level PMU counters with workloads
>> pinned to different CPUs.
>>
>> Results with workload pinned to CPU 1 in Core 1 on an AMD Zen4 Genoa 
>> machine:
>>
>> $ perf stat -a --per-core -e power_per_core/energy-per-core/ -- sleep 1
>>
>>  Performance counter stats for 'system wide':
>>
>> S0-D0-C0         1          0.02 Joules power_per_core/energy-per-core/
>> S0-D0-C1         1          5.72 Joules power_per_core/energy-per-core/
>> S0-D0-C2         1          0.02 Joules power_per_core/energy-per-core/
>> S0-D0-C3         1          0.02 Joules power_per_core/energy-per-core/
>> S0-D0-C4         1          0.02 Joules power_per_core/energy-per-core/
>> S0-D0-C5         1          0.02 Joules power_per_core/energy-per-core/
>> S0-D0-C6         1          0.02 Joules power_per_core/energy-per-core/
>> S0-D0-C7         1          0.02 Joules power_per_core/energy-per-core/
>> S0-D0-C8         1          0.02 Joules power_per_core/energy-per-core/
>> S0-D0-C9         1          0.02 Joules power_per_core/energy-per-core/
>> S0-D0-C10        1          0.02 Joules power_per_core/energy-per-core/
>>
>> v4 Link: https://lore.kernel.org/all/20240711102436.4432-1-Dhananjay.Ugwekar@amd.com/
>>
>> v5 changes:
>> * Rebase on top of Kan Liang's "PMU scope" patchset [2]
>> * rapl_cntr_mask moved to rapl_pmus struct in patch 8
>> * Patch 1 from v4 is merged separately, so removed from this series
>> * Add an extra argument "scope" in patch 5 to the init functions
>> * Add an new patch 2, which removes the cpu_to_rapl_pmu() function
>>
>> Base: tip/perf/core(currently has just 1-5 patches from [2]) + patch 6 from [2] + 
>>       diff [3] + patch 7 from [2] + revert [4] + apply [5] 
>>
>> [1]: https://lore.kernel.org/lkml/3e766f0e-37d4-0f82-3868-31b14228868d@linux.intel.com/
>> [2]: https://lore.kernel.org/all/20240802151643.1691631-1-kan.liang@linux.intel.com/
>> [3]: https://lore.kernel.org/all/8c09633c-5bf2-48a2-91a6-a0af9b9f2e8c@linux.intel.com/
>> [4]: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=perf/core&id=8d72eba1cf8cecd76a2b4c1dd7673c2dc775f514
>> [5]: https://lore.kernel.org/all/20240910085504.204814-1-Dhananjay.Ugwekar@amd.com/
>>
>> Dhananjay Ugwekar (8):
>>   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 cleanup and init 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 per-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                | 406 ++++++++++++++++----------
>>  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, 266 insertions(+), 148 deletions(-)
> 
> With v6.11-rc7 + all the mentioned preparatory patches and this series:
> 
> $ taskset -c 9 dd if=/dev/zero of=/dev/null &
> 
> $ sudo perf stat -a --per-core -e power_per_core/energy-per-core/ sleep 5
> 
>  Performance counter stats for 'system wide':
> 
> S0-D0-C0              1               3,79 Joules power_per_core/energy-per-core/
> S0-D0-C1              1               5,65 Joules power_per_core/energy-per-core/
> S0-D0-C2              1               1,26 Joules power_per_core/energy-per-core/
> S0-D0-C3              1               3,18 Joules power_per_core/energy-per-core/
> S0-D0-C4              1               2,06 Joules power_per_core/energy-per-core/
> S0-D0-C5              1               3,51 Joules power_per_core/energy-per-core/
> S0-D0-C6              1               0,77 Joules power_per_core/energy-per-core/
> S0-D0-C7              1               0,55 Joules power_per_core/energy-per-core/
> S0-D0-C8              1               1,65 Joules power_per_core/energy-per-core/
> S0-D0-C9              1              47,85 Joules power_per_core/energy-per-core/
> S0-D0-C10             1               2,49 Joules power_per_core/energy-per-core/
> S0-D0-C11             1              11,85 Joules power_per_core/energy-per-core/
> S0-D0-C12             1               1,75 Joules power_per_core/energy-per-core/
> S0-D0-C13             1               0,74 Joules power_per_core/energy-per-core/
> S0-D0-C14             1               2,58 Joules power_per_core/energy-per-core/
> S0-D0-C15             1               4,67 Joules power_per_core/energy-per-core/
> 
>        5,003391425 seconds time elapsed
> 
> on the following CPU:
> 
> AMD Ryzen 9 5950X 16-Core Processor
> 
> If this behaves as expected, please add:
> 
> Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>

Energy reported by core 9 is considerably higher than other cores, which is as expected.

However, can you please post the core_id for CPU 9 just to be sure, also I see that other 
cores are also showing considerable energy consumption(e.g. core 11), are there some other 
tasks running in the background?

Thanks for testing!

Regards,
Dhananjay

> 
> Thank you.
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v5 0/9] Add per-core RAPL energy counter support for AMD CPUs
  2024-09-16  4:18   ` Dhananjay Ugwekar
@ 2024-09-16  6:49     ` Oleksandr Natalenko
  2024-09-16  8:01       ` Dhananjay Ugwekar
  0 siblings, 1 reply; 18+ messages in thread
From: Oleksandr Natalenko @ 2024-09-16  6:49 UTC (permalink / raw)
  To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
	jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen,
	x86, hpa, rui.zhang, Dhananjay Ugwekar
  Cc: eranian, gautham.shenoy, ravi.bangoria, linux-perf-users,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 8149 bytes --]

Hello.

On pondělí 16. září 2024 6:18:13, SELČ Dhananjay Ugwekar wrote:
> Hello Oleksandr,
> 
> On 9/14/2024 2:48 AM, Oleksandr Natalenko wrote:
> > Hello.
> > 
> > On pátek 13. září 2024 17:21:40, SELČ Dhananjay Ugwekar wrote:
> >> 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_per_core" PMU alongside the existing
> >> "power" PMU, which will be responsible for collecting the new
> >> "energy-per-core" event.
> >>
> >> Tested the package level and core level PMU counters with workloads
> >> pinned to different CPUs.
> >>
> >> Results with workload pinned to CPU 1 in Core 1 on an AMD Zen4 Genoa 
> >> machine:
> >>
> >> $ perf stat -a --per-core -e power_per_core/energy-per-core/ -- sleep 1
> >>
> >>  Performance counter stats for 'system wide':
> >>
> >> S0-D0-C0         1          0.02 Joules power_per_core/energy-per-core/
> >> S0-D0-C1         1          5.72 Joules power_per_core/energy-per-core/
> >> S0-D0-C2         1          0.02 Joules power_per_core/energy-per-core/
> >> S0-D0-C3         1          0.02 Joules power_per_core/energy-per-core/
> >> S0-D0-C4         1          0.02 Joules power_per_core/energy-per-core/
> >> S0-D0-C5         1          0.02 Joules power_per_core/energy-per-core/
> >> S0-D0-C6         1          0.02 Joules power_per_core/energy-per-core/
> >> S0-D0-C7         1          0.02 Joules power_per_core/energy-per-core/
> >> S0-D0-C8         1          0.02 Joules power_per_core/energy-per-core/
> >> S0-D0-C9         1          0.02 Joules power_per_core/energy-per-core/
> >> S0-D0-C10        1          0.02 Joules power_per_core/energy-per-core/
> >>
> >> v4 Link: https://lore.kernel.org/all/20240711102436.4432-1-Dhananjay.Ugwekar@amd.com/
> >>
> >> v5 changes:
> >> * Rebase on top of Kan Liang's "PMU scope" patchset [2]
> >> * rapl_cntr_mask moved to rapl_pmus struct in patch 8
> >> * Patch 1 from v4 is merged separately, so removed from this series
> >> * Add an extra argument "scope" in patch 5 to the init functions
> >> * Add an new patch 2, which removes the cpu_to_rapl_pmu() function
> >>
> >> Base: tip/perf/core(currently has just 1-5 patches from [2]) + patch 6 from [2] + 
> >>       diff [3] + patch 7 from [2] + revert [4] + apply [5] 
> >>
> >> [1]: https://lore.kernel.org/lkml/3e766f0e-37d4-0f82-3868-31b14228868d@linux.intel.com/
> >> [2]: https://lore.kernel.org/all/20240802151643.1691631-1-kan.liang@linux.intel.com/
> >> [3]: https://lore.kernel.org/all/8c09633c-5bf2-48a2-91a6-a0af9b9f2e8c@linux.intel.com/
> >> [4]: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=perf/core&id=8d72eba1cf8cecd76a2b4c1dd7673c2dc775f514
> >> [5]: https://lore.kernel.org/all/20240910085504.204814-1-Dhananjay.Ugwekar@amd.com/
> >>
> >> Dhananjay Ugwekar (8):
> >>   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 cleanup and init 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 per-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                | 406 ++++++++++++++++----------
> >>  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, 266 insertions(+), 148 deletions(-)
> > 
> > With v6.11-rc7 + all the mentioned preparatory patches and this series:
> > 
> > $ taskset -c 9 dd if=/dev/zero of=/dev/null &
> > 
> > $ sudo perf stat -a --per-core -e power_per_core/energy-per-core/ sleep 5
> > 
> >  Performance counter stats for 'system wide':
> > 
> > S0-D0-C0              1               3,79 Joules power_per_core/energy-per-core/
> > S0-D0-C1              1               5,65 Joules power_per_core/energy-per-core/
> > S0-D0-C2              1               1,26 Joules power_per_core/energy-per-core/
> > S0-D0-C3              1               3,18 Joules power_per_core/energy-per-core/
> > S0-D0-C4              1               2,06 Joules power_per_core/energy-per-core/
> > S0-D0-C5              1               3,51 Joules power_per_core/energy-per-core/
> > S0-D0-C6              1               0,77 Joules power_per_core/energy-per-core/
> > S0-D0-C7              1               0,55 Joules power_per_core/energy-per-core/
> > S0-D0-C8              1               1,65 Joules power_per_core/energy-per-core/
> > S0-D0-C9              1              47,85 Joules power_per_core/energy-per-core/
> > S0-D0-C10             1               2,49 Joules power_per_core/energy-per-core/
> > S0-D0-C11             1              11,85 Joules power_per_core/energy-per-core/
> > S0-D0-C12             1               1,75 Joules power_per_core/energy-per-core/
> > S0-D0-C13             1               0,74 Joules power_per_core/energy-per-core/
> > S0-D0-C14             1               2,58 Joules power_per_core/energy-per-core/
> > S0-D0-C15             1               4,67 Joules power_per_core/energy-per-core/
> > 
> >        5,003391425 seconds time elapsed
> > 
> > on the following CPU:
> > 
> > AMD Ryzen 9 5950X 16-Core Processor
> > 
> > If this behaves as expected, please add:
> > 
> > Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
> 
> Energy reported by core 9 is considerably higher than other cores, which is as expected.
> 
> However, can you please post the core_id for CPU 9 just to be sure,

$ cat /sys/devices/system/cpu/cpu9/topology/core_id
9

> also I see that other 
> cores are also showing considerable energy consumption(e.g. core 11), are there some other 
> tasks running in the background?

Yes, this is a fully-featured desktop environment. Another run:

S0-D0-C0              1               7,68 Joules power_per_core/energy-per-core/
S0-D0-C1              1               2,05 Joules power_per_core/energy-per-core/
S0-D0-C2              1               0,96 Joules power_per_core/energy-per-core/
S0-D0-C3              1               5,16 Joules power_per_core/energy-per-core/
S0-D0-C4              1               1,11 Joules power_per_core/energy-per-core/
S0-D0-C5              1               2,42 Joules power_per_core/energy-per-core/
S0-D0-C6              1               1,11 Joules power_per_core/energy-per-core/
S0-D0-C7              1               0,51 Joules power_per_core/energy-per-core/
S0-D0-C8              1               1,61 Joules power_per_core/energy-per-core/
S0-D0-C9              1              47,07 Joules power_per_core/energy-per-core/
S0-D0-C10             1               1,59 Joules power_per_core/energy-per-core/
S0-D0-C11             1               3,02 Joules power_per_core/energy-per-core/
S0-D0-C12             1               1,43 Joules power_per_core/energy-per-core/
S0-D0-C13             1               0,84 Joules power_per_core/energy-per-core/
S0-D0-C14             1               0,73 Joules power_per_core/energy-per-core/
S0-D0-C15             1               0,78 Joules power_per_core/energy-per-core/

Thank you.

> 
> Thanks for testing!
> 
> Regards,
> Dhananjay
> 
> > 
> > Thank you.
> > 
> 


-- 
Oleksandr Natalenko (post-factum)

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v5 0/9] Add per-core RAPL energy counter support for AMD CPUs
  2024-09-16  6:49     ` Oleksandr Natalenko
@ 2024-09-16  8:01       ` Dhananjay Ugwekar
  0 siblings, 0 replies; 18+ messages in thread
From: Dhananjay Ugwekar @ 2024-09-16  8:01 UTC (permalink / raw)
  To: Oleksandr Natalenko, peterz, mingo, acme, namhyung, mark.rutland,
	alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang,
	tglx, bp, dave.hansen, x86, hpa, rui.zhang
  Cc: eranian, gautham.shenoy, ravi.bangoria, linux-perf-users,
	linux-kernel



On 9/16/2024 12:19 PM, Oleksandr Natalenko wrote:
> Hello.
> 
> On pondělí 16. září 2024 6:18:13, SELČ Dhananjay Ugwekar wrote:
>> Hello Oleksandr,
>>
>> On 9/14/2024 2:48 AM, Oleksandr Natalenko wrote:
>>> Hello.
>>>
>>> On pátek 13. září 2024 17:21:40, SELČ Dhananjay Ugwekar wrote:
>>>> 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_per_core" PMU alongside the existing
>>>> "power" PMU, which will be responsible for collecting the new
>>>> "energy-per-core" event.
>>>>
>>>> Tested the package level and core level PMU counters with workloads
>>>> pinned to different CPUs.
>>>>
>>>> Results with workload pinned to CPU 1 in Core 1 on an AMD Zen4 Genoa 
>>>> machine:
>>>>
>>>> $ perf stat -a --per-core -e power_per_core/energy-per-core/ -- sleep 1
>>>>
>>>>  Performance counter stats for 'system wide':
>>>>
>>>> S0-D0-C0         1          0.02 Joules power_per_core/energy-per-core/
>>>> S0-D0-C1         1          5.72 Joules power_per_core/energy-per-core/
>>>> S0-D0-C2         1          0.02 Joules power_per_core/energy-per-core/
>>>> S0-D0-C3         1          0.02 Joules power_per_core/energy-per-core/
>>>> S0-D0-C4         1          0.02 Joules power_per_core/energy-per-core/
>>>> S0-D0-C5         1          0.02 Joules power_per_core/energy-per-core/
>>>> S0-D0-C6         1          0.02 Joules power_per_core/energy-per-core/
>>>> S0-D0-C7         1          0.02 Joules power_per_core/energy-per-core/
>>>> S0-D0-C8         1          0.02 Joules power_per_core/energy-per-core/
>>>> S0-D0-C9         1          0.02 Joules power_per_core/energy-per-core/
>>>> S0-D0-C10        1          0.02 Joules power_per_core/energy-per-core/
>>>>
>>>> v4 Link: https://lore.kernel.org/all/20240711102436.4432-1-Dhananjay.Ugwekar@amd.com/
>>>>
>>>> v5 changes:
>>>> * Rebase on top of Kan Liang's "PMU scope" patchset [2]
>>>> * rapl_cntr_mask moved to rapl_pmus struct in patch 8
>>>> * Patch 1 from v4 is merged separately, so removed from this series
>>>> * Add an extra argument "scope" in patch 5 to the init functions
>>>> * Add an new patch 2, which removes the cpu_to_rapl_pmu() function
>>>>
>>>> Base: tip/perf/core(currently has just 1-5 patches from [2]) + patch 6 from [2] + 
>>>>       diff [3] + patch 7 from [2] + revert [4] + apply [5] 
>>>>
>>>> [1]: https://lore.kernel.org/lkml/3e766f0e-37d4-0f82-3868-31b14228868d@linux.intel.com/
>>>> [2]: https://lore.kernel.org/all/20240802151643.1691631-1-kan.liang@linux.intel.com/
>>>> [3]: https://lore.kernel.org/all/8c09633c-5bf2-48a2-91a6-a0af9b9f2e8c@linux.intel.com/
>>>> [4]: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=perf/core&id=8d72eba1cf8cecd76a2b4c1dd7673c2dc775f514
>>>> [5]: https://lore.kernel.org/all/20240910085504.204814-1-Dhananjay.Ugwekar@amd.com/
>>>>
>>>> Dhananjay Ugwekar (8):
>>>>   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 cleanup and init 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 per-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                | 406 ++++++++++++++++----------
>>>>  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, 266 insertions(+), 148 deletions(-)
>>>
>>> With v6.11-rc7 + all the mentioned preparatory patches and this series:
>>>
>>> $ taskset -c 9 dd if=/dev/zero of=/dev/null &
>>>
>>> $ sudo perf stat -a --per-core -e power_per_core/energy-per-core/ sleep 5
>>>
>>>  Performance counter stats for 'system wide':
>>>
>>> S0-D0-C0              1               3,79 Joules power_per_core/energy-per-core/
>>> S0-D0-C1              1               5,65 Joules power_per_core/energy-per-core/
>>> S0-D0-C2              1               1,26 Joules power_per_core/energy-per-core/
>>> S0-D0-C3              1               3,18 Joules power_per_core/energy-per-core/
>>> S0-D0-C4              1               2,06 Joules power_per_core/energy-per-core/
>>> S0-D0-C5              1               3,51 Joules power_per_core/energy-per-core/
>>> S0-D0-C6              1               0,77 Joules power_per_core/energy-per-core/
>>> S0-D0-C7              1               0,55 Joules power_per_core/energy-per-core/
>>> S0-D0-C8              1               1,65 Joules power_per_core/energy-per-core/
>>> S0-D0-C9              1              47,85 Joules power_per_core/energy-per-core/
>>> S0-D0-C10             1               2,49 Joules power_per_core/energy-per-core/
>>> S0-D0-C11             1              11,85 Joules power_per_core/energy-per-core/
>>> S0-D0-C12             1               1,75 Joules power_per_core/energy-per-core/
>>> S0-D0-C13             1               0,74 Joules power_per_core/energy-per-core/
>>> S0-D0-C14             1               2,58 Joules power_per_core/energy-per-core/
>>> S0-D0-C15             1               4,67 Joules power_per_core/energy-per-core/
>>>
>>>        5,003391425 seconds time elapsed
>>>
>>> on the following CPU:
>>>
>>> AMD Ryzen 9 5950X 16-Core Processor
>>>
>>> If this behaves as expected, please add:
>>>
>>> Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
>>
>> Energy reported by core 9 is considerably higher than other cores, which is as expected.
>>
>> However, can you please post the core_id for CPU 9 just to be sure,
> 
> $ cat /sys/devices/system/cpu/cpu9/topology/core_id
> 9
> 
>> also I see that other 
>> cores are also showing considerable energy consumption(e.g. core 11), are there some other 
>> tasks running in the background?
> 
> Yes, this is a fully-featured desktop environment. Another run:
> 
> S0-D0-C0              1               7,68 Joules power_per_core/energy-per-core/
> S0-D0-C1              1               2,05 Joules power_per_core/energy-per-core/
> S0-D0-C2              1               0,96 Joules power_per_core/energy-per-core/
> S0-D0-C3              1               5,16 Joules power_per_core/energy-per-core/
> S0-D0-C4              1               1,11 Joules power_per_core/energy-per-core/
> S0-D0-C5              1               2,42 Joules power_per_core/energy-per-core/
> S0-D0-C6              1               1,11 Joules power_per_core/energy-per-core/
> S0-D0-C7              1               0,51 Joules power_per_core/energy-per-core/
> S0-D0-C8              1               1,61 Joules power_per_core/energy-per-core/
> S0-D0-C9              1              47,07 Joules power_per_core/energy-per-core/
> S0-D0-C10             1               1,59 Joules power_per_core/energy-per-core/
> S0-D0-C11             1               3,02 Joules power_per_core/energy-per-core/
> S0-D0-C12             1               1,43 Joules power_per_core/energy-per-core/
> S0-D0-C13             1               0,84 Joules power_per_core/energy-per-core/
> S0-D0-C14             1               0,73 Joules power_per_core/energy-per-core/
> S0-D0-C15             1               0,78 Joules power_per_core/energy-per-core/

Great!, the results are as expected, will add your tested-by,

Thanks,
Dhananjay

> 
> Thank you.
> 
>>
>> Thanks for testing!
>>
>> Regards,
>> Dhananjay
>>
>>>
>>> Thank you.
>>>
>>
> 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v5 9/9] perf/x86/rapl: Add per-core energy counter support for AMD CPUs
  2024-09-13 15:48   ` [PATCH v5 9/9] perf/x86/rapl: Add per-core energy counter support for AMD CPUs Dhananjay Ugwekar
@ 2024-10-07  6:41     ` Gautham R. Shenoy
  2024-10-07  8:38       ` Dhananjay Ugwekar
  0 siblings, 1 reply; 18+ messages in thread
From: Gautham R. Shenoy @ 2024-10-07  6:41 UTC (permalink / raw)
  To: Dhananjay Ugwekar
  Cc: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
	jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen,
	x86, hpa, rui.zhang, oleksandr, eranian, ravi.bangoria,
	linux-perf-users, linux-kernel

On Fri, Sep 13, 2024 at 03:48:01PM +0000, Dhananjay Ugwekar wrote:
> Add a new "power_per_core" PMU and "energy-per-core" event for
> monitoring energy consumption by each core. The existing energy-cores
> event aggregates the energy consumption 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 1 in core 1 on a AMD Zen4 Genoa
> machine:
> 
> $ perf stat -a --per-core -e power_per_core/energy-per-core/ sleep 1
> 
>  Performance counter stats for 'system wide':
> 
> S0-D0-C0         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C1         1          5.72 Joules power_per_core/energy-per-core/
> S0-D0-C2         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C3         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C4         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C5         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C6         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C7         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C8         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C9         1          0.02 Joules power_per_core/energy-per-core/
> S0-D0-C10        1          0.02 Joules power_per_core/energy-per-core/
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> ---
> v5 changes:
> * Pass an extra "scope" argument to the get_rapl_pmu_idx() to
>   support the per-core PMU as well.
> * Pass "perf_event" pointer argument to rapl_scale(), to use the 
>   corresponding hw_unit value (pkg or core) to scale.
> * Move around the code in rapl_pmu_event_init() to consolidate the PMU
>   specific code to single if block.
> * Removed the sanity check in rapl_scale() as we have already
>   bound-checked "cfg" in rapl_pmu_event_init()
> ---
>  arch/x86/events/rapl.c | 178 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 143 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index 7e9cb01f749a..6514ea67cf0c 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -39,6 +39,10 @@
>   *	  event: rapl_energy_psys
>   *    perf code: 0x5
>   *
> + *  per_core counter: consumption of a single physical core
> + *	  event: rapl_energy_per_core (power_per_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_PER_CORE		0		/* per-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 = "per-core";
> +
>  /*
>   * event code: LSB 8 bits, passed in attr->config
>   * any other bit is reserved
> @@ -128,14 +138,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,14 @@ 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);
> +	if (scope == PERF_PMU_SCOPE_PKG)
> +		return topology_logical_package_id(cpu);
> +	else if (scope == PERF_PMU_SCOPE_DIE)

You don't need the "else if" since you are returning if there is a
match for the earlier if condition.

> +		return topology_logical_die_id(cpu);
> +	else
        ^^^^^
Please check if the scope is SCOPE_CORE here. Again, you don't need the
else condition.


> +		return topology_logical_core_id(cpu);



>  }
>  
>  static inline u64 rapl_read_counter(struct perf_event *event)
> @@ -169,19 +187,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 +227,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 +356,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;

Don't we need the check to only look at RAPL events of pkg or core ?
Or is that covered by a check below ?



> +	/* 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 +371,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 per-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 +430,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-per-core,   rapl_per_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-per-core.unit,   rapl_per_core_unit, "Joules");
>  
>  /*
>   * we compute in 0.23 nJ increments regardless of MSR
> @@ -407,6 +447,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-per-core.scale,   rapl_per_core_scale, "2.3283064365386962890625e-10");
>  
>  /*
>   * There are no default events, but we need to create
> @@ -439,6 +480,12 @@ static const struct attribute_group *rapl_attr_groups[] = {
>  	NULL,
>  };
>  
> +static const struct attribute_group *rapl_per_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 +546,18 @@ static struct attribute_group rapl_events_psys_group = {
>  	.attrs = rapl_events_psys,
>  };
>  
> +static struct attribute *rapl_events_per_core[] = {
> +	EVENT_PTR(rapl_per_core),
> +	EVENT_PTR(rapl_per_core_unit),
> +	EVENT_PTR(rapl_per_core_scale),
> +	NULL,
> +};
> +
> +static struct attribute_group rapl_events_per_core_group = {
> +	.name  = "events",
> +	.attrs = rapl_events_per_core,
> +};
> +
>  static bool test_msr(int idx, void *data)
>  {
>  	return test_bit(idx, (unsigned long *) data);
> @@ -536,6 +595,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_PER_CORE] = { MSR_AMD_CORE_ENERGY_STATUS, &rapl_events_per_core_group,
> +				 test_msr, false, RAPL_MSR_MASK },
> +};
> +
>  static int rapl_check_hw_unit(void)
>  {
>  	u64 msr_rapl_power_unit_bits;
> @@ -547,6 +611,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 +631,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 +649,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 +663,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_PER_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 +687,10 @@ static const struct attribute_group *rapl_attr_update[] = {
>  	NULL,
>  };
>  
> +static const struct attribute_group *rapl_per_core_attr_update[] = {
> +	&rapl_events_per_core_group,
> +};
> +
>  static void __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
>  {
>  	struct rapl_pmu *rapl_pmu;
> @@ -622,10 +699,9 @@ static void __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
>  	cpus_read_lock();
>  
>  	for_each_cpu(cpu, cpu_online_mask) {
> -		rapl_pmu_idx = get_rapl_pmu_idx(cpu);
> +		rapl_pmu_idx = get_rapl_pmu_idx(cpu, rapl_pmus->pmu.scope);
>  		if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
>  			continue;
> -
>  		rapl_pmu = rapl_pmus->rapl_pmu[rapl_pmu_idx];
>  		if (rapl_pmu)
>  			continue;
> @@ -644,15 +720,19 @@ static void __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
>  	cpus_read_unlock();
>  }
>  
> -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;
>  	struct rapl_pmus *rapl_pmus;
>  
>  	if (rapl_pmu_scope == PERF_PMU_SCOPE_PKG)
>  		nr_rapl_pmu	= topology_max_packages();
> -	else
> +	else if (rapl_pmu_scope == PERF_PMU_SCOPE_DIE)
>  		nr_rapl_pmu	= topology_max_packages() * topology_max_dies_per_package();
> +	else
> +		nr_rapl_pmu	= topology_max_packages() * topology_num_cores_per_package();

Here please check if the rapl_pmu_scope is PERF_PMU_SCOPE_CORE instead
of assuming it to be the case if the scope is neither SCOPE_PKG nor
SCOPE_DIE. If it is neither of these three, then return an error.


--
Thanks and Regards
gautham.

>  
>  	rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu, nr_rapl_pmu), GFP_KERNEL);
>  	if (!rapl_pmus)
> @@ -743,8 +823,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_PER_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 = {
> @@ -816,7 +898,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;
>  
> @@ -828,6 +911,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_per_core_attr_groups,
> +				     rapl_per_core_attr_update);
> +		if (ret) {
> +			pr_warn("Per-core PMU initialization failed (%d)\n", ret);
> +			goto per_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_per_core", -1);
> +		if (ret) {
> +			pr_warn("Per-core PMU registration failed (%d)\n", ret);
> +			cleanup_rapl_pmus(rapl_pmus_core);
> +		}
> +	}
> +
> +per_core_init_failed:
>  	rapl_advertise();
>  	return 0;
>  
> @@ -840,6 +944,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
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v5 9/9] perf/x86/rapl: Add per-core energy counter support for AMD CPUs
  2024-10-07  6:41     ` Gautham R. Shenoy
@ 2024-10-07  8:38       ` Dhananjay Ugwekar
  0 siblings, 0 replies; 18+ messages in thread
From: Dhananjay Ugwekar @ 2024-10-07  8:38 UTC (permalink / raw)
  To: Gautham R. Shenoy
  Cc: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
	jolsa, irogers, adrian.hunter, kan.liang, tglx, bp, dave.hansen,
	x86, hpa, rui.zhang, oleksandr, eranian, ravi.bangoria,
	linux-perf-users, linux-kernel

Hello Gautham,

On 10/7/2024 12:11 PM, Gautham R. Shenoy wrote:
> On Fri, Sep 13, 2024 at 03:48:01PM +0000, Dhananjay Ugwekar wrote:
>> Add a new "power_per_core" PMU and "energy-per-core" event for
>> monitoring energy consumption by each core. The existing energy-cores
>> event aggregates the energy consumption at the package level.
>> This new event aligns with the AMD's per_core energy counters.
[Snip]
>>  
>> @@ -156,10 +170,14 @@ 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);
>> +	if (scope == PERF_PMU_SCOPE_PKG)
>> +		return topology_logical_package_id(cpu);
>> +	else if (scope == PERF_PMU_SCOPE_DIE)
> 
> You don't need the "else if" since you are returning if there is a
> match for the earlier if condition.

Right, will modify accordingly

> 
>> +		return topology_logical_die_id(cpu);
>> +	else
>         ^^^^^
> Please check if the scope is SCOPE_CORE here. Again, you don't need the
> else condition.

Yes, will add the if check and remove the else

> 
> 
>> +		return topology_logical_core_id(cpu);
> 
> 
> 
>>  }
>>  
>>  static inline u64 rapl_read_counter(struct perf_event *event)
[Snip]
>> @@ -337,12 +356,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;
> 
> Don't we need the check to only look at RAPL events of pkg or core ?
> Or is that covered by a check below ?

I moved this check to the PMU specific if blocks (highlighted below)

> 
> 
> 
>> +	/* 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 +371,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; 		^^^^ here and
>> +
>> +		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 per-core events */
>> +		if (event->attr.type != rapl_pmus_core->pmu.type)
>> +			return -ENOENT;
		^^^^ here
>> +
>> +		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;
>>  
[Snip]
>> @@ -644,15 +720,19 @@ static void __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
>>  	cpus_read_unlock();
>>  }
>>  
>> -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;
>>  	struct rapl_pmus *rapl_pmus;
>>  
>>  	if (rapl_pmu_scope == PERF_PMU_SCOPE_PKG)
>>  		nr_rapl_pmu	= topology_max_packages();
>> -	else
>> +	else if (rapl_pmu_scope == PERF_PMU_SCOPE_DIE)
>>  		nr_rapl_pmu	= topology_max_packages() * topology_max_dies_per_package();
>> +	else
>> +		nr_rapl_pmu	= topology_max_packages() * topology_num_cores_per_package();
> 
> Here please check if the rapl_pmu_scope is PERF_PMU_SCOPE_CORE instead
> of assuming it to be the case if the scope is neither SCOPE_PKG nor
> SCOPE_DIE. If it is neither of these three, then return an error.

Actually, I thought there is only one caller of this function (rapl_pmu_init), 
which only passes one of these 3 values, so I thought maybe the error check isnt 
needed. What do you think? 

Thanks,
Dhananjay

> 
> 
> --
> Thanks and Regards
> gautham.
> 
>>  
>>  	rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu, nr_rapl_pmu), GFP_KERNEL);
>>  	if (!rapl_pmus)


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v5 0/9] Add per-core RAPL energy counter support for AMD CPUs
  2024-09-13 15:21 [PATCH v5 0/9] Add per-core RAPL energy counter support for AMD CPUs Dhananjay Ugwekar
                   ` (4 preceding siblings ...)
  2024-09-13 21:18 ` [PATCH v5 0/9] Add per-core RAPL " Oleksandr Natalenko
@ 2024-10-08  6:08 ` Zhang, Rui
  2024-10-08  6:43   ` Dhananjay Ugwekar
  5 siblings, 1 reply; 18+ messages in thread
From: Zhang, Rui @ 2024-10-08  6:08 UTC (permalink / raw)
  To: alexander.shishkin@linux.intel.com, dave.hansen@linux.intel.com,
	Hunter, Adrian, mingo@redhat.com, irogers@google.com,
	tglx@linutronix.de, kan.liang@linux.intel.com,
	Dhananjay.Ugwekar@amd.com, hpa@zytor.com, mark.rutland@arm.com,
	peterz@infradead.org, bp@alien8.de, acme@kernel.org,
	oleksandr@natalenko.name, jolsa@kernel.org, x86@kernel.org,
	namhyung@kernel.org
  Cc: linux-perf-users@vger.kernel.org, ravi.bangoria@amd.com,
	linux-kernel@vger.kernel.org, Eranian, Stephane,
	gautham.shenoy@amd.com

On Fri, 2024-09-13 at 15:21 +0000, Dhananjay Ugwekar wrote:
> 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_per_core" PMU alongside the existing
> "power" PMU, which will be responsible for collecting the new
> "energy-per-core" event.
> 
> Tested the package level and core level PMU counters with workloads
> pinned to different CPUs.
> 
> Results with workload pinned to CPU 1 in Core 1 on an AMD Zen4 Genoa 
> machine:
> 
> $ perf stat -a --per-core -e power_per_core/energy-per-core/ -- sleep
> 1
> 
>  Performance counter stats for 'system wide':
> 
> S0-D0-C0         1          0.02 Joules power_per_core/energy-per-
> core/
> S0-D0-C1         1          5.72 Joules power_per_core/energy-per-
> core/
> S0-D0-C2         1          0.02 Joules power_per_core/energy-per-
> core/
> S0-D0-C3         1          0.02 Joules power_per_core/energy-per-
> core/
> S0-D0-C4         1          0.02 Joules power_per_core/energy-per-
> core/
> S0-D0-C5         1          0.02 Joules power_per_core/energy-per-
> core/
> S0-D0-C6         1          0.02 Joules power_per_core/energy-per-
> core/
> S0-D0-C7         1          0.02 Joules power_per_core/energy-per-
> core/
> S0-D0-C8         1          0.02 Joules power_per_core/energy-per-
> core/
> S0-D0-C9         1          0.02 Joules power_per_core/energy-per-
> core/
> S0-D0-C10        1          0.02 Joules power_per_core/energy-per-
> core/
> 
> v4 Link:
> https://lore.kernel.org/all/20240711102436.4432-1-Dhananjay.Ugwekar@amd.com/
> 
> v5 changes:
> * Rebase on top of Kan Liang's "PMU scope" patchset [2]
> * rapl_cntr_mask moved to rapl_pmus struct in patch 8
> * Patch 1 from v4 is merged separately, so removed from this series
> * Add an extra argument "scope" in patch 5 to the init functions
> * Add an new patch 2, which removes the cpu_to_rapl_pmu() function
> 
> Base: tip/perf/core(currently has just 1-5 patches from [2]) + patch
> 6 from [2] + 
>       diff [3] + patch 7 from [2] + revert [4] + apply [5] 
> 

The patch LGTM. But they don't apply to latest upstream because
https://lore.kernel.org/all/20240910085504.204814-1-Dhananjay.Ugwekar@amd.com/
and the upstream version diverges.

will you rebase your patches so that I can give them a try on my Intel
testbox?

thanks,
rui

> [1]:
> https://lore.kernel.org/lkml/3e766f0e-37d4-0f82-3868-31b14228868d@linux.intel.com/
> [2]:
> https://lore.kernel.org/all/20240802151643.1691631-1-kan.liang@linux.intel.com/
> [3]:
> https://lore.kernel.org/all/8c09633c-5bf2-48a2-91a6-a0af9b9f2e8c@linux.intel.com/
> [4]:
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=perf/core&id=8d72eba1cf8cecd76a2b4c1dd7673c2dc775f514
> [5]:
> https://lore.kernel.org/all/20240910085504.204814-1-Dhananjay.Ugwekar@amd.com/
> 
> Dhananjay Ugwekar (8):
>   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 cleanup and init 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 per-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                | 406 ++++++++++++++++--------
> --
>  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, 266 insertions(+), 148 deletions(-)
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v5 0/9] Add per-core RAPL energy counter support for AMD CPUs
  2024-10-08  6:08 ` Zhang, Rui
@ 2024-10-08  6:43   ` Dhananjay Ugwekar
  0 siblings, 0 replies; 18+ messages in thread
From: Dhananjay Ugwekar @ 2024-10-08  6:43 UTC (permalink / raw)
  To: Zhang, Rui, alexander.shishkin@linux.intel.com,
	dave.hansen@linux.intel.com, Hunter, Adrian, mingo@redhat.com,
	irogers@google.com, tglx@linutronix.de, kan.liang@linux.intel.com,
	hpa@zytor.com, mark.rutland@arm.com, peterz@infradead.org,
	bp@alien8.de, acme@kernel.org, oleksandr@natalenko.name,
	jolsa@kernel.org, x86@kernel.org, namhyung@kernel.org
  Cc: linux-perf-users@vger.kernel.org, ravi.bangoria@amd.com,
	linux-kernel@vger.kernel.org, Eranian, Stephane,
	gautham.shenoy@amd.com

Hello Rui,

On 10/8/2024 11:38 AM, Zhang, Rui wrote:
> On Fri, 2024-09-13 at 15:21 +0000, Dhananjay Ugwekar wrote:
>> 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_per_core" PMU alongside the existing
>> "power" PMU, which will be responsible for collecting the new
>> "energy-per-core" event.
>>
>> Tested the package level and core level PMU counters with workloads
>> pinned to different CPUs.
>>
>> Results with workload pinned to CPU 1 in Core 1 on an AMD Zen4 Genoa 
>> machine:
>>
>> $ perf stat -a --per-core -e power_per_core/energy-per-core/ -- sleep
>> 1
>>
>>  Performance counter stats for 'system wide':
>>
>> S0-D0-C0         1          0.02 Joules power_per_core/energy-per-
>> core/
>> S0-D0-C1         1          5.72 Joules power_per_core/energy-per-
>> core/
>> S0-D0-C2         1          0.02 Joules power_per_core/energy-per-
>> core/
>> S0-D0-C3         1          0.02 Joules power_per_core/energy-per-
>> core/
>> S0-D0-C4         1          0.02 Joules power_per_core/energy-per-
>> core/
>> S0-D0-C5         1          0.02 Joules power_per_core/energy-per-
>> core/
>> S0-D0-C6         1          0.02 Joules power_per_core/energy-per-
>> core/
>> S0-D0-C7         1          0.02 Joules power_per_core/energy-per-
>> core/
>> S0-D0-C8         1          0.02 Joules power_per_core/energy-per-
>> core/
>> S0-D0-C9         1          0.02 Joules power_per_core/energy-per-
>> core/
>> S0-D0-C10        1          0.02 Joules power_per_core/energy-per-
>> core/
>>
>> v4 Link:
>> https://lore.kernel.org/all/20240711102436.4432-1-Dhananjay.Ugwekar@amd.com/
>>
>> v5 changes:
>> * Rebase on top of Kan Liang's "PMU scope" patchset [2]
>> * rapl_cntr_mask moved to rapl_pmus struct in patch 8
>> * Patch 1 from v4 is merged separately, so removed from this series
>> * Add an extra argument "scope" in patch 5 to the init functions
>> * Add an new patch 2, which removes the cpu_to_rapl_pmu() function
>>
>> Base: tip/perf/core(currently has just 1-5 patches from [2]) + patch
>> 6 from [2] + 
>>       diff [3] + patch 7 from [2] + revert [4] + apply [5] 
>>
> 
> The patch LGTM. But they don't apply to latest upstream because
> https://lore.kernel.org/all/20240910085504.204814-1-Dhananjay.Ugwekar@amd.com/
> and the upstream version diverges.
> 
> will you rebase your patches so that I can give them a try on my Intel
> testbox?

Thanks for taking a look. 
Yes, I will be rebasing on top of upstream and posting v6, mostly by monday.

Regards,
Dhananjay

> 
> thanks,
> rui
> 
>> [1]:
>> https://lore.kernel.org/lkml/3e766f0e-37d4-0f82-3868-31b14228868d@linux.intel.com/
>> [2]:
>> https://lore.kernel.org/all/20240802151643.1691631-1-kan.liang@linux.intel.com/
>> [3]:
>> https://lore.kernel.org/all/8c09633c-5bf2-48a2-91a6-a0af9b9f2e8c@linux.intel.com/
>> [4]:
>> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=perf/core&id=8d72eba1cf8cecd76a2b4c1dd7673c2dc775f514
>> [5]:
>> https://lore.kernel.org/all/20240910085504.204814-1-Dhananjay.Ugwekar@amd.com/
>>
>> Dhananjay Ugwekar (8):
>>   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 cleanup and init 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 per-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                | 406 ++++++++++++++++--------
>> --
>>  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, 266 insertions(+), 148 deletions(-)
>>
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2024-10-08  6:44 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-13 15:21 [PATCH v5 0/9] Add per-core RAPL energy counter support for AMD CPUs Dhananjay Ugwekar
2024-09-13 15:21 ` [PATCH v5 1/9] x86/topology: Introduce topology_logical_core_id() Dhananjay Ugwekar
2024-09-13 15:21 ` [PATCH v5 2/9] perf/x86/rapl: Remove the cpu_to_rapl_pmu() function Dhananjay Ugwekar
2024-09-13 15:21 ` [PATCH v5 3/9] perf/x86/rapl: Rename rapl_pmu variables Dhananjay Ugwekar
2024-09-13 15:47 ` [PATCH v5 4/9] perf/x86/rapl: Make rapl_model struct global Dhananjay Ugwekar
2024-09-13 15:47   ` [PATCH v5 5/9] perf/x86/rapl: Add arguments to the cleanup and init functions Dhananjay Ugwekar
2024-09-13 15:47   ` [PATCH v5 6/9] perf/x86/rapl: Modify the generic variable names to *_pkg* Dhananjay Ugwekar
2024-09-13 15:47   ` [PATCH v5 7/9] perf/x86/rapl: Remove the global variable rapl_msrs Dhananjay Ugwekar
2024-09-13 15:48   ` [PATCH v5 8/9] perf/x86/rapl: Move the cntr_mask to rapl_pmus struct Dhananjay Ugwekar
2024-09-13 15:48   ` [PATCH v5 9/9] perf/x86/rapl: Add per-core energy counter support for AMD CPUs Dhananjay Ugwekar
2024-10-07  6:41     ` Gautham R. Shenoy
2024-10-07  8:38       ` Dhananjay Ugwekar
2024-09-13 21:18 ` [PATCH v5 0/9] Add per-core RAPL " Oleksandr Natalenko
2024-09-16  4:18   ` Dhananjay Ugwekar
2024-09-16  6:49     ` Oleksandr Natalenko
2024-09-16  8:01       ` Dhananjay Ugwekar
2024-10-08  6:08 ` Zhang, Rui
2024-10-08  6:43   ` Dhananjay Ugwekar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).