Resctrl file system code was built with the assumption that monitor
events can only be read from a CPU in the cpumast_t set for each
domain. This was true for x86 events accessed with an MSR interface,
but may not be true for other access methods such as MMIO.

Add a flag to each instance of struct mon_evt that can be set by
architecture code to indicate there is no restriction on which
CPU can read the event counter.

Change struct mon_data and struct rmid_read to have a pointer to
the struct mon_evt instead of the event id.

Add an extra argument to resctrl_enable_mon_event() so architecture
code can indicate which events can be read on any CPU when enabling
the event.

Bypass all the smp_call*() code for events that can be read on any CPU
and call mon_event_count() directly from mon_event_read().

Skip checks in __mon_event_count() that the read is being done from
a CPU in the correct domain or cache scope.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 include/linux/resctrl.h            |  2 +-
 fs/resctrl/internal.h              | 12 +++++++-----
 arch/x86/kernel/cpu/resctrl/core.c |  6 +++---
 fs/resctrl/ctrlmondata.c           | 24 +++++++++++++++---------
 fs/resctrl/monitor.c               | 26 ++++++++++++++------------
 fs/resctrl/rdtgroup.c              |  6 +++---
 6 files changed, 43 insertions(+), 33 deletions(-)

diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index cd7881313d4e..4af5e8d30193 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -379,7 +379,7 @@ 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);
+void resctrl_enable_mon_event(enum resctrl_event_id evtid, bool any_cpu);
 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/fs/resctrl/internal.h b/fs/resctrl/internal.h
index 759768e2a2a8..d8aa69b42c74 100644
--- a/fs/resctrl/internal.h
+++ b/fs/resctrl/internal.h
@@ -72,6 +72,7 @@ static inline struct rdt_fs_context *rdt_fc2context(struct fs_context *fc)
  * @name:		name of the event
  * @configurable:	true if the event is configurable
  * @enabled:		true if the event is enabled
+ * @any_cpu:		true if the event can be read from any CPU
  * @list:		entry in &rdt_resource->evt_list
  */
 struct mon_evt {
@@ -80,6 +81,7 @@ struct mon_evt {
 	char			*name;
 	bool			configurable;
 	bool			enabled;
+	bool			any_cpu;
 	struct list_head	list;
 };
 
@@ -89,7 +91,7 @@ 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.
  * @rid:             Resource id associated with the event file.
- * @evtid:           Event id associated with the event file.
+ * @evt:             Event associated with the event file.
  * @sum:             Set when event must be summed across multiple
  *                   domains.
  * @domid:           When @sum is zero this is the domain to which
@@ -103,7 +105,7 @@ extern struct mon_evt mon_event_all[QOS_NUM_EVENTS];
 struct mon_data {
 	struct list_head	list;
 	enum resctrl_res_level	rid;
-	enum resctrl_event_id	evtid;
+	struct mon_evt		*evt;
 	int			domid;
 	bool			sum;
 };
@@ -116,7 +118,7 @@ struct mon_data {
  * @r:	   Resource describing the properties of the event being read.
  * @d:	   Domain that the counter should be read from. If NULL then sum all
  *	   domains in @r sharing L3 @ci.id
- * @evtid: Which monitor event to read.
+ * @evt:   Which monitor event to read.
  * @first: Initialize MBM counter when true.
  * @ci:    Cacheinfo for L3. Only set when @d is NULL. Used when summing domains.
  * @err:   Error encountered when reading counter.
@@ -130,7 +132,7 @@ struct rmid_read {
 	struct rdtgroup		*rgrp;
 	struct rdt_resource	*r;
 	struct rdt_l3_mon_domain	*d;
-	enum resctrl_event_id	evtid;
+	struct mon_evt		*evt;
 	bool			first;
 	struct cacheinfo	*ci;
 	int			err;
@@ -366,7 +368,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg);
 
 void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
 		    struct rdt_l3_mon_domain *d, struct rdtgroup *rdtgrp,
-		    cpumask_t *cpumask, int evtid, int first);
+		    cpumask_t *cpumask, struct mon_evt *evt, int first);
 
 int resctrl_mon_resource_init(void);
 
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 01843dd0b8b7..58bc218070e2 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -877,15 +877,15 @@ static __init bool get_rdt_mon_resources(void)
 	bool ret = false;
 
 	if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
