| Message ID | 20240624055907.7720-6-Dhananjay.Ugwekar@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add per-core RAPL energy counter support for AMD CPUs | expand |
On Mon, Jun 24, 2024 at 05:59:02AM +0000, Dhananjay Ugwekar wrote: > This patch is in preparation for addition of per-core energy counter > support for AMD CPUs. > > Per-core energy counter PMU will need a separate cpumask. It seems like > a better approach to add the cpumask inside the rapl_pmus struct, instead > of creating another global cpumask variable for per-core PMU. This way, in > future, if there is a need for a new PMU with a different scope (e.g. CCD), > adding a new global cpumask variable won't be necessary. > > No functional change. > > Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> > --- > arch/x86/events/rapl.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c > index e5e878146542..be139e9f9ee0 100644 > --- a/arch/x86/events/rapl.c > +++ b/arch/x86/events/rapl.c > @@ -119,6 +119,7 @@ struct rapl_pmu { > > struct rapl_pmus { > struct pmu pmu; > + cpumask_t cpumask; > unsigned int nr_rapl_pmu; > struct rapl_pmu *rapl_pmu[] __counted_by(nr_rapl_pmu); > }; Yikes no, please use cpumask_var_t and alloc_cpumask_var() and friends.
Hello Peter, On 7/1/2024 6:32 PM, Peter Zijlstra wrote: > On Mon, Jun 24, 2024 at 05:59:02AM +0000, Dhananjay Ugwekar wrote: >> This patch is in preparation for addition of per-core energy counter >> support for AMD CPUs. >> >> Per-core energy counter PMU will need a separate cpumask. It seems like >> a better approach to add the cpumask inside the rapl_pmus struct, instead >> of creating another global cpumask variable for per-core PMU. This way, in >> future, if there is a need for a new PMU with a different scope (e.g. CCD), >> adding a new global cpumask variable won't be necessary. >> >> No functional change. >> >> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> >> --- >> arch/x86/events/rapl.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c >> index e5e878146542..be139e9f9ee0 100644 >> --- a/arch/x86/events/rapl.c >> +++ b/arch/x86/events/rapl.c >> @@ -119,6 +119,7 @@ struct rapl_pmu { >> >> struct rapl_pmus { >> struct pmu pmu; >> + cpumask_t cpumask; >> unsigned int nr_rapl_pmu; >> struct rapl_pmu *rapl_pmu[] __counted_by(nr_rapl_pmu); >> }; > > Yikes no, please use cpumask_var_t and alloc_cpumask_var() and friends. Ah yes!, I did not know about this API, will use this in the next version. Thanks, Dhananjay
diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c index e5e878146542..be139e9f9ee0 100644 --- a/arch/x86/events/rapl.c +++ b/arch/x86/events/rapl.c @@ -119,6 +119,7 @@ struct rapl_pmu { struct rapl_pmus { struct pmu pmu; + cpumask_t cpumask; unsigned int nr_rapl_pmu; struct rapl_pmu *rapl_pmu[] __counted_by(nr_rapl_pmu); }; @@ -139,7 +140,6 @@ struct rapl_model { /* 1/2^hw_unit Joule */ static int rapl_hw_unit[NR_RAPL_DOMAINS] __read_mostly; static struct rapl_pmus *rapl_pmus; -static cpumask_t rapl_cpu_mask; static unsigned int rapl_cntr_mask; static u64 rapl_timer_ms; static struct perf_msr *rapl_msrs; @@ -394,7 +394,7 @@ static void rapl_pmu_event_read(struct perf_event *event) static ssize_t rapl_get_attr_cpumask(struct device *dev, struct device_attribute *attr, char *buf) { - return cpumap_print_to_pagebuf(true, buf, &rapl_cpu_mask); + return cpumap_print_to_pagebuf(true, buf, &rapl_pmus->cpumask); } static DEVICE_ATTR(cpumask, S_IRUGO, rapl_get_attr_cpumask, NULL); @@ -565,7 +565,7 @@ static int rapl_cpu_offline(unsigned int cpu) int target; /* Check if exiting cpu is used for collecting rapl events */ - if (!cpumask_test_and_clear_cpu(cpu, &rapl_cpu_mask)) + if (!cpumask_test_and_clear_cpu(cpu, &rapl_pmus->cpumask)) return 0; rapl_pmu->cpu = -1; @@ -574,7 +574,7 @@ static int rapl_cpu_offline(unsigned int cpu) /* Migrate rapl events to the new target */ if (target < nr_cpu_ids) { - cpumask_set_cpu(target, &rapl_cpu_mask); + cpumask_set_cpu(target, &rapl_pmus->cpumask); rapl_pmu->cpu = target; perf_pmu_migrate_context(rapl_pmu->pmu, cpu, target); } @@ -606,11 +606,11 @@ static int rapl_cpu_online(unsigned int cpu) * Check if there is an online cpu in the package which collects rapl * events already. */ - target = cpumask_any_and(&rapl_cpu_mask, rapl_pmu_cpumask); + target = cpumask_any_and(&rapl_pmus->cpumask, rapl_pmu_cpumask); if (target < nr_cpu_ids) return 0; - cpumask_set_cpu(cpu, &rapl_cpu_mask); + cpumask_set_cpu(cpu, &rapl_pmus->cpumask); rapl_pmu->cpu = cpu; return 0; }
This patch is in preparation for addition of per-core energy counter support for AMD CPUs. Per-core energy counter PMU will need a separate cpumask. It seems like a better approach to add the cpumask inside the rapl_pmus struct, instead of creating another global cpumask variable for per-core PMU. This way, in future, if there is a need for a new PMU with a different scope (e.g. CCD), adding a new global cpumask variable won't be necessary. No functional change. Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> --- arch/x86/events/rapl.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)