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

Reinette Chatre
2025-12-03T23:25:29Z

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