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

Luck, Tony
2025-12-03T22:27:24Z

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().

So new proposal. New name event_group::pfname for the string:

@pfname: PMT feature type (energy or perf) of this event group.

static enum pmt_feature_id lookup_pfid(char *pfname)
{
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;
}

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.

-Tony