The fs/arch boundary is a little muddy for adding new monitor features.

Clean it up by making the mon_evt structure the source of all information
about each event. In this case replace the bitmap of enabled monitor
features with an "enabled" bit in the mon_evt structure.

Change architecture code to inform file system code which events are
available on a system with resctrl_enable_mon_event().

Replace the event and architecture specific:
	resctrl_arch_is_llc_occupancy_enabled()
	resctrl_arch_is_mbm_total_enabled()
	resctrl_arch_is_mbm_local_enabled()
functions with calls to resctrl_is_mon_event_enabled() with the
appropriate QOS_L3_* enum resctrl_event_id.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 include/linux/resctrl.h               |  2 +
 arch/x86/include/asm/resctrl.h        | 16 -------
 fs/resctrl/internal.h                 |  4 ++
 arch/x86/kernel/cpu/resctrl/core.c    | 25 +++++++----
 arch/x86/kernel/cpu/resctrl/monitor.c |  9 +---
 fs/resctrl/ctrlmondata.c              |  4 +-
 fs/resctrl/monitor.c                  | 60 ++++++++++++++++-----------
 fs/resctrl/rdtgroup.c                 | 18 ++++----
 8 files changed, 71 insertions(+), 67 deletions(-)

diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 9ba771f2ddea..3c5d111aae65 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -372,6 +372,8 @@ u32 resctrl_arch_get_num_closid(struct rdt_resource *r);
 u32 resctrl_arch_system_num_rmid_idx(void);
 int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid);
 
+void resctrl_enable_mon_event(enum resctrl_event_id evtid);
+bool resctrl_is_mon_event_enabled(enum resctrl_event_id evt);
 bool resctrl_arch_is_evt_configurable(enum resctrl_event_id evt);
 
 /**
diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
index 189f885dcf3e..a59b3adb56cd 100644
--- a/arch/x86/include/asm/resctrl.h
+++ b/arch/x86/include/asm/resctrl.h
@@ -42,7 +42,6 @@ DECLARE_PER_CPU(struct resctrl_pqr_state, pqr_state);
 
 extern bool rdt_alloc_capable;
 extern bool rdt_mon_capable;
-extern unsigned int rdt_mon_features;
 
 DECLARE_STATIC_KEY_FALSE(rdt_enable_key);
 DECLARE_STATIC_KEY_FALSE(rdt_alloc_enable_key);
@@ -82,21 +81,6 @@ static inline void resctrl_arch_disable_mon(void)
 	static_branch_dec_cpuslocked(&rdt_enable_key);
 }
 
-static inline bool resctrl_arch_is_llc_occupancy_enabled(void)
-{
-	return (rdt_mon_features & (1 << QOS_L3_OCCUP_EVENT_ID));
-}
-
-static inline bool resctrl_arch_is_mbm_total_enabled(void)
-{
-	return (rdt_mon_features & (1 << QOS_L3_MBM_TOTAL_EVENT_ID));
-}
-
-static inline bool resctrl_arch_is_mbm_local_enabled(void)
-{
-	return (rdt_mon_features & (1 << QOS_L3_MBM_LOCAL_EVENT_ID));
-}
-
 /*
  * __resctrl_sched_in() - Writes the task's CLOSid/RMID to IA32_PQR_MSR
  *
diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
index 6dd2a74cf3ec..ff89a0ca130e 100644
--- a/fs/resctrl/internal.h
+++ b/fs/resctrl/internal.h
@@ -70,15 +70,19 @@ static inline struct rdt_fs_context *rdt_fc2context(struct fs_context *fc)
  * @evtid:		event id
  * @name:		name of the event
  * @configurable:	true if the event is configurable
+ * @enabled:		true if the event is enabled
  * @list:		entry in &rdt_resource->evt_list
  */
 struct mon_evt {
 	enum resctrl_event_id	evtid;
 	char			*name;
 	bool			configurable;
+	bool			enabled;
 	struct list_head	list;
 };
 
