From: Kan Liang <kan.liang@linux.intel.com>
The rapl pmu just needs to be allocated once. It doesn't matter to be
allocated at each CPU hotplug, or the global init_rapl_pmus().
Move the pmu allocation to the init_rapl_pmus(). So the generic hotplug
supports can be applied.
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
Cc: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
arch/x86/events/rapl.c | 43 +++++++++++++++++++++++++++++-------------
1 file changed, 30 insertions(+), 13 deletions(-)
diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index b985ca79cf97..f8b6d504d03f 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -568,19 +568,8 @@ static int rapl_cpu_online(unsigned int cpu)
struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);
int target;
- if (!pmu) {
- pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
- if (!pmu)
- return -ENOMEM;
-
- 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);
-
- rapl_pmus->pmus[topology_logical_die_id(cpu)] = pmu;
- }
+ if (!pmu)
+ return -ENOMEM;
/*
* Check if there is an online cpu in the package which collects rapl
@@ -673,6 +662,32 @@ static const struct attribute_group *rapl_attr_update[] = {
NULL,
};
+static void __init init_rapl_pmu(void)
+{
+ struct rapl_pmu *pmu;
+ int cpu;
+
+ cpus_read_lock();
+
+ for_each_cpu(cpu, cpu_online_mask) {
+ pmu = cpu_to_rapl_pmu(cpu);
+ if (pmu)
+ continue;
+ pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
+ if (!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);
+
+ rapl_pmus->pmus[topology_logical_die_id(cpu)] = pmu;
+ }
+
+ cpus_read_unlock();
+}
+
static int __init init_rapl_pmus(void)
{
int nr_rapl_pmu = topology_max_packages() * topology_max_dies_per_package();
@@ -681,6 +696,8 @@ static int __init init_rapl_pmus(void)
if (!rapl_pmus)
return -ENOMEM;
+ init_rapl_pmu();
+
rapl_pmus->nr_rapl_pmu = nr_rapl_pmu;
rapl_pmus->pmu.attr_groups = rapl_attr_groups;
rapl_pmus->pmu.attr_update = rapl_attr_update;
--
2.38.1Hello Kan,
On 8/2/2024 8:46 PM, kan.liang@linux.intel.com wrote:
> From: Kan Liang <kan.liang@linux.intel.com>
>
> The rapl pmu just needs to be allocated once. It doesn't matter to be
> allocated at each CPU hotplug, or the global init_rapl_pmus().
>
> Move the pmu allocation to the init_rapl_pmus(). So the generic hotplug
> supports can be applied.
>
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> Cc: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> ---
> arch/x86/events/rapl.c | 43 +++++++++++++++++++++++++++++-------------
> 1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index b985ca79cf97..f8b6d504d03f 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -568,19 +568,8 @@ static int rapl_cpu_online(unsigned int cpu)
> struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);
> int target;
>
> - if (!pmu) {
> - pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
> - if (!pmu)
> - return -ENOMEM;
> -
> - 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);
> -
> - rapl_pmus->pmus[topology_logical_die_id(cpu)] = pmu;
> - }
> + if (!pmu)
> + return -ENOMEM;
>
> /*
> * Check if there is an online cpu in the package which collects rapl
> @@ -673,6 +662,32 @@ static const struct attribute_group *rapl_attr_update[] = {
> NULL,
> };
>
> +static void __init init_rapl_pmu(void)
> +{
> + struct rapl_pmu *pmu;
> + int cpu;
> +
> + cpus_read_lock();
> +
> + for_each_cpu(cpu, cpu_online_mask) {
> + pmu = cpu_to_rapl_pmu(cpu);
> + if (pmu)
> + continue;
> + pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
> + if (!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);
> +
> + rapl_pmus->pmus[topology_logical_die_id(cpu)] = pmu;
> + }
> +
> + cpus_read_unlock();
> +}
> +
> static int __init init_rapl_pmus(void)
> {
> int nr_rapl_pmu = topology_max_packages() * topology_max_dies_per_package();
> @@ -681,6 +696,8 @@ static int __init init_rapl_pmus(void)
> if (!rapl_pmus)
> return -ENOMEM;
>
> + init_rapl_pmu();
> +
> rapl_pmus->nr_rapl_pmu = nr_rapl_pmu;
I feel there is one potential bug here, first we are calling init_rapl_pmu() --> cpu_to_rapl_pmu(cpu) --> "return rapl_pmu_idx < rapl_pmus->nr_rapl_pmu ? rapl_pmus->pmus[rapl_pmu_idx] : NULL;"
Then we are updating "rapl_pmus->nr_rapl_pmu = nr_rapl_pmu;". This makes the return check in cpu_to_rapl_pmu() ineffective as the rapl_pmus->nr_rapl_pmu value would be falsely zero.
Please let me know if I'm missing something.
Thanks,
Dhananjay
> rapl_pmus->pmu.attr_groups = rapl_attr_groups;
> rapl_pmus->pmu.attr_update = rapl_attr_update;
Hi Dhananjay,
On 2024-09-09 5:26 a.m., Dhananjay Ugwekar wrote:
> Hello Kan,
>
> On 8/2/2024 8:46 PM, kan.liang@linux.intel.com wrote:
>> From: Kan Liang <kan.liang@linux.intel.com>
>>
>> The rapl pmu just needs to be allocated once. It doesn't matter to be
>> allocated at each CPU hotplug, or the global init_rapl_pmus().
>>
>> Move the pmu allocation to the init_rapl_pmus(). So the generic hotplug
>> supports can be applied.
>>
>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
>> Cc: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
>> ---
>> arch/x86/events/rapl.c | 43 +++++++++++++++++++++++++++++-------------
>> 1 file changed, 30 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
>> index b985ca79cf97..f8b6d504d03f 100644
>> --- a/arch/x86/events/rapl.c
>> +++ b/arch/x86/events/rapl.c
>> @@ -568,19 +568,8 @@ static int rapl_cpu_online(unsigned int cpu)
>> struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);
>> int target;
>>
>> - if (!pmu) {
>> - pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
>> - if (!pmu)
>> - return -ENOMEM;
>> -
>> - 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);
>> -
>> - rapl_pmus->pmus[topology_logical_die_id(cpu)] = pmu;
>> - }
>> + if (!pmu)
>> + return -ENOMEM;
>>
>> /*
>> * Check if there is an online cpu in the package which collects rapl
>> @@ -673,6 +662,32 @@ static const struct attribute_group *rapl_attr_update[] = {
>> NULL,
>> };
>>
>> +static void __init init_rapl_pmu(void)
>> +{
>> + struct rapl_pmu *pmu;
>> + int cpu;
>> +
>> + cpus_read_lock();
>> +
>> + for_each_cpu(cpu, cpu_online_mask) {
>> + pmu = cpu_to_rapl_pmu(cpu);
>> + if (pmu)
>> + continue;
>> + pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
>> + if (!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);
>> +
>> + rapl_pmus->pmus[topology_logical_die_id(cpu)] = pmu;
>> + }
>> +
>> + cpus_read_unlock();
>> +}
>> +
>> static int __init init_rapl_pmus(void)
>> {
>> int nr_rapl_pmu = topology_max_packages() * topology_max_dies_per_package();
>> @@ -681,6 +696,8 @@ static int __init init_rapl_pmus(void)
>> if (!rapl_pmus)
>> return -ENOMEM;
>>
>> + init_rapl_pmu();
>> +
>> rapl_pmus->nr_rapl_pmu = nr_rapl_pmu;
>
> I feel there is one potential bug here, first we are calling init_rapl_pmu() --> cpu_to_rapl_pmu(cpu) --> "return rapl_pmu_idx < rapl_pmus->nr_rapl_pmu ? rapl_pmus->pmus[rapl_pmu_idx] : NULL;"
> Then we are updating "rapl_pmus->nr_rapl_pmu = nr_rapl_pmu;". This makes the return check in cpu_to_rapl_pmu() ineffective as the rapl_pmus->nr_rapl_pmu value would be falsely zero.
>
Ah, right. A pmu will be allocated for each CPU rather than each socket.
A user wouldn't see a difference, but it wastes memory and may cause
memory leak.
I think we should move the init_rapl_pmu(); to the end of the function.
The patch set has been merged into Peter's perf/core branch. Do you want
to post a fix patch to address the issue?
Thanks,
Kan
> Please let me know if I'm missing something.
>
> Thanks,
> Dhananjay
>
>> rapl_pmus->pmu.attr_groups = rapl_attr_groups;
>> rapl_pmus->pmu.attr_update = rapl_attr_update;
>
On Mon, Sep 09, 2024 at 09:02:56AM -0400, Liang, Kan wrote: > The patch set has been merged into Peter's perf/core branch. Do you want > to post a fix patch to address the issue? I've not yet pushed out to tip, so I can readily rebase. Send me a delta and indicate what patch it should go into and I'll make it happen.
On 2024-09-09 9:24 a.m., Peter Zijlstra wrote:
> On Mon, Sep 09, 2024 at 09:02:56AM -0400, Liang, Kan wrote:
>
>> The patch set has been merged into Peter's perf/core branch. Do you want
>> to post a fix patch to address the issue?
>
> I've not yet pushed out to tip, so I can readily rebase. Send me a delta
> and indicate what patch it should go into and I'll make it happen.
>
Thanks Peter. Please fold the below patch into commit 90942140bb6c
("perf/x86/rapl: Move the pmu allocation out of CPU hotplug").
diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index b70ad880c5bc..1b38f8771488 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -631,8 +632,6 @@ static int __init init_rapl_pmus(void)
if (!rapl_pmus)
return -ENOMEM;
- init_rapl_pmu();
-
rapl_pmus->nr_rapl_pmu = nr_rapl_pmu;
rapl_pmus->pmu.attr_groups = rapl_attr_groups;
rapl_pmus->pmu.attr_update = rapl_attr_update;
@@ -646,6 +645,9 @@ static int __init init_rapl_pmus(void)
rapl_pmus->pmu.module = THIS_MODULE;
rapl_pmus->pmu.scope = PERF_PMU_SCOPE_DIE;
rapl_pmus->pmu.capabilities = PERF_PMU_CAP_NO_EXCLUDE;
+
+ init_rapl_pmu();
+
return 0;
}
Thanks,
Kan
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 351e6ba39e5c851b00d83716ffb4d19b807ecc3d
Gitweb: https://git.kernel.org/tip/351e6ba39e5c851b00d83716ffb4d19b807ecc3d
Author: Kan Liang <kan.liang@linux.intel.com>
AuthorDate: Fri, 02 Aug 2024 08:16:42 -07:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Tue, 10 Sep 2024 11:44:14 +02:00
perf/x86/rapl: Move the pmu allocation out of CPU hotplug
The rapl pmu just needs to be allocated once. It doesn't matter to be
allocated at each CPU hotplug, or the global init_rapl_pmus().
Move the pmu allocation to the init_rapl_pmus(). So the generic hotplug
supports can be applied.
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/r/20240802151643.1691631-7-kan.liang@linux.intel.com
---
arch/x86/events/rapl.c | 44 ++++++++++++++++++++++++++++-------------
1 file changed, 31 insertions(+), 13 deletions(-)
diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index b985ca7..d12f3a6 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -568,19 +568,8 @@ static int rapl_cpu_online(unsigned int cpu)
struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);
int target;
- if (!pmu) {
- pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
- if (!pmu)
- return -ENOMEM;
-
- 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);
-
- rapl_pmus->pmus[topology_logical_die_id(cpu)] = pmu;
- }
+ if (!pmu)
+ return -ENOMEM;
/*
* Check if there is an online cpu in the package which collects rapl
@@ -673,6 +662,32 @@ static const struct attribute_group *rapl_attr_update[] = {
NULL,
};
+static void __init init_rapl_pmu(void)
+{
+ struct rapl_pmu *pmu;
+ int cpu;
+
+ cpus_read_lock();
+
+ for_each_cpu(cpu, cpu_online_mask) {
+ pmu = cpu_to_rapl_pmu(cpu);
+ if (pmu)
+ continue;
+ pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
+ if (!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);
+
+ rapl_pmus->pmus[topology_logical_die_id(cpu)] = pmu;
+ }
+
+ cpus_read_unlock();
+}
+
static int __init init_rapl_pmus(void)
{
int nr_rapl_pmu = topology_max_packages() * topology_max_dies_per_package();
@@ -693,6 +708,9 @@ static int __init init_rapl_pmus(void)
rapl_pmus->pmu.read = rapl_pmu_event_read;
rapl_pmus->pmu.module = THIS_MODULE;
rapl_pmus->pmu.capabilities = PERF_PMU_CAP_NO_EXCLUDE;
+
+ init_rapl_pmu();
+
return 0;
}© 2016 - 2025 Red Hat, Inc.