Re: [PATCH v5 9/9] perf/x86/rapl: Add per-core energy counter support for AMD CPUs

[Dhananjay Ugwekar]

Hello Gautham,

On 10/7/2024 12:11 PM, Gautham R. Shenoy wrote:
> On Fri, Sep 13, 2024 at 03:48:01PM +0000, Dhananjay Ugwekar wrote:
>> Add a new "power_per_core" PMU and "energy-per-core" event for
>> monitoring energy consumption by each core. The existing energy-cores
>> event aggregates the energy consumption at the package level.
>> This new event aligns with the AMD's per_core energy counters.
[Snip]
>>  
>> @@ -156,10 +170,14 @@ static struct rapl_model *rapl_model;
>>   * Helper function to get the correct topology id according to the
>>   * RAPL PMU scope.
>>   */
>> -static inline unsigned int get_rapl_pmu_idx(int cpu)
>> +static inline unsigned int get_rapl_pmu_idx(int cpu, int scope)
>>  {
>> -	return rapl_pkg_pmu_is_pkg_scope() ? topology_logical_package_id(cpu) :
>> -					 topology_logical_die_id(cpu);
>> +	if (scope == PERF_PMU_SCOPE_PKG)
>> +		return topology_logical_package_id(cpu);
>> +	else if (scope == PERF_PMU_SCOPE_DIE)
> 
> You don't need the "else if" since you are returning if there is a
> match for the earlier if condition.

Right, will modify accordingly

> 
>> +		return topology_logical_die_id(cpu);
>> +	else
>         ^^^^^
> Please check if the scope is SCOPE_CORE here. Again, you don't need the
> else condition.

Yes, will add the if check and remove the else

> 
> 
>> +		return topology_logical_core_id(cpu);
> 
> 
> 
>>  }
>>  
>>  static inline u64 rapl_read_counter(struct perf_event *event)
[Snip]
>> @@ -337,12 +356,13 @@ static void rapl_pmu_event_del(struct perf_event *event, int flags)
>>  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;
>> +	int bit, rapl_pmus_scope, rapl_pmu_idx, ret = 0;
>>  	struct rapl_pmu *rapl_pmu;
>> +	struct rapl_pmus *rapl_pmus;
>>  
>> -	/* only look at RAPL events */
>> -	if (event->attr.type != rapl_pmus_pkg->pmu.type)
>> -		return -ENOENT;
> 
> Don't we need the check to only look at RAPL events of pkg or core ?
> Or is that covered by a check below ?

I moved this check to the PMU specific if blocks (highlighted below)

> 
> 
> 
>> +	/* unsupported modes and filters */
>> +	if (event->attr.sample_period) /* no sampling */
>> +		return -EINVAL;
>>  
>>  	/* check only supported bits are set */
>>  	if (event->attr.config & ~RAPL_EVENT_MASK)
>> @@ -351,31 +371,49 @@ static int rapl_pmu_event_init(struct perf_event *event)
>>  	if (event->cpu < 0)
>>  		return -EINVAL;
>>  
>> -	if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
>> +	rapl_pmus = container_of(event->pmu, struct rapl_pmus, pmu);
>> +	if (!rapl_pmus)
>>  		return -EINVAL;
>> -
>> -	cfg = array_index_nospec((long)cfg, NR_RAPL_PKG_DOMAINS + 1);
>> -	bit = cfg - 1;
>> -
>> -	/* check event supported */
>> -	if (!(rapl_pmus_pkg->cntr_mask & (1 << bit)))
>> +	rapl_pmus_scope = rapl_pmus->pmu.scope;
>> +
>> +	if (rapl_pmus_scope == PERF_PMU_SCOPE_PKG || rapl_pmus_scope == PERF_PMU_SCOPE_DIE) {
>> +		/* only look at RAPL package events */
>> +		if (event->attr.type != rapl_pmus_pkg->pmu.type)
>> +			return -ENOENT; 		^^^^ here and
>> +
>> +		cfg = array_index_nospec((long)cfg, NR_RAPL_PKG_DOMAINS + 1);
>> +		if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
>> +			return -EINVAL;
>> +
>> +		bit = cfg - 1;
>> +		event->hw.event_base = rapl_model->rapl_pkg_msrs[bit].msr;
>> +	} else if (rapl_pmus_scope == PERF_PMU_SCOPE_CORE) {
>> +		/* only look at RAPL per-core events */
>> +		if (event->attr.type != rapl_pmus_core->pmu.type)
>> +			return -ENOENT;
		^^^^ here
>> +
>> +		cfg = array_index_nospec((long)cfg, NR_RAPL_CORE_DOMAINS + 1);
>> +		if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
>> +			return -EINVAL;
>> +
>> +		bit = cfg - 1;
>> +		event->hw.event_base = rapl_model->rapl_core_msrs[bit].msr;
>> +	} else
>>  		return -EINVAL;
>>  
>> -	/* unsupported modes and filters */
>> -	if (event->attr.sample_period) /* no sampling */
>> +	/* check event supported */
>> +	if (!(rapl_pmus->cntr_mask & (1 << bit)))
>>  		return -EINVAL;
>>  
>> -	rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
>> -	if (rapl_pmu_idx >= rapl_pmus_pkg->nr_rapl_pmu)
>> +	rapl_pmu_idx = get_rapl_pmu_idx(event->cpu, rapl_pmus_scope);
>> +	if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
>>  		return -EINVAL;
>> -
>>  	/* must be done before validate_group */
>> -	rapl_pmu = rapl_pmus_pkg->rapl_pmu[rapl_pmu_idx];
>> +	rapl_pmu = rapl_pmus->rapl_pmu[rapl_pmu_idx];
>>  	if (!rapl_pmu)
>>  		return -EINVAL;
>>  
>>  	event->pmu_private = rapl_pmu;
>> -	event->hw.event_base = rapl_model->rapl_pkg_msrs[bit].msr;
>>  	event->hw.config = cfg;
>>  	event->hw.idx = bit;
>>  
[Snip]
>> @@ -644,15 +720,19 @@ static void __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
>>  	cpus_read_unlock();
>>  }
>>  
>> -static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr, int rapl_pmu_scope)
>> +static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr, int rapl_pmu_scope,
>> +				 const struct attribute_group **rapl_attr_groups,
>> +				 const struct attribute_group **rapl_attr_update)
>>  {
>>  	int nr_rapl_pmu;
>>  	struct rapl_pmus *rapl_pmus;
>>  
>>  	if (rapl_pmu_scope == PERF_PMU_SCOPE_PKG)
>>  		nr_rapl_pmu	= topology_max_packages();
>> -	else
>> +	else if (rapl_pmu_scope == PERF_PMU_SCOPE_DIE)
>>  		nr_rapl_pmu	= topology_max_packages() * topology_max_dies_per_package();
>> +	else
>> +		nr_rapl_pmu	= topology_max_packages() * topology_num_cores_per_package();
> 
> Here please check if the rapl_pmu_scope is PERF_PMU_SCOPE_CORE instead
> of assuming it to be the case if the scope is neither SCOPE_PKG nor
> SCOPE_DIE. If it is neither of these three, then return an error.

Actually, I thought there is only one caller of this function (rapl_pmu_init), 
which only passes one of these 3 values, so I thought maybe the error check isnt 
needed. What do you think? 

Thanks,
Dhananjay

> 
> 
> --
> Thanks and Regards
> gautham.
> 
>>  
>>  	rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu, nr_rapl_pmu), GFP_KERNEL);
>>  	if (!rapl_pmus)