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

[Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>]

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. 

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;