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

["Chen, Yu C" <yu.c.chen@intel.com>]

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