Hi Tony,
On 5/21/25 3:50 PM, Tony Luck wrote:
> There are now three meanings for "number of RMIDs":
>
> 1) The number for legacy features enumerated by CPUID leaf 0xF. This
> is the maximum number of distinct values that can be loaded into the
> IA32_PQR_ASSOC MSR. Note that systems with Sub-NUMA Cluster mode enabled
> will force scaling down the CPUID enumerated value by the number of SNC
> nodes per L3-cache.
>
> 2) The number of registers in MMIO space for each event. This
> is enumerated in the XML files and is the value initialized into
> event_group::num_rmids. This will be overwritten with a lower
> value if hardware does not support all these registers at the
> same time (see next case).
>
> 3) The number of "h/w counters" (this isn't a strictly accurate
> description of how things work, but serves as a useful analogy that
> does describe the limitations) feeding to those MMIO registers. This
> is enumerated in telemetry_region::num_rmids returned from the call to
> intel_pmt_get_regions_by_feature()
>
> Event groups with insufficient "h/w counter" to track all RMIDs are
I'd like to highlight that the above sentence follows a section with
heading "There are now three meanings for "number of RMIDs":" ... following
such a section with a sentence that then refers to "all RMIDs" is bound
to make reader wonder which of the three RMIDs are being talked about.
Perhaps something like "Event groups with insufficient "h/w counter" to
track all values that can be loaded into the IA32_PQR_ASSOC MSR ..."
> difficult for users to use, since the system may reassign "h/w counters"
> as any time. This means that users cannot reliably collect two consecutive
> event counts to compute the rate at which events are occurring.
>
> Ignore such under-resourced event groups unless the user explicitly
> requests to enable them using the "rdt=" Linux boot argument.
>
> Scan all enabled event groups and assign the RDT_RESOURCE_PERF_PKG
> resource "num_rmids" value to the smallest of these values to ensure
> that all resctrl groups have equal monitor capabilities.
>
> N.B. Changed type of rdt_resource::num_rmids to u32 to match.
>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> include/linux/resctrl.h | 2 +-
> arch/x86/kernel/cpu/resctrl/internal.h | 2 ++
> arch/x86/kernel/cpu/resctrl/intel_aet.c | 27 +++++++++++++++++++++++++
> arch/x86/kernel/cpu/resctrl/monitor.c | 2 ++
> 4 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 4ba51cb598e1..b7e15abcde23 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -286,7 +286,7 @@ struct rdt_resource {
> int rid;
> bool alloc_capable;
> bool mon_capable;
> - int num_rmid;
> + u32 num_rmid;
> enum resctrl_scope ctrl_scope;
> enum resctrl_scope mon_scope;
> struct resctrl_cache cache;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 524f3c183900..795534b9b9d2 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -18,6 +18,8 @@
>
> #define RMID_VAL_UNAVAIL BIT_ULL(62)
>
> +extern int rdt_num_system_rmids;
> +
> /*
> * With the above fields in use 62 bits remain in MSR_IA32_QM_CTR for
> * data to be returned. The counter width is discovered from the hardware
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index c1fc85dbf0d8..1b41167ad976 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -14,6 +14,7 @@
> #include <linux/cleanup.h>
> #include <linux/cpu.h>
> #include <linux/io.h>
> +#include <linux/minmax.h>
> #include <linux/resctrl.h>
> #include <linux/slab.h>
>
> @@ -57,6 +58,9 @@ struct pmt_event {
> * telemetry regions.
> * @pkginfo: Per-package MMIO addresses of telemetry regions belonging to this group
> * @guid: Unique number per XML description file.
> + * @num_rmids: Number of RMIDS supported by this group. Will be adjusted downwards
"Will be adjusted" -> "Adjusted"
> + * if enumeration from intel_pmt_get_regions_by_feature() indicates
> + * fewer RMIDs can be tracked simultaneously.
> * @mmio_size: Number of bytes of MMIO registers for this group.
> * @num_events: Number of events in this group.
> * @evts: Array of event descriptors.
> @@ -69,6 +73,7 @@ struct event_group {
>
> /* Remaining fields initialized from XML file. */
> u32 guid;
> + u32 num_rmids;
> size_t mmio_size;
> int num_events;
> struct pmt_event evts[] __counted_by(num_events);
> @@ -81,6 +86,7 @@ struct event_group {
> static struct event_group energy_0x26696143 = {
> .name = "energy",
> .guid = 0x26696143,
> + .num_rmids = 576,
> .mmio_size = (576 * 2 + 3) * 8,
> .num_events = 2,
> .evts = {
> @@ -96,6 +102,7 @@ static struct event_group energy_0x26696143 = {
> static struct event_group perf_0x26557651 = {
> .name = "perf",
> .guid = 0x26557651,
> + .num_rmids = 576,
> .mmio_size = (576 * 7 + 3) * 8,
> .num_events = 7,
> .evts = {
> @@ -253,6 +260,15 @@ static bool get_pmt_feature(enum pmt_feature_id feature)
> if ((*peg)->guid == p->regions[i].guid) {
> if (rdt_is_option_force_disabled((*peg)->name))
> return false;
> + /*
> + * Ignore event group with insufficient RMIDs unless the
"insufficient RMIDs" -> "fewer RMIDs than can be loaded into the IA32_PQR_ASSOC MSR"?
Please feel free to improve.
> + * user used the rdt= boot option to specifically ask
> + * for it to be enabled.
> + */
> + if (p->regions[i].num_rmids < rdt_num_system_rmids &&
> + !rdt_is_option_force_enabled((*peg)->name))
> + return false;
> + (*peg)->num_rmids = min((*peg)->num_rmids, p->regions[i].num_rmids);
> ret = configure_events(*peg, p);
> if (!ret) {
> (*peg)->pfg = no_free_ptr(p);
> @@ -272,11 +288,22 @@ static bool get_pmt_feature(enum pmt_feature_id feature)
> */
> bool intel_aet_get_events(void)
> {
> + struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_PERF_PKG].r_resctrl;
> + struct event_group **eg;
> bool ret1, ret2;
>
> ret1 = get_pmt_feature(FEATURE_PER_RMID_ENERGY_TELEM);
> ret2 = get_pmt_feature(FEATURE_PER_RMID_PERF_TELEM);
>
> + for (eg = &known_event_groups[0]; eg < &known_event_groups[NUM_KNOWN_GROUPS]; eg++) {
> + if (!(*eg)->pfg)
> + continue;
> + if (r->num_rmid)
> + r->num_rmid = min(r->num_rmid, (*eg)->num_rmids);
> + else
> + r->num_rmid = (*eg)->num_rmids;
> + }
> +
> return ret1 || ret2;
> }
>
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index c99aa9dacfd8..9cd37be262a2 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -32,6 +32,7 @@ bool rdt_mon_capable;
>
> #define CF(cf) ((unsigned long)(1048576 * (cf) + 0.5))
>
> +int rdt_num_system_rmids;
> static int snc_nodes_per_l3_cache = 1;
>
> /*
> @@ -350,6 +351,7 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
> resctrl_rmid_realloc_limit = boot_cpu_data.x86_cache_size * 1024;
> hw_res->mon_scale = boot_cpu_data.x86_cache_occ_scale / snc_nodes_per_l3_cache;
> r->num_rmid = (boot_cpu_data.x86_cache_max_rmid + 1) / snc_nodes_per_l3_cache;
> + rdt_num_system_rmids = r->num_rmid;
> hw_res->mbm_width = MBM_CNTR_WIDTH_BASE;
>
> if (mbm_offset > 0 && mbm_offset <= MBM_CNTR_WIDTH_OFFSET_MAX)
Reinette