Re: [PATCH v4 12/31] fs/resctrl: Improve handling for events that can be read from any CPU

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

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;
 
 	/*