Re: [PATCH v6 04/10] perf/x86/rapl: Rename rapl_pmu variables

["Gautham R. Shenoy" <gautham.shenoy@amd.com>]

Hello Dhananjay,

On Fri, Oct 25, 2024 at 11:13:42AM +0000, Dhananjay Ugwekar wrote:
> 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>

This looks good to me.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
--
Thanks and Regards
gautham.

> ---
>  arch/x86/events/rapl.c | 91 +++++++++++++++++++++---------------------
>  1 file changed, 46 insertions(+), 45 deletions(-)
> 
> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
> index d20c5b1dd0ad..7387bca95018 100644
> --- a/arch/x86/events/rapl.c
> +++ b/arch/x86/events/rapl.c
> @@ -129,7 +129,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,27 +616,27 @@ static const struct attribute_group *rapl_attr_update[] = {
>  
>  static int __init init_rapl_pmu(void)
>  {
> -	struct rapl_pmu *pmu;
> +	struct rapl_pmu *rapl_pmu;
>  	int idx;
>  
>  	for (idx = 0; idx < rapl_pmus->nr_rapl_pmu; idx++) {
> -		pmu = kzalloc(sizeof(*pmu), GFP_KERNEL);
> -		if (!pmu)
> +		rapl_pmu = kzalloc(sizeof(*rapl_pmu), GFP_KERNEL);
> +		if (!rapl_pmu)
>  			goto free;
>  
> -		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[idx] = pmu;
> +		rapl_pmus->rapl_pmu[idx] = rapl_pmu;
>  	}
>  
>  	return 0;
>  free:
>  	for (; idx > 0; idx--)
> -		kfree(rapl_pmus->pmus[idx - 1]);
> +		kfree(rapl_pmus->rapl_pmu[idx - 1]);
>  	return -ENOMEM;
>  }
>  
> @@ -649,7 +650,7 @@ static int __init init_rapl_pmus(void)
>  		rapl_pmu_scope = PERF_PMU_SCOPE_DIE;
>  	}
>  
> -	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
>