-		resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID);
+		resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID, false);
 		ret = true;
 	}
 	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL)) {
-		resctrl_enable_mon_event(QOS_L3_MBM_TOTAL_EVENT_ID);
+		resctrl_enable_mon_event(QOS_L3_MBM_TOTAL_EVENT_ID, false);
 		ret = true;
 	}
 	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_LOCAL)) {
-		resctrl_enable_mon_event(QOS_L3_MBM_LOCAL_EVENT_ID);
+		resctrl_enable_mon_event(QOS_L3_MBM_LOCAL_EVENT_ID, false);
 		ret = true;
 	}
 
diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
index 8c0f6d229130..7a2957b9c13e 100644
--- a/fs/resctrl/ctrlmondata.c
+++ b/fs/resctrl/ctrlmondata.c
@@ -527,7 +527,7 @@ struct rdt_domain_hdr *resctrl_find_domain(struct list_head *h, int id,
 
 void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
 		    struct rdt_l3_mon_domain *d, struct rdtgroup *rdtgrp,
-		    cpumask_t *cpumask, int evtid, int first)
+		    cpumask_t *cpumask, struct mon_evt *evt, int first)
 {
 	int cpu;
 
@@ -538,16 +538,21 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
 	 * Setup the parameters to pass to mon_event_count() to read the data.
 	 */
 	rr->rgrp = rdtgrp;
-	rr->evtid = evtid;
+	rr->evt = evt;
 	rr->r = r;
 	rr->d = d;
 	rr->first = first;
-	rr->arch_mon_ctx = resctrl_arch_mon_ctx_alloc(r, evtid);
+	rr->arch_mon_ctx = resctrl_arch_mon_ctx_alloc(r, evt->evtid);
 	if (IS_ERR(rr->arch_mon_ctx)) {
 		rr->err = -EINVAL;
 		return;
 	}
 
+	if (evt->any_cpu) {
+		mon_event_count(rr);
+		goto done;
+	}
+
 	cpu = cpumask_any_housekeeping(cpumask, RESCTRL_PICK_ANY_CPU);
 
 	/*
@@ -560,8 +565,8 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
 		smp_call_function_any(cpumask, mon_event_count, rr, 1);
 	else
 		smp_call_on_cpu(cpu, smp_mon_event_count, rr, false);
-
-	resctrl_arch_mon_ctx_free(r, evtid, rr->arch_mon_ctx);
+done:
+	resctrl_arch_mon_ctx_free(r, evt->evtid, rr->arch_mon_ctx);
 }
 
 int rdtgroup_mondata_show(struct seq_file *m, void *arg)
@@ -570,7 +575,8 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
 	struct rdt_domain_hdr *hdr;
 	struct rmid_read rr = {0};
 	struct rdt_l3_mon_domain *d;
-	u32 resid, evtid, domid;
+	struct mon_evt *evt;
+	u32 resid, domid;
 	struct rdtgroup *rdtgrp;
 	struct rdt_resource *r;
 	struct mon_data *md;
@@ -590,7 +596,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
 
 	resid = md->rid;
 	domid = md->domid;
-	evtid = md->evtid;
+	evt = md->evt;
 	r = resctrl_arch_get_resource(resid);
 
 	if (md->sum) {
@@ -604,7 +610,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
 			if (d->ci->id == domid) {
 				rr.ci = d->ci;
 				mon_event_read(&rr, r, NULL, rdtgrp,
-					       &d->ci->shared_cpu_map, evtid, false);
+					       &d->ci->shared_cpu_map, evt, false);
 				goto checkresult;
 			}
 		}
@@ -621,7 +627,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
 			goto out;
 		}
 		d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
-		mon_event_read(&rr, r, d, rdtgrp, &d->hdr.cpu_mask, evtid, false);
+		mon_event_read(&rr, r, d, rdtgrp, &d->hdr.cpu_mask, evt, false);
 	}
 
 checkresult:
diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
index 19cba29452b7..e903d3c076ee 100644
--- a/fs/resctrl/monitor.c
+++ b/fs/resctrl/monitor.c
@@ -365,19 +365,19 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
 	u64 tval = 0;
 
 	if (rr->first) {
-		resctrl_arch_reset_rmid(rr->r, rr->d, closid, rmid, rr->evtid);
-		m = get_mbm_state(rr->d, closid, rmid, rr->evtid);
+		resctrl_arch_reset_rmid(rr->r, rr->d, closid, rmid, rr->evt->evtid);
+		m = get_mbm_state(rr->d, closid, rmid, rr->evt->evtid);
 		if (m)
 			memset(m, 0, sizeof(struct mbm_state));
 		return 0;
 	}
 
 	if (rr->d) {
-		/* Reading a single domain, must be on a CPU in that domain. */
-		if (!cpumask_test_cpu(cpu, &rr->d->hdr.cpu_mask))
+		/* Reading a single domain, must usually be on a CPU in that domain. */
+		if (!rr->evt->any_cpu && !cpumask_test_cpu(cpu, &rr->d->hdr.cpu_mask))
 			return -EINVAL;
 		rr->err = resctrl_arch_rmid_read(rr->r, rr->d, closid, rmid,
-						 rr->evtid, &tval, rr->arch_mon_ctx);
+						 rr->evt->evtid, &tval, rr->arch_mon_ctx);
 		if (rr->err)
 			return rr->err;
 