+extern struct mon_evt mon_event_all[QOS_NUM_EVENTS];
+
 /**
  * struct mon_data - Monitoring details for each event file.
  * @list:            Member of the global @mon_data_kn_priv_list list.
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 224bed28f341..819bc7a09327 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -401,13 +401,13 @@ static int arch_domain_mbm_alloc(u32 num_rmid, struct rdt_hw_mon_domain *hw_dom)
 {
 	size_t tsize;
 
-	if (resctrl_arch_is_mbm_total_enabled()) {
+	if (resctrl_is_mon_event_enabled(QOS_L3_MBM_TOTAL_EVENT_ID)) {
 		tsize = sizeof(*hw_dom->arch_mbm_total);
 		hw_dom->arch_mbm_total = kcalloc(num_rmid, tsize, GFP_KERNEL);
 		if (!hw_dom->arch_mbm_total)
 			return -ENOMEM;
 	}
-	if (resctrl_arch_is_mbm_local_enabled()) {
+	if (resctrl_is_mon_event_enabled(QOS_L3_MBM_LOCAL_EVENT_ID)) {
 		tsize = sizeof(*hw_dom->arch_mbm_local);
 		hw_dom->arch_mbm_local = kcalloc(num_rmid, tsize, GFP_KERNEL);
 		if (!hw_dom->arch_mbm_local) {
@@ -860,15 +860,22 @@ static __init bool get_rdt_alloc_resources(void)
 static __init bool get_rdt_mon_resources(void)
 {
 	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
+	bool ret = false;
 
-	if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC))
-		rdt_mon_features |= (1 << QOS_L3_OCCUP_EVENT_ID);
-	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL))
-		rdt_mon_features |= (1 << QOS_L3_MBM_TOTAL_EVENT_ID);
-	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_LOCAL))
-		rdt_mon_features |= (1 << QOS_L3_MBM_LOCAL_EVENT_ID);
+	if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
+		resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID);
+		ret = true;
+	}
+	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL)) {
+		resctrl_enable_mon_event(QOS_L3_MBM_TOTAL_EVENT_ID);
+		ret = true;
+	}
+	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_LOCAL)) {
+		resctrl_enable_mon_event(QOS_L3_MBM_LOCAL_EVENT_ID);
+		ret = true;
+	}
 
-	if (!rdt_mon_features)
+	if (!ret)
 		return false;
 
 	return !rdt_get_mon_l3_config(r);
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index 3fc4d9f56f0d..fda579251dba 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -30,11 +30,6 @@
  */
 bool rdt_mon_capable;
 
-/*
- * Global to indicate which monitoring events are enabled.
- */
-unsigned int rdt_mon_features;
-
 #define CF(cf)	((unsigned long)(1048576 * (cf) + 0.5))
 
 static int snc_nodes_per_l3_cache = 1;
@@ -206,11 +201,11 @@ void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mon_domain *
 {
 	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
 
-	if (resctrl_arch_is_mbm_total_enabled())
+	if (resctrl_is_mon_event_enabled(QOS_L3_MBM_TOTAL_EVENT_ID))
 		memset(hw_dom->arch_mbm_total, 0,
 		       sizeof(*hw_dom->arch_mbm_total) * r->num_rmid);
 
-	if (resctrl_arch_is_mbm_local_enabled())
+	if (resctrl_is_mon_event_enabled(QOS_L3_MBM_LOCAL_EVENT_ID))
 		memset(hw_dom->arch_mbm_local, 0,
 		       sizeof(*hw_dom->arch_mbm_local) * r->num_rmid);
 }
diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
index d56b78450a99..b17b60114afd 100644
--- a/fs/resctrl/ctrlmondata.c
+++ b/fs/resctrl/ctrlmondata.c
@@ -473,12 +473,12 @@ ssize_t rdtgroup_mba_mbps_event_write(struct kernfs_open_file *of,
 	rdt_last_cmd_clear();
 
 	if (!strcmp(buf, "mbm_local_bytes")) {
-		if (resctrl_arch_is_mbm_local_enabled())
+		if (resctrl_is_mon_event_enabled(QOS_L3_MBM_LOCAL_EVENT_ID))
 			rdtgrp->mba_mbps_event = QOS_L3_MBM_LOCAL_EVENT_ID;
 		else
 			ret = -EINVAL;
 	} else if (!strcmp(buf, "mbm_total_bytes")) {
-		if (resctrl_arch_is_mbm_total_enabled())
+		if (resctrl_is_mon_event_enabled(QOS_L3_MBM_TOTAL_EVENT_ID))
 			rdtgrp->mba_mbps_event = QOS_L3_MBM_TOTAL_EVENT_ID;
 		else
 			ret = -EINVAL;
diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
index bde2801289d3..7de4e219dba3 100644
--- a/fs/resctrl/monitor.c
+++ b/fs/resctrl/monitor.c
@@ -336,7 +336,7 @@ void free_rmid(u32 closid, u32 rmid)
 
 	entry = __rmid_entry(idx);
 
-	if (resctrl_arch_is_llc_occupancy_enabled())
+	if (resctrl_is_mon_event_enabled(QOS_L3_OCCUP_EVENT_ID))
 		add_rmid_to_limbo(entry);
 	else
 		list_add_tail(&entry->list, &rmid_free_lru);
@@ -635,10 +635,10 @@ static void mbm_update(struct rdt_resource *r, struct rdt_mon_domain *d,
 	 * This is protected from concurrent reads from user as both
 	 * the user and overflow handler hold the global mutex.
 	 */
-	if (resctrl_arch_is_mbm_total_enabled())
+	if (resctrl_is_mon_event_enabled(QOS_L3_MBM_TOTAL_EVENT_ID))
 		mbm_update_one_event(r, d, closid, rmid, QOS_L3_MBM_TOTAL_EVENT_ID);
 
-	if (resctrl_arch_is_mbm_local_enabled())
+	if (resctrl_is_mon_event_enabled(QOS_L3_MBM_LOCAL_EVENT_ID))
 		mbm_update_one_event(r, d, closid, rmid, QOS_L3_MBM_LOCAL_EVENT_ID);
 }
 
@@ -842,20 +842,33 @@ static void dom_data_exit(struct rdt_resource *r)
 	mutex_unlock(&rdtgroup_mutex);
 }
 
-static struct mon_evt llc_occupancy_event = {
-	.name		= "llc_occupancy",
-	.evtid		= QOS_L3_OCCUP_EVENT_ID,
+struct mon_evt mon_event_all[QOS_NUM_EVENTS] = {
+	[QOS_L3_OCCUP_EVENT_ID] = {
+		.name	= "llc_occupancy",
+		.evtid	= QOS_L3_OCCUP_EVENT_ID,
+	},
+	[QOS_L3_MBM_TOTAL_EVENT_ID] = {
+		.name	= "mbm_total_bytes",
+		.evtid	= QOS_L3_MBM_TOTAL_EVENT_ID,
+	},
+	[QOS_L3_MBM_LOCAL_EVENT_ID] = {
+		.name	= "mbm_local_bytes",
+		.evtid	= QOS_L3_MBM_LOCAL_EVENT_ID,
+	},
 };
 
-static struct mon_evt mbm_total_event = {
-	.name		= "mbm_total_bytes",
-	.evtid		= QOS_L3_MBM_TOTAL_EVENT_ID,
-};
+void resctrl_enable_mon_event(enum resctrl_event_id evtid)
+{
+	if (WARN_ON_ONCE(evtid >= QOS_NUM_EVENTS))
+		return;
 
-static struct mon_evt mbm_local_event = {
-	.name		= "mbm_local_bytes",
-	.evtid		= QOS_L3_MBM_LOCAL_EVENT_ID,
-};
+	mon_event_all[evtid].enabled = true;
+}
+
+bool resctrl_is_mon_event_enabled(enum resctrl_event_id evtid)
+{
+	return evtid < QOS_NUM_EVENTS && mon_event_all[evtid].enabled;
+}
 
 /*
  * Initialize the event list for the resource.
@@ -866,14 +879,13 @@ static struct mon_evt mbm_local_event = {
  */
 static void l3_mon_evt_init(struct rdt_resource *r)
 {
+	enum resctrl_event_id evt;
+
 	INIT_LIST_HEAD(&r->evt_list);
 
-	if (resctrl_arch_is_llc_occupancy_enabled())
-		list_add_tail(&llc_occupancy_event.list, &r->evt_list);
-	if (resctrl_arch_is_mbm_total_enabled())
-		list_add_tail(&mbm_total_event.list, &r->evt_list);
-	if (resctrl_arch_is_mbm_local_enabled())
-		list_add_tail(&mbm_local_event.list, &r->evt_list);
+	for (evt = 0; evt < QOS_NUM_EVENTS; evt++)
+		if (mon_event_all[evt].enabled)
+			list_add_tail(&mon_event_all[evt].list, &r->evt_list);
 }
 
 /**
@@ -903,19 +915,19 @@ int resctrl_mon_resource_init(void)
 	l3_mon_evt_init(r);
 
 	if (resctrl_arch_is_evt_configurable(QOS_L3_MBM_TOTAL_EVENT_ID)) {
-		mbm_total_event.configurable = true;
+		mon_event_all[QOS_L3_MBM_TOTAL_EVENT_ID].configurable = true;
 		resctrl_file_fflags_init("mbm_total_bytes_config",
 					 RFTYPE_MON_INFO | RFTYPE_RES_CACHE);
 	}
 	if (resctrl_arch_is_evt_configurable(QOS_L3_MBM_LOCAL_EVENT_ID)) {
-		mbm_local_event.configurable = true;
+		mon_event_all[QOS_L3_MBM_LOCAL_EVENT_ID].configurable = true;
 		resctrl_file_fflags_init("mbm_local_bytes_config",
 					 RFTYPE_MON_INFO | RFTYPE_RES_CACHE);
 	}
 
-	if (resctrl_arch_is_mbm_local_enabled())
+	if (resctrl_is_mon_event_enabled(QOS_L3_MBM_LOCAL_EVENT_ID))
 		mba_mbps_default_event = QOS_L3_MBM_LOCAL_EVENT_ID;
-	else if (resctrl_arch_is_mbm_total_enabled())
+	else if (resctrl_is_mon_event_enabled(QOS_L3_MBM_TOTAL_EVENT_ID))
 		mba_mbps_default_event = QOS_L3_MBM_TOTAL_EVENT_ID;
 
 	return 0;
diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
index 07f91d18c1b8..4a092c305255 100644
--- a/fs/resctrl/rdtgroup.c
+++ b/fs/resctrl/rdtgroup.c
@@ -123,8 +123,8 @@ void rdt_staged_configs_clear(void)
 
 static bool resctrl_is_mbm_enabled(void)
 {
-	return (resctrl_arch_is_mbm_total_enabled() ||
-		resctrl_arch_is_mbm_local_enabled());
+	return (resctrl_is_mon_event_enabled(QOS_L3_MBM_TOTAL_EVENT_ID) ||
+		resctrl_is_mon_event_enabled(QOS_L3_MBM_LOCAL_EVENT_ID));
 }
 
 static bool resctrl_is_mbm_event(int e)
@@ -196,7 +196,7 @@ static int closid_alloc(void)
 	lockdep_assert_held(&rdtgroup_mutex);
 
 	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) &&
-	    resctrl_arch_is_llc_occupancy_enabled()) {
+	    resctrl_is_mon_event_enabled(QOS_L3_OCCUP_EVENT_ID)) {
 		cleanest_closid = resctrl_find_cleanest_closid();
 		if (cleanest_closid < 0)
 			return cleanest_closid;
@@ -4046,7 +4046,7 @@ void resctrl_offline_mon_domain(struct rdt_resource *r, struct rdt_mon_domain *d
 
 	if (resctrl_is_mbm_enabled())
 		cancel_delayed_work(&d->mbm_over);
-	if (resctrl_arch_is_llc_occupancy_enabled() && has_busy_rmid(d)) {
+	if (resctrl_is_mon_event_enabled(QOS_L3_OCCUP_EVENT_ID) && has_busy_rmid(d)) {
 		/*
 		 * When a package is going down, forcefully
 		 * decrement rmid->ebusy. There is no way to know
@@ -4082,12 +4082,12 @@ static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_mon_domain
 	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
 	size_t tsize;
 
-	if (resctrl_arch_is_llc_occupancy_enabled()) {
+	if (resctrl_is_mon_event_enabled(QOS_L3_OCCUP_EVENT_ID)) {
 		d->rmid_busy_llc = bitmap_zalloc(idx_limit, GFP_KERNEL);
 		if (!d->rmid_busy_llc)
 			return -ENOMEM;
 	}
-	if (resctrl_arch_is_mbm_total_enabled()) {
+	if (resctrl_is_mon_event_enabled(QOS_L3_MBM_TOTAL_EVENT_ID)) {
 		tsize = sizeof(*d->mbm_total);
 		d->mbm_total = kcalloc(idx_limit, tsize, GFP_KERNEL);
 		if (!d->mbm_total) {
@@ -4095,7 +4095,7 @@ static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_mon_domain
 			return -ENOMEM;
 		}
 	}
-	if (resctrl_arch_is_mbm_local_enabled()) {
+	if (resctrl_is_mon_event_enabled(QOS_L3_MBM_LOCAL_EVENT_ID)) {
 		tsize = sizeof(*d->mbm_local);
 		d->mbm_local = kcalloc(idx_limit, tsize, GFP_KERNEL);
 		if (!d->mbm_local) {
@@ -4140,7 +4140,7 @@ int resctrl_online_mon_domain(struct rdt_resource *r, struct rdt_mon_domain *d)
 					   RESCTRL_PICK_ANY_CPU);
 	}
 
-	if (resctrl_arch_is_llc_occupancy_enabled())
+	if (resctrl_is_mon_event_enabled(QOS_L3_OCCUP_EVENT_ID))
 		INIT_DELAYED_WORK(&d->cqm_limbo, cqm_handle_limbo);
 
 	/*
@@ -4215,7 +4215,7 @@ void resctrl_offline_cpu(unsigned int cpu)
 			cancel_delayed_work(&d->mbm_over);
 			mbm_setup_overflow_handler(d, 0, cpu);
 		}
-		if (resctrl_arch_is_llc_occupancy_enabled() &&
+		if (resctrl_is_mon_event_enabled(QOS_L3_OCCUP_EVENT_ID) &&
 		    cpu == d->cqm_work_cpu && has_busy_rmid(d)) {
 			cancel_delayed_work(&d->cqm_limbo);
 			cqm_setup_limbo_handler(d, 0, cpu);
-- 
2.48.1

Hi Tony,

On 4/28/25 5:33 PM, Tony Luck wrote:
> The fs/arch boundary is a little muddy for adding new monitor features.

It is not possible to accurately interpret what is meant with "little muddy".
Please add specific information that can be verified/reasoned about.

> 
> Clean it up by making the mon_evt structure the source of all information
> about each event. In this case replace the bitmap of enabled monitor
> features with an "enabled" bit in the mon_evt structure.

bit -> boolean?

> 
> Change architecture code to inform file system code which events are
> available on a system with resctrl_enable_mon_event().

(nit: no need to mention that a patch changes code, it should be implied.)

This could be, "An architecture uses resctrl_enable_mon_event() to inform
resctrl fs which events are enabled on the system."

(I think we need to be cautious about the "available" vs "enabled"
distinction.)

> 
> Replace the event and architecture specific:
> 	resctrl_arch_is_llc_occupancy_enabled()
> 	resctrl_arch_is_mbm_total_enabled()
> 	resctrl_arch_is_mbm_local_enabled()
> functions with calls to resctrl_is_mon_event_enabled() with the
> appropriate QOS_L3_* enum resctrl_event_id.

No mention or motivation for the new array. I think the new array is an
improvement and now it begs the question whether rdt_resource::evt_list is
still needed? It seems to me that any usage of rdt_resource::evt_list can
use the new mon_event_all[] instead?
With struct mon_evt being independent like before this
patch it almost seems as though it prepared for multiple resources to
support the same event (do you know history here?). This appears to already
be thwarted by rdt_mon_features though ... although theoretically it could
have been "rdt_l3_mon_features".
Even so, with patch #4 adding the resource ID all event information is
centralized. Only potential issue may be if multiple resources use the
same event ... but since the existing event IDs already have resource
name embedded this does not seem to be of concern?

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---

...

> @@ -866,14 +879,13 @@ static struct mon_evt mbm_local_event = {
>   */
>  static void l3_mon_evt_init(struct rdt_resource *r)
>  {
> +	enum resctrl_event_id evt;
> +
>  	INIT_LIST_HEAD(&r->evt_list);
>  
> -	if (resctrl_arch_is_llc_occupancy_enabled())
> -		list_add_tail(&llc_occupancy_event.list, &r->evt_list);
> -	if (resctrl_arch_is_mbm_total_enabled())
> -		list_add_tail(&mbm_total_event.list, &r->evt_list);
> -	if (resctrl_arch_is_mbm_local_enabled())
> -		list_add_tail(&mbm_local_event.list, &r->evt_list);
> +	for (evt = 0; evt < QOS_NUM_EVENTS; evt++)
> +		if (mon_event_all[evt].enabled)
> +			list_add_tail(&mon_event_all[evt].list, &r->evt_list);
>  }

This hunk can create confusion with it adding "all enabled events" to
a single resource. I understand that at this point only L3 supports monitoring
and this works ok, but in the context of this work it creates a caveat early
in series that needs to be fixed later (patch #4). This wrangling becomes
unnecessary if removing rdt_resource::evt_list.

Reinette

On Wed, May 07, 2025 at 08:28:56PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 4/28/25 5:33 PM, Tony Luck wrote:
> > The fs/arch boundary is a little muddy for adding new monitor features.
> 
> It is not possible to accurately interpret what is meant with "little muddy".
> Please add specific information that can be verified/reasoned about.

I'll work on something more descriptive/useful.

> > 
> > Clean it up by making the mon_evt structure the source of all information
> > about each event. In this case replace the bitmap of enabled monitor
> > features with an "enabled" bit in the mon_evt structure.
> 
> bit -> boolean?

Will fix ("bit" was left over from earlier implementation).

> > 
> > Change architecture code to inform file system code which events are
> > available on a system with resctrl_enable_mon_event().
> 
> (nit: no need to mention that a patch changes code, it should be implied.)
> 
> This could be, "An architecture uses resctrl_enable_mon_event() to inform
> resctrl fs which events are enabled on the system."

Will update with this.

> (I think we need to be cautious about the "available" vs "enabled"
> distinction.)

Maybe a comment above mon_event_all[]?

/*
 * All available events. Architecture code marks the ones that
 * are supported by a system using resctrl_enable_mon_event()
 * to set .enabled.
 */
struct mon_evt mon_event_all[QOS_NUM_EVENTS] = {

> > 
> > Replace the event and architecture specific:
> > 	resctrl_arch_is_llc_occupancy_enabled()
> > 	resctrl_arch_is_mbm_total_enabled()
> > 	resctrl_arch_is_mbm_local_enabled()
> > functions with calls to resctrl_is_mon_event_enabled() with the
> > appropriate QOS_L3_* enum resctrl_event_id.
> 
> No mention or motivation for the new array. I think the new array is an
> improvement and now it begs the question whether rdt_resource::evt_list is
> still needed? It seems to me that any usage of rdt_resource::evt_list can
> use the new mon_event_all[] instead?

Good suggestion. rdt_resource::evt_list can indeed be dropped. A
standalone patch to do so reduces lines of code:

 include/linux/resctrl.h |  2 --
 fs/resctrl/internal.h   |  2 --
 fs/resctrl/monitor.c    | 18 +-----------------
 fs/resctrl/rdtgroup.c   | 11 ++++++-----
 4 files changed, 7 insertions(+), 26 deletions(-)

But I'll merge into one of the early patches to avoid adding new code to create
the evt_list and then delete it again.

> With struct mon_evt being independent like before this
> patch it almost seems as though it prepared for multiple resources to
> support the same event (do you know history here?). This appears to already
> be thwarted by rdt_mon_features though ... although theoretically it could
> have been "rdt_l3_mon_features".
> Even so, with patch #4 adding the resource ID all event information is
> centralized. Only potential issue may be if multiple resources use the
> same event ... but since the existing event IDs already have resource
> name embedded this does not seem to be of concern?

The existing evt_list approach would corrupt the lists if the same event
were added to multiple resources. Without the list this becomes
possible, but seems neither desirable, nor useful.

I will add a warning to resctrl_enable_mon_event() if architecture
code tries to enable an already enabled event.
> 
> > 
> > Signed-off-by: Tony Luck <tony.luck@intel.com>
> > ---
> 
> ...
> 
> > @@ -866,14 +879,13 @@ static struct mon_evt mbm_local_event = {
> >   */
> >  static void l3_mon_evt_init(struct rdt_resource *r)
> >  {
> > +	enum resctrl_event_id evt;
> > +
> >  	INIT_LIST_HEAD(&r->evt_list);
> >  
> > -	if (resctrl_arch_is_llc_occupancy_enabled())
> > -		list_add_tail(&llc_occupancy_event.list, &r->evt_list);
> > -	if (resctrl_arch_is_mbm_total_enabled())
> > -		list_add_tail(&mbm_total_event.list, &r->evt_list);
> > -	if (resctrl_arch_is_mbm_local_enabled())
> > -		list_add_tail(&mbm_local_event.list, &r->evt_list);
> > +	for (evt = 0; evt < QOS_NUM_EVENTS; evt++)
> > +		if (mon_event_all[evt].enabled)
> > +			list_add_tail(&mon_event_all[evt].list, &r->evt_list);
> >  }
> 
> This hunk can create confusion with it adding "all enabled events" to
> a single resource. I understand that at this point only L3 supports monitoring
> and this works ok, but in the context of this work it creates a caveat early
> in series that needs to be fixed later (patch #4). This wrangling becomes
> unnecessary if removing rdt_resource::evt_list.

I'll see if I can get a clean sequence between these patches to avoid
this confusion. Maybe evt_list removal needs to happen here.
> 
> Reinette

-Tony

Hi Tony,

On 5/8/25 11:32 AM, Luck, Tony wrote:
> On Wed, May 07, 2025 at 08:28:56PM -0700, Reinette Chatre wrote:
>> On 4/28/25 5:33 PM, Tony Luck wrote:

...

>>> Change architecture code to inform file system code which events are
>>> available on a system with resctrl_enable_mon_event().
>>
>> (nit: no need to mention that a patch changes code, it should be implied.)
>>
>> This could be, "An architecture uses resctrl_enable_mon_event() to inform
>> resctrl fs which events are enabled on the system."
> 
> Will update with this.
> 
>> (I think we need to be cautious about the "available" vs "enabled"
>> distinction.)
> 
> Maybe a comment above mon_event_all[]?

Good idea.

> 
> /*
>  * All available events. Architecture code marks the ones that

I think "available" may be interpreted differently by people.
How about "All known events."? 

>  * are supported by a system using resctrl_enable_mon_event()
>  * to set .enabled.
>  */
> struct mon_evt mon_event_all[QOS_NUM_EVENTS] = {
> 
>>>
>>> Replace the event and architecture specific:
>>> 	resctrl_arch_is_llc_occupancy_enabled()
>>> 	resctrl_arch_is_mbm_total_enabled()
>>> 	resctrl_arch_is_mbm_local_enabled()
>>> functions with calls to resctrl_is_mon_event_enabled() with the
>>> appropriate QOS_L3_* enum resctrl_event_id.
>>
>> No mention or motivation for the new array. I think the new array is an
>> improvement and now it begs the question whether rdt_resource::evt_list is
>> still needed? It seems to me that any usage of rdt_resource::evt_list can
>> use the new mon_event_all[] instead?
> 
> Good suggestion. rdt_resource::evt_list can indeed be dropped. A
> standalone patch to do so reduces lines of code:
> 
>  include/linux/resctrl.h |  2 --
>  fs/resctrl/internal.h   |  2 --
>  fs/resctrl/monitor.c    | 18 +-----------------
>  fs/resctrl/rdtgroup.c   | 11 ++++++-----
>  4 files changed, 7 insertions(+), 26 deletions(-)
> 
> But I'll merge into one of the early patches to avoid adding new code to create
> the evt_list and then delete it again.

Thanks for considering.

> 
>> With struct mon_evt being independent like before this
>> patch it almost seems as though it prepared for multiple resources to
>> support the same event (do you know history here?). This appears to already
>> be thwarted by rdt_mon_features though ... although theoretically it could
>> have been "rdt_l3_mon_features".
>> Even so, with patch #4 adding the resource ID all event information is
>> centralized. Only potential issue may be if multiple resources use the
>> same event ... but since the existing event IDs already have resource
>> name embedded this does not seem to be of concern?
> 
> The existing evt_list approach would corrupt the lists if the same event
> were added to multiple resources. Without the list this becomes
> possible, but seems neither desirable, nor useful.

ack. With an event array indexed by event ID it would also take some additional
changes to support.

> 
> I will add a warning to resctrl_enable_mon_event() if architecture
> code tries to enable an already enabled event.

Thank you very much.

>>
>>>
>>> Signed-off-by: Tony Luck <tony.luck@intel.com>
>>> ---
>>
>> ...
>>
>>> @@ -866,14 +879,13 @@ static struct mon_evt mbm_local_event = {
>>>   */
>>>  static void l3_mon_evt_init(struct rdt_resource *r)
>>>  {
>>> +	enum resctrl_event_id evt;
>>> +
>>>  	INIT_LIST_HEAD(&r->evt_list);
>>>  
>>> -	if (resctrl_arch_is_llc_occupancy_enabled())
>>> -		list_add_tail(&llc_occupancy_event.list, &r->evt_list);
>>> -	if (resctrl_arch_is_mbm_total_enabled())
>>> -		list_add_tail(&mbm_total_event.list, &r->evt_list);
>>> -	if (resctrl_arch_is_mbm_local_enabled())
>>> -		list_add_tail(&mbm_local_event.list, &r->evt_list);
>>> +	for (evt = 0; evt < QOS_NUM_EVENTS; evt++)
>>> +		if (mon_event_all[evt].enabled)
>>> +			list_add_tail(&mon_event_all[evt].list, &r->evt_list);
>>>  }
>>
>> This hunk can create confusion with it adding "all enabled events" to
>> a single resource. I understand that at this point only L3 supports monitoring
>> and this works ok, but in the context of this work it creates a caveat early
>> in series that needs to be fixed later (patch #4). This wrangling becomes
>> unnecessary if removing rdt_resource::evt_list.
> 
> I'll see if I can get a clean sequence between these patches to avoid
> this confusion. Maybe evt_list removal needs to happen here.

Thank you.

Reinette