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

["Luck, Tony" <tony.luck@intel.com>]

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