@@ -387,7 +387,7 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
 	}
 
 	/* Summing domains that share a cache, must be on a CPU for that cache. */
-	if (!cpumask_test_cpu(cpu, &rr->ci->shared_cpu_map))
+	if (!rr->evt->any_cpu && !cpumask_test_cpu(cpu, &rr->ci->shared_cpu_map))
 		return -EINVAL;
 
 	/*
@@ -402,7 +402,7 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
 		if (d->ci->id != rr->ci->id)
 			continue;
 		err = resctrl_arch_rmid_read(rr->r, d, closid, rmid,
-					     rr->evtid, &tval, rr->arch_mon_ctx);
+					     rr->evt->evtid, &tval, rr->arch_mon_ctx);
 		if (!err) {
 			rr->val += tval;
 			ret = 0;
@@ -432,7 +432,7 @@ static void mbm_bw_count(u32 closid, u32 rmid, struct rmid_read *rr)
 	u64 cur_bw, bytes, cur_bytes;
 	struct mbm_state *m;
 
-	m = get_mbm_state(rr->d, closid, rmid, rr->evtid);
+	m = get_mbm_state(rr->d, closid, rmid, rr->evt->evtid);
 	if (WARN_ON_ONCE(!m))
 		return;
 
@@ -603,12 +603,13 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_l3_mon_domain *dom_m
 static void mbm_update_one_event(struct rdt_resource *r, struct rdt_l3_mon_domain *d,
 				 u32 closid, u32 rmid, enum resctrl_event_id evtid)
 {
+	struct mon_evt *evt = &mon_event_all[evtid];
 	struct rmid_read rr = {0};
 
 	rr.r = r;
 	rr.d = d;
-	rr.evtid = evtid;
-	rr.arch_mon_ctx = resctrl_arch_mon_ctx_alloc(rr.r, rr.evtid);
+	rr.evt = evt;
+	rr.arch_mon_ctx = resctrl_arch_mon_ctx_alloc(rr.r, evtid);
 	if (IS_ERR(rr.arch_mon_ctx)) {
 		pr_warn_ratelimited("Failed to allocate monitor context: %ld",
 				    PTR_ERR(rr.arch_mon_ctx));
@@ -624,7 +625,7 @@ static void mbm_update_one_event(struct rdt_resource *r, struct rdt_l3_mon_domai
 	if (is_mba_sc(NULL))
 		mbm_bw_count(closid, rmid, &rr);
 
-	resctrl_arch_mon_ctx_free(rr.r, rr.evtid, rr.arch_mon_ctx);
+	resctrl_arch_mon_ctx_free(rr.r, evtid, rr.arch_mon_ctx);
 }
 
 static void mbm_update(struct rdt_resource *r, struct rdt_l3_mon_domain *d,
@@ -859,12 +860,13 @@ struct mon_evt mon_event_all[QOS_NUM_EVENTS] = {
 	},
 };
 
-void resctrl_enable_mon_event(enum resctrl_event_id evtid)
+void resctrl_enable_mon_event(enum resctrl_event_id evtid, bool any_cpu)
 {
 	if (WARN_ON_ONCE(evtid >= QOS_NUM_EVENTS))
 		return;
 
 	mon_event_all[evtid].enabled = true;
+	mon_event_all[evtid].any_cpu = any_cpu;
 }
 
 bool resctrl_is_mon_event_enabled(enum resctrl_event_id evtid)
diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
index d2f9361694b6..d16bb05fafe8 100644
--- a/fs/resctrl/rdtgroup.c
+++ b/fs/resctrl/rdtgroup.c
@@ -2897,7 +2897,7 @@ static struct mon_data *mon_get_kn_priv(int rid, int domid,
 
 	list_for_each_entry(priv, &mon_data_kn_priv_list, list) {
 		if (priv->rid == rid && priv->domid == domid &&
-		    priv->sum == do_sum && priv->evtid == mevt->evtid)
+		    priv->sum == do_sum && priv->evt == mevt)
 			return priv;
 	}
 
@@ -2908,7 +2908,7 @@ static struct mon_data *mon_get_kn_priv(int rid, int domid,
 	priv->rid = rid;
 	priv->domid = domid;
 	priv->sum = do_sum;
-	priv->evtid = mevt->evtid;
+	priv->evt = mevt;
 	list_add_tail(&priv->list, &mon_data_kn_priv_list);
 
 	return priv;
@@ -3082,7 +3082,7 @@ static int mon_add_all_files(struct kernfs_node *kn, struct rdt_domain_hdr *hdr,
 			return ret;
 
 		if (r->rid == RDT_RESOURCE_L3 && !do_sum && resctrl_is_mbm_event(mevt->evtid))
-			mon_event_read(&rr, r, d, prgrp, &hdr->cpu_mask, mevt->evtid, true);
+			mon_event_read(&rr, r, d, prgrp, &hdr->cpu_mask, mevt, true);
 	}
 
 	return 0;
-- 
2.48.1

Hi Tony,

On 4/29/2025 8:33 AM, Tony Luck wrote:
> Resctrl file system code was built with the assumption that monitor
> events can only be read from a CPU in the cpumast_t set for each
> domain. This was true for x86 events accessed with an MSR interface,
> but may not be true for other access methods such as MMIO.
> 
> Add a flag to each instance of struct mon_evt that can be set by
> architecture code to indicate there is no restriction on which
> CPU can read the event counter.
> 
> Change struct mon_data and struct rmid_read to have a pointer to
> the struct mon_evt instead of the event id.
> 
> Add an extra argument to resctrl_enable_mon_event() so architecture
> code can indicate which events can be read on any CPU when enabling
> the event.
> 
> Bypass all the smp_call*() code for events that can be read on any CPU
> and call mon_event_count() directly from mon_event_read().
> 
> Skip checks in __mon_event_count() that the read is being done from
> a CPU in the correct domain or cache scope.
> 

Since __mon_event_count() was supposed to run in atomic context, the
smp_processor_id() would not report any warning previously. After
this change, if the evt->any_cpu is true, we read the telemetry counter 
directly without IPI involved and in non-atomic context, we might
get warning like below:
BUG: using smp_processor_id() in preemptible [00000000] code: mount/1595
caller is __mon_event_count+0x2e/0x1e0
2483 [ 2095.332850] Call Trace:
2484 [ 2095.332861]  <TASK>
2485 [ 2095.332872]  dump_stack_lvl+0x55/0x70
2486 [ 2095.332887]  check_preemption_disabled+0xbf/0xe0
2487 [ 2095.332902]  __mon_event_count+0x2e/0x1e0
2488 [ 2095.332918]  mon_event_count+0x2a/0xa0
2489 [ 2095.332934]  mon_add_all_files+0x202/0x270
2490 [ 2095.332953]  mkdir_mondata_subdir+0x1bf/0x1e0
2491 [ 2095.332970]  ? kcore_update_ram.isra.0+0x270/0x270
2492 [ 2095.332985]  mkdir_mondata_all+0x9d/0x100
2493 [ 2095.333000]  rdt_get_tree+0x336/0x5d0
2494 [ 2095.333014]  vfs_get_tree+0x26/0xf0
2495 [ 2095.333028]  do_new_mount+0x186/0x350
2496 [ 2095.333044]  __x64_sys_mount+0x101/0x130
2497 [ 2095.333061]  do_syscall_64+0x54/0xd70
2498 [ 2095.333075]  entry_SYSCALL_64_after_hwframe+0x76/0x7e

Maybe avoid getting the CPU at all in __mon_event_count() if
evt->any_cpu is true?

thanks,
Chenyu
diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
index d9364bee486e..32385c811a92 100644
--- a/fs/resctrl/monitor.c
+++ b/fs/resctrl/monitor.c
@@ -358,12 +358,15 @@ static struct mbm_state *get_mbm_state(struct 
rdt_l3_mon_domain *d, u32 closid,

  static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
  {
-       int cpu = smp_processor_id();
         struct rdt_l3_mon_domain *d;
         struct mbm_state *m;
-       int err, ret;
+       int err, ret, cpu;
         u64 tval = 0;

+       /*only CPU sensitive event read cares about which CPU to read 
from */
+       if (!rr->evt->any_cpu)
+               cpu = smp_processor_id();

