Re: [PATCH v14 24/32] x86/resctrl: Add energy/perf choices to rdt boot option

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 12/3/25 10:04 AM, Luck, Tony wrote:
> On Tue, Dec 02, 2025 at 08:28:56AM -0800, Reinette Chatre wrote:
>>> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
>>> index 46c64419ec10..50c8b4c50790 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
>>> @@ -57,12 +57,16 @@ struct pmt_event {
>>>   * struct event_group - Events with the same feature type ("energy" or "perf") and guid.
>>>   * @feature:		Type of events, for example FEATURE_PER_RMID_PERF_TELEM or
>>>   *			FEATURE_PER_RMID_ENERGY_TELEM, in this group.
>>> + * @name:		Name for this group (used by boot rdt= option)
>>
>> This needs a new definition since multiple groups can have the same name now.
> 
> How about this:
> 
>  * @type:               Type (energy or perf) of this group.

I find this to be confusing when considering it together with existing @feature and its
definition that also refers to itself as a "type" using perf and energy terms.

> 
> That covers how different instances have the same string where "name"
> was confusing.
> 
Essentially this is the name used for @feature and for this there is already
pmt_feature_names[]. Is it needed to create a new name? This would mean that the
kernel parameters become "per_rmid_energy_telemetry" and "per_rmid_perf_telemetry" which
is much longer though. The only limiting factor I am aware of is the command line size which
is defined in arch/x86/include/asm/setup.h as 2048. Here I do not know if there are customs on
whether kernel parameters need to be brief or not ... some kernel parameters seem to be quite
verbose while others are cryptic.

The safest may be to stick with the separate names but I am curious about your opinion.

Even so, it seems unnecessary to force each new instance of struct event_group to
set a parameter that is required to be of particular value based on value of event_group::feature.
If not using pmt_feature_names[] then intel_aet.c could have its own private array
that maps pmt_feature_id to "energy" or "perf". I find that doing so would make it obvious
what this property is/should be.

What do you think?

Reinette