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 2:27 PM, Luck, Tony wrote:
> On Wed, Dec 03, 2025 at 01:21:56PM -0800, Reinette Chatre wrote:
>> 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.
> 
> Agreed. The two fields duplicate the same basic function.
> 
>>>
>>> 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.
> 
> Personally I prefer brief names (as it isn't always possible to cut and
> paste to a remote serial console window to add them in grub edit mode).
>>
>> The safest may be to stick with the separate names but I am curious about your opinion.
> 
> Short names are good.
> 
>> 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?
> 
> I agree with one value and a mapping function. But I'm using the string
> name eight times, and the event_group::feature just once.  So I think
> I'd like to keep the string name in the structure, and just do a lookup
> of the enum in order to call intel_pmt_get_regions_by_feature().

ok. Since intel_pmt_get_regions_by_feature() is always called early and only once for
an event group this seems good to ensure that the name is always valid.

> 
> So new proposal. New name event_group::pfname for the string:
> 
>  @pfname:            PMT feature type (energy or perf) of this event group.

I think the addition of "used by boot rdt= option"/"used by rdt= kernel parameter"
is valuable to highlight this cannot be some arbitrary name but is instead connected
to ABI.

nit: should this instead be "PMT feature name" as used in the error message below?

> 
> static enum pmt_feature_id lookup_pfid(char *pfname)

Please handle the name as a const throughout, as a struct member and its handling.

> {
> 	if (!strcmp(pfname, "energy"))
> 		return FEATURE_PER_RMID_ENERGY_TELEM;
> 	else if (!strcmp(pfname, "perf"))
> 		return FEATURE_PER_RMID_PERF_TELEM;
> 
> 	pr_warn("Unknown PMT feature name '%s'\n", pfname);
> 
> 	return FEATURE_INVALID;

Looks like intel_pmt_get_regions_by_feature() handles FEATURE_INVALID fine.

> }
> 
> There are only two options, and no sign of additional ones. So this
> if/else style seems simplest, rather than creating a lookup table to
> iterate through looking for the feature name.

ack.

Reinette