tele

On Tue, May 13, 2025 at 11:19:23AM +0800, Chen, Yu C wrote:

Thanks for the bug report.

> get warning like below:
> BUG: using smp_processor_id() in preemptible [00000000] code: mount/1595
> caller is __mon_event_count+0x2e/0x1e0
> 2483 [ 2095.332850] Call Trace:
> 2484 [ 2095.332861]  <TASK>
> 2485 [ 2095.332872]  dump_stack_lvl+0x55/0x70
> 2486 [ 2095.332887]  check_preemption_disabled+0xbf/0xe0
> 2487 [ 2095.332902]  __mon_event_count+0x2e/0x1e0
> 2488 [ 2095.332918]  mon_event_count+0x2a/0xa0
> 2489 [ 2095.332934]  mon_add_all_files+0x202/0x270
> 2490 [ 2095.332953]  mkdir_mondata_subdir+0x1bf/0x1e0
> 2491 [ 2095.332970]  ? kcore_update_ram.isra.0+0x270/0x270
> 2492 [ 2095.332985]  mkdir_mondata_all+0x9d/0x100
> 2493 [ 2095.333000]  rdt_get_tree+0x336/0x5d0
> 2494 [ 2095.333014]  vfs_get_tree+0x26/0xf0
> 2495 [ 2095.333028]  do_new_mount+0x186/0x350
> 2496 [ 2095.333044]  __x64_sys_mount+0x101/0x130
> 2497 [ 2095.333061]  do_syscall_64+0x54/0xd70
> 2498 [ 2095.333075]  entry_SYSCALL_64_after_hwframe+0x76/0x7e

Hmmm. You are right, but I didn't see this. Perhaps it only shows
if CONFIG_DEBUG_PREEMPT is set?

> Maybe avoid getting the CPU at all in __mon_event_count() if
> evt->any_cpu is true?
> 
> thanks,
> Chenyu
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index d9364bee486e..32385c811a92 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -358,12 +358,15 @@ static struct mbm_state *get_mbm_state(struct
> rdt_l3_mon_domain *d, u32 closid,
> 
>  static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
>  {
> -       int cpu = smp_processor_id();
>         struct rdt_l3_mon_domain *d;
>         struct mbm_state *m;
> -       int err, ret;
> +       int err, ret, cpu;
>         u64 tval = 0;
> 
> +       /*only CPU sensitive event read cares about which CPU to read from
> */
> +       if (!rr->evt->any_cpu)
> +               cpu = smp_processor_id();
> 
> tele

I might fix with a helper just in case some compiler doesn't keep track
and issues a "may be used before set" warning.

-Tony


diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
index ddfc1c5f60d6..6041cb304624 100644
--- a/fs/resctrl/monitor.c
+++ b/fs/resctrl/monitor.c
@@ -356,9 +356,24 @@ static struct mbm_state *get_mbm_state(struct rdt_l3_mon_domain *d, u32 closid,
 	return states ? &states[idx] : NULL;
 }
 
+static bool cpu_on_wrong_domain(struct rmid_read *rr)
+{
+	cpumask_t *mask;
+
+	if (rr->evt->any_cpu)
+		return false;
+
+	/*
+	 * When reading from a specific domain the CPU must be in that
+	 * domain. Otherwise the CPU must be one that shares the cache.
+	 */
+	mask = rr->d ? &rr->d->hdr.cpu_mask : &rr->ci->shared_cpu_map;
+
+	return !cpumask_test_cpu(smp_processor_id(), mask);
+}
+
 static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
 {
-	int cpu = smp_processor_id();
 	struct rdt_l3_mon_domain *d;
 	struct mbm_state *m;
 	int err, ret;
@@ -373,11 +388,7 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
 	}
 
 	if (rr->d) {
-		/*
-		 * Unless this event can be read from any CPU, check
-		 * that execution is on a CPU in the domain.
-		 */
-		if (!rr->evt->any_cpu && !cpumask_test_cpu(cpu, &rr->d->hdr.cpu_mask))
+		if (cpu_on_wrong_domain(rr))
 			return -EINVAL;
 		rr->err = resctrl_arch_rmid_read(rr->r, rr->d, closid, rmid,
 						 rr->evt->evtid, &tval, rr->arch_mon_ctx);
@@ -389,11 +400,7 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
 		return 0;
 	}
 
-	/*
-	 * Unless this event can be read from any CPU, check that
-	 * execution is on a CPU that shares the cache.
-	 */
-	if (!rr->evt->any_cpu && !cpumask_test_cpu(cpu, &rr->ci->shared_cpu_map))
+	if (cpu_on_wrong_domain(rr))
 		return -EINVAL;
 
 	/*

On 5/14/2025 12:20 AM, Luck, Tony wrote:
> On Tue, May 13, 2025 at 11:19:23AM +0800, Chen, Yu C wrote:
> 
> Thanks for the bug report.
> 
>> get warning like below:
>> BUG: using smp_processor_id() in preemptible [00000000] code: mount/1595
>> caller is __mon_event_count+0x2e/0x1e0
>> 2483 [ 2095.332850] Call Trace:
>> 2484 [ 2095.332861]  <TASK>
>> 2485 [ 2095.332872]  dump_stack_lvl+0x55/0x70
>> 2486 [ 2095.332887]  check_preemption_disabled+0xbf/0xe0
>> 2487 [ 2095.332902]  __mon_event_count+0x2e/0x1e0
>> 2488 [ 2095.332918]  mon_event_count+0x2a/0xa0
>> 2489 [ 2095.332934]  mon_add_all_files+0x202/0x270
>> 2490 [ 2095.332953]  mkdir_mondata_subdir+0x1bf/0x1e0
>> 2491 [ 2095.332970]  ? kcore_update_ram.isra.0+0x270/0x270
>> 2492 [ 2095.332985]  mkdir_mondata_all+0x9d/0x100
>> 2493 [ 2095.333000]  rdt_get_tree+0x336/0x5d0
>> 2494 [ 2095.333014]  vfs_get_tree+0x26/0xf0
>> 2495 [ 2095.333028]  do_new_mount+0x186/0x350
>> 2496 [ 2095.333044]  __x64_sys_mount+0x101/0x130
>> 2497 [ 2095.333061]  do_syscall_64+0x54/0xd70
>> 2498 [ 2095.333075]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> Hmmm. You are right, but I didn't see this. Perhaps it only shows
> if CONFIG_DEBUG_PREEMPT is set?
> 

Yes, CONFIG_DEBUG_PREEMPT checks that.

>> Maybe avoid getting the CPU at all in __mon_event_count() if
>> evt->any_cpu is true?
>>
>> thanks,
>> Chenyu
>> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
>> index d9364bee486e..32385c811a92 100644
>> --- a/fs/resctrl/monitor.c
>> +++ b/fs/resctrl/monitor.c
>> @@ -358,12 +358,15 @@ static struct mbm_state *get_mbm_state(struct
>> rdt_l3_mon_domain *d, u32 closid,
>>
>>   static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
>>   {
>> -       int cpu = smp_processor_id();
>>          struct rdt_l3_mon_domain *d;
>>          struct mbm_state *m;
>> -       int err, ret;
>> +       int err, ret, cpu;
>>          u64 tval = 0;
>>
>> +       /*only CPU sensitive event read cares about which CPU to read from
>> */
>> +       if (!rr->evt->any_cpu)
>> +               cpu = smp_processor_id();
>>
>> tele
> 
> I might fix with a helper just in case some compiler doesn't keep track
> and issues a "may be used before set" warning.
> 

The following fix looks good to me. I'll apply it in the internal tree
to continue the test.

thanks,
Chenyu


> -Tony
> 
> 
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index ddfc1c5f60d6..6041cb304624 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -356,9 +356,24 @@ static struct mbm_state *get_mbm_state(struct rdt_l3_mon_domain *d, u32 closid,
>   	return states ? &states[idx] : NULL;
>   }
>   
> +static bool cpu_on_wrong_domain(struct rmid_read *rr)
> +{
> +	cpumask_t *mask;
> +
> +	if (rr->evt->any_cpu)
> +		return false;
> +
> +	/*
> +	 * When reading from a specific domain the CPU must be in that
> +	 * domain. Otherwise the CPU must be one that shares the cache.
> +	 */
> +	mask = rr->d ? &rr->d->hdr.cpu_mask : &rr->ci->shared_cpu_map;
> +
> +	return !cpumask_test_cpu(smp_processor_id(), mask);
> +}
> +
>   static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
>   {
> -	int cpu = smp_processor_id();
>   	struct rdt_l3_mon_domain *d;
>   	struct mbm_state *m;
>   	int err, ret;
> @@ -373,11 +388,7 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
>   	}
>   
>   	if (rr->d) {
> -		/*
> -		 * Unless this event can be read from any CPU, check
> -		 * that execution is on a CPU in the domain.
> -		 */
> -		if (!rr->evt->any_cpu && !cpumask_test_cpu(cpu, &rr->d->hdr.cpu_mask))
> +		if (cpu_on_wrong_domain(rr))
>   			return -EINVAL;
>   		rr->err = resctrl_arch_rmid_read(rr->r, rr->d, closid, rmid,
>   						 rr->evt->evtid, &tval, rr->arch_mon_ctx);
> @@ -389,11 +400,7 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
>   		return 0;
>   	}
>   
> -	/*
> -	 * Unless this event can be read from any CPU, check that
> -	 * execution is on a CPU that shares the cache.
> -	 */
> -	if (!rr->evt->any_cpu && !cpumask_test_cpu(cpu, &rr->ci->shared_cpu_map))
> +	if (cpu_on_wrong_domain(rr))
>   		return -EINVAL;
>   
>   	/*

Hi Tony,

How about: "fs/resctrl: Handle events that can be read from any CPU"

On 4/28/25 5:33 PM, Tony Luck wrote:
> Resctrl file system code was built with the assumption that monitor
> events can only be read from a CPU in the cpumast_t set for each

cpumast_t -> cpumask_t

> domain. This was true for x86 events accessed with an MSR interface,
> but may not be true for other access methods such as MMIO.

Please separate context and problem description into separate paragraphs.

> 
> Add a flag to each instance of struct mon_evt that can be set by
> architecture code to indicate there is no restriction on which
> CPU can read the event counter.
> 
> Change struct mon_data and struct rmid_read to have a pointer to
> the struct mon_evt instead of the event id.
> 
> Add an extra argument to resctrl_enable_mon_event() so architecture
> code can indicate which events can be read on any CPU when enabling
> the event.
> 
> Bypass all the smp_call*() code for events that can be read on any CPU
> and call mon_event_count() directly from mon_event_read().
> 
> Skip checks in __mon_event_count() that the read is being done from
> a CPU in the correct domain or cache scope.

hmmm ... this patch is bundling quite a few changes. Most are related
to the stated goal but I think separating out the change to 
struct mon_data and struct rmid_read to have a pointer to struct mon_evt
will make it easier to recognize what it takes to support the
stated requirement of needing to support this new style of events.

...

> @@ -570,7 +575,8 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  	struct rdt_domain_hdr *hdr;
>  	struct rmid_read rr = {0};
>  	struct rdt_l3_mon_domain *d;
> -	u32 resid, evtid, domid;
> +	struct mon_evt *evt;
> +	u32 resid, domid;

Please fix reverse fir.

>  	struct rdtgroup *rdtgrp;
>  	struct rdt_resource *r;
>  	struct mon_data *md;
> @@ -590,7 +596,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  
>  	resid = md->rid;
>  	domid = md->domid;
> -	evtid = md->evtid;
> +	evt = md->evt;
>  	r = resctrl_arch_get_resource(resid);
>  
>  	if (md->sum) {
> @@ -604,7 +610,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  			if (d->ci->id == domid) {
>  				rr.ci = d->ci;
>  				mon_event_read(&rr, r, NULL, rdtgrp,
> -					       &d->ci->shared_cpu_map, evtid, false);
> +					       &d->ci->shared_cpu_map, evt, false);
>  				goto checkresult;
>  			}
>  		}
> @@ -621,7 +627,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  			goto out;
>  		}
>  		d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
> -		mon_event_read(&rr, r, d, rdtgrp, &d->hdr.cpu_mask, evtid, false);
> +		mon_event_read(&rr, r, d, rdtgrp, &d->hdr.cpu_mask, evt, false);
>  	}
>  
>  checkresult:
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 19cba29452b7..e903d3c076ee 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -365,19 +365,19 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
>  	u64 tval = 0;
>  
>  	if (rr->first) {
> -		resctrl_arch_reset_rmid(rr->r, rr->d, closid, rmid, rr->evtid);
> -		m = get_mbm_state(rr->d, closid, rmid, rr->evtid);
> +		resctrl_arch_reset_rmid(rr->r, rr->d, closid, rmid, rr->evt->evtid);
> +		m = get_mbm_state(rr->d, closid, rmid, rr->evt->evtid);
>  		if (m)
>  			memset(m, 0, sizeof(struct mbm_state));
>  		return 0;
>  	}
>  
>  	if (rr->d) {
> -		/* Reading a single domain, must be on a CPU in that domain. */
> -		if (!cpumask_test_cpu(cpu, &rr->d->hdr.cpu_mask))
> +		/* Reading a single domain, must usually be on a CPU in that domain. */

No need to use vague "usually" when it is very specific that it must be on a CPU in that
domain unless it is an event that can be read from any CPU.

> +		if (!rr->evt->any_cpu && !cpumask_test_cpu(cpu, &rr->d->hdr.cpu_mask))
>  			return -EINVAL;
>  		rr->err = resctrl_arch_rmid_read(rr->r, rr->d, closid, rmid,
> -						 rr->evtid, &tval, rr->arch_mon_ctx);
> +						 rr->evt->evtid, &tval, rr->arch_mon_ctx);
>  		if (rr->err)
>  			return rr->err;
>  
> @@ -387,7 +387,7 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
>  	}
>  
>  	/* Summing domains that share a cache, must be on a CPU for that cache. */

missed comment change

> -	if (!cpumask_test_cpu(cpu, &rr->ci->shared_cpu_map))
> +	if (!rr->evt->any_cpu && !cpumask_test_cpu(cpu, &rr->ci->shared_cpu_map))
>  		return -EINVAL;
>  
>  	/*

Reinette