Legacy resctrl features are enumerated by X86_FEATURE_* flags. These may be
overridden by quirks to disable features in the case of errata. Users can
use kernel command line options to either disable a feature, or to force
enable a feature that was disabled by a quirk.
Provide similar functionality for hardware features that do not have an
X86_FEATURE_* flag. Unlike other features that are tied to X86_FEATURE_*
flags, these are defined by the feature name.
Users may force a feature to be disabled. E.g. "rdt=!perf" will ensure that
none of the perf telemetry events are enabled.
Resctrl architecture code may disable a feature that does not provide full
functionality. Users may override that decision. E.g. "rdt=energy" will
enable any available energy telemetry events even if they do not provide
full functionality.
An optional guid can be included for more granular control of features sharing
a name. E.g. rdt=energy:0x12345 will only override disabling of the energy
feature with guid = 0x12345.
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
.../admin-guide/kernel-parameters.txt | 6 ++-
arch/x86/kernel/cpu/resctrl/internal.h | 2 +
arch/x86/kernel/cpu/resctrl/core.c | 2 +
arch/x86/kernel/cpu/resctrl/intel_aet.c | 37 +++++++++++++++++++
4 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 8c5636a120ee..e47def4a3dd8 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6207,9 +6207,13 @@
rdt= [HW,X86,RDT]
Turn on/off individual RDT features. List is:
cmt, mbmtotal, mbmlocal, l3cat, l3cdp, l2cat, l2cdp,
- mba, smba, bmec, abmc, sdciae.
+ mba, smba, bmec, abmc, sdciae, energy[:guid],
+ perf[:guid].
E.g. to turn on cmt and turn off mba use:
rdt=cmt,!mba
+ To turn off all energy features and ensure that
+ the 0x12345 perf feature is enabled use:
+ rdt=!energy,perf:0x12345
reboot= [KNL]
Format (x86 or x86_64):
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 0b7f8317be14..304e6e341905 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -236,6 +236,7 @@ void __exit intel_aet_exit(void);
int intel_aet_read_event(int domid, u32 rmid, void *arch_priv, u64 *val);
void intel_aet_mon_domain_setup(int cpu, int id, struct rdt_resource *r,
struct list_head *add_pos);
+bool intel_aet_option(bool force_off, char *tok);
#else
static inline bool intel_aet_get_events(void) { return false; }
static inline void __exit intel_aet_exit(void) { }
@@ -246,6 +247,7 @@ static inline int intel_aet_read_event(int domid, u32 rmid, void *arch_priv, u64
static inline void intel_aet_mon_domain_setup(int cpu, int id, struct rdt_resource *r,
struct list_head *add_pos) { }
+static inline bool intel_aet_option(bool force_off, char *tok) { return false; }
#endif
#endif /* _ASM_X86_RESCTRL_INTERNAL_H */
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 283d653002a2..960974ffa866 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -820,6 +820,8 @@ static int __init set_rdt_options(char *str)
force_off = *tok == '!';
if (force_off)
tok++;
+ if (intel_aet_option(force_off, tok))
+ continue;
for (o = rdt_options; o < &rdt_options[NUM_RDT_OPTIONS]; o++) {
if (strcmp(tok, o->name) == 0) {
if (force_off)
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)
* @pfg: Points to the aggregated telemetry space information
* returned by the intel_pmt_get_regions_by_feature()
* call to the INTEL_PMT_TELEMETRY driver that contains
* data for all telemetry regions type @feature.
* Valid if the system supports the event group.
* NULL otherwise.
+ * @force_off: True when "rdt" command line disables this @guid.
+ * @force_on: True when "rdt" command line overrides disable of
+ * this @guid due to insufficient @num_rmid.
* @guid: Unique number per XML description file.
* @mmio_size: Number of bytes of MMIO registers for this group.
* @num_events: Number of events in this group.
@@ -71,7 +75,9 @@ struct pmt_event {
struct event_group {
/* Data fields for additional structures to manage this group. */
enum pmt_feature_id feature;
+ char *name;
struct pmt_feature_group *pfg;
+ bool force_off, force_on;
/* Remaining fields initialized from XML file. */
u32 guid;
@@ -89,6 +95,7 @@ struct event_group {
*/
static struct event_group energy_0x26696143 = {
.feature = FEATURE_PER_RMID_ENERGY_TELEM,
+ .name = "energy",
.guid = 0x26696143,
.mmio_size = XML_MMIO_SIZE(576, 2, 3),
.num_events = 2,
@@ -104,6 +111,7 @@ static struct event_group energy_0x26696143 = {
*/
static struct event_group perf_0x26557651 = {
.feature = FEATURE_PER_RMID_PERF_TELEM,
+ .name = "perf",
.guid = 0x26557651,
.mmio_size = XML_MMIO_SIZE(576, 7, 3),
.num_events = 7,
@@ -128,6 +136,32 @@ static struct event_group *known_event_groups[] = {
_peg < &known_event_groups[ARRAY_SIZE(known_event_groups)]; \
_peg++)
+bool intel_aet_option(bool force_off, char *tok)
+{
+ struct event_group **peg;
+ bool ret = false;
+ u32 guid = 0;
+ char *name;
+
+ name = strsep(&tok, ":");
+ if (tok && kstrtou32(tok, 16, &guid))
+ return false;
+
+ for_each_event_group(peg) {
+ if (strcmp(name, (*peg)->name))
+ continue;
+ if (guid && (*peg)->guid != guid)
+ continue;
+ if (force_off)
+ (*peg)->force_off = true;
+ else
+ (*peg)->force_on = true;
+ ret = true;
+ }
+
+ return ret;
+}
+
/*
* Clear the address field of regions that did not pass the checks in
* skip_telem_region() so they will not be used by intel_aet_read_event().
@@ -178,6 +212,9 @@ static bool enable_events(struct event_group *e, struct pmt_feature_group *p)
{
int skipped_events = 0;
+ if (e->force_off)
+ return false;
+
if (!group_has_usable_regions(e, p))
return false;
--
2.51.1Hi Tony,
On 11/24/25 10:54 AM, Tony Luck wrote:
> Legacy resctrl features are enumerated by X86_FEATURE_* flags. These may be
> overridden by quirks to disable features in the case of errata. Users can
> use kernel command line options to either disable a feature, or to force
> enable a feature that was disabled by a quirk.
>
> Provide similar functionality for hardware features that do not have an
> X86_FEATURE_* flag. Unlike other features that are tied to X86_FEATURE_*
> flags, these are defined by the feature name.
This needs an update. Above describes previous implementation that was a generic
"for hardware features ...." while the new implementation is specific to telemetry.
(also needs imperative)
>
> Users may force a feature to be disabled. E.g. "rdt=!perf" will ensure that
> none of the perf telemetry events are enabled.
>
> Resctrl architecture code may disable a feature that does not provide full
The changelog needs to be clear on what it means by "a feature". Above implies
that "a feature" is all event groups with the same feature type ... but the final
paragraph implies that a feature is a specific event group.
> functionality. Users may override that decision. E.g. "rdt=energy" will
> enable any available energy telemetry events even if they do not provide
> full functionality.
>
> An optional guid can be included for more granular control of features sharing
"User can specify an optional guid ..."
> a name. E.g. rdt=energy:0x12345 will only override disabling of the energy
> feature with guid = 0x12345.
New implementation information just slapped onto the end of changelog.
>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> .../admin-guide/kernel-parameters.txt | 6 ++-
> arch/x86/kernel/cpu/resctrl/internal.h | 2 +
> arch/x86/kernel/cpu/resctrl/core.c | 2 +
> arch/x86/kernel/cpu/resctrl/intel_aet.c | 37 +++++++++++++++++++
> 4 files changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 8c5636a120ee..e47def4a3dd8 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6207,9 +6207,13 @@
> rdt= [HW,X86,RDT]
> Turn on/off individual RDT features. List is:
> cmt, mbmtotal, mbmlocal, l3cat, l3cdp, l2cat, l2cdp,
> - mba, smba, bmec, abmc, sdciae.
> + mba, smba, bmec, abmc, sdciae, energy[:guid],
> + perf[:guid].
> E.g. to turn on cmt and turn off mba use:
> rdt=cmt,!mba
> + To turn off all energy features and ensure that
> + the 0x12345 perf feature is enabled use:
> + rdt=!energy,perf:0x12345
The short "energy" and "perf" is reasonable to use as parameters but for help
text I think it will be better to use "energy telemetry" and "perf telemetry". How should
user interpret "energy features"? While "features" may be ok as a general
term the example is not required to use it since it is specific to telemetry monitoring.
How about something like "all energy telemetry monitoring" and "perf telemetry monitoring
associated with guid 0x12345"
>
> reboot= [KNL]
> Format (x86 or x86_64):
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 0b7f8317be14..304e6e341905 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -236,6 +236,7 @@ void __exit intel_aet_exit(void);
> int intel_aet_read_event(int domid, u32 rmid, void *arch_priv, u64 *val);
> void intel_aet_mon_domain_setup(int cpu, int id, struct rdt_resource *r,
> struct list_head *add_pos);
> +bool intel_aet_option(bool force_off, char *tok);
> #else
> static inline bool intel_aet_get_events(void) { return false; }
> static inline void __exit intel_aet_exit(void) { }
> @@ -246,6 +247,7 @@ static inline int intel_aet_read_event(int domid, u32 rmid, void *arch_priv, u64
>
> static inline void intel_aet_mon_domain_setup(int cpu, int id, struct rdt_resource *r,
> struct list_head *add_pos) { }
> +static inline bool intel_aet_option(bool force_off, char *tok) { return false; }
> #endif
>
> #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 283d653002a2..960974ffa866 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -820,6 +820,8 @@ static int __init set_rdt_options(char *str)
> force_off = *tok == '!';
> if (force_off)
> tok++;
> + if (intel_aet_option(force_off, tok))
> + continue;
> for (o = rdt_options; o < &rdt_options[NUM_RDT_OPTIONS]; o++) {
> if (strcmp(tok, o->name) == 0) {
> if (force_off)
> 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.
> * @pfg: Points to the aggregated telemetry space information
> * returned by the intel_pmt_get_regions_by_feature()
> * call to the INTEL_PMT_TELEMETRY driver that contains
> * data for all telemetry regions type @feature.
> * Valid if the system supports the event group.
> * NULL otherwise.
> + * @force_off: True when "rdt" command line disables this @guid.
Per changelog the architecture can also disable a feature.
> + * @force_on: True when "rdt" command line overrides disable of
> + * this @guid due to insufficient @num_rmid.
The idea of "insufficient RMID" does not exist at this point.
> * @guid: Unique number per XML description file.
> * @mmio_size: Number of bytes of MMIO registers for this group.
> * @num_events: Number of events in this group.
> @@ -71,7 +75,9 @@ struct pmt_event {
> struct event_group {
> /* Data fields for additional structures to manage this group. */
> enum pmt_feature_id feature;
> + char *name;
> struct pmt_feature_group *pfg;
> + bool force_off, force_on;
>
> /* Remaining fields initialized from XML file. */
> u32 guid;
> @@ -89,6 +95,7 @@ struct event_group {
> */
> static struct event_group energy_0x26696143 = {
> .feature = FEATURE_PER_RMID_ENERGY_TELEM,
> + .name = "energy",
> .guid = 0x26696143,
> .mmio_size = XML_MMIO_SIZE(576, 2, 3),
> .num_events = 2,
> @@ -104,6 +111,7 @@ static struct event_group energy_0x26696143 = {
> */
> static struct event_group perf_0x26557651 = {
> .feature = FEATURE_PER_RMID_PERF_TELEM,
> + .name = "perf",
> .guid = 0x26557651,
> .mmio_size = XML_MMIO_SIZE(576, 7, 3),
> .num_events = 7,
> @@ -128,6 +136,32 @@ static struct event_group *known_event_groups[] = {
> _peg < &known_event_groups[ARRAY_SIZE(known_event_groups)]; \
> _peg++)
>
> +bool intel_aet_option(bool force_off, char *tok)
> +{
> + struct event_group **peg;
> + bool ret = false;
> + u32 guid = 0;
> + char *name;
> +
> + name = strsep(&tok, ":");
> + if (tok && kstrtou32(tok, 16, &guid))
> + return false;
> +
> + for_each_event_group(peg) {
> + if (strcmp(name, (*peg)->name))
> + continue;
> + if (guid && (*peg)->guid != guid)
> + continue;
> + if (force_off)
> + (*peg)->force_off = true;
> + else
> + (*peg)->force_on = true;
> + ret = true;
> + }
> +
> + return ret;
> +}
> +
> /*
> * Clear the address field of regions that did not pass the checks in
> * skip_telem_region() so they will not be used by intel_aet_read_event().
> @@ -178,6 +212,9 @@ static bool enable_events(struct event_group *e, struct pmt_feature_group *p)
> {
> int skipped_events = 0;
>
> + if (e->force_off)
> + return false;
> +
> if (!group_has_usable_regions(e, p))
> return false;
>
Reinette
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.
That covers how different instances have the same string where "name"
was confusing.
-Tony
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
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
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
© 2016 - 2026 Red Hat, Inc.