Re: [PATCH 6/7] perf/x86/rapl: Move the pmu allocation out of CPU hotplug

[Liang, Kan]

Hi Dhananjay,

On 2024-09-09 5:26 a.m., Dhananjay Ugwekar wrote:
> Hello Kan,
> 
> On 8/2/2024 8:46 PM, kan.liang@xxxxxxxxxxxxxxx wrote:
>> From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx>
>> Cc: Dhananjay Ugwekar <Dhananjay.Ugwekar@xxxxxxx>
>> ---
>>  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;
>