Re: [PATCH v8 7/9] fs/resctrl: Do not invoke smp_processor_id() in preemptible context

[Reinette Chatre]

Hi Chenyu,

On 9/17/26 9:50 PM, Chen Yu wrote:
> From: Tony Luck <tony.luck@xxxxxxxxx>
> 
> An event flagged with mon_evt::any_cpu is read directly in preemptible task
> context instead of being dispatched to a CPU of the monitoring domain. Only
> Intel AET events set this flag, so __mon_event_count() rejects an any_cpu L3
> event with WARN_ON_ONCE(). The L3 read paths can therefore call
> smp_processor_id() unconditionally to check that the current CPU belongs to the
> domain being read.
> 
> MMIO-based LLC occupancy counters can be read from any CPU, so the L3 read
> paths have to tolerate an any_cpu event. Move that check into a helper which
> returns early for any_cpu events, avoiding smp_processor_id() in preemptible
> context, and report a wrong CPU via rmid_read::err.

"report a wrong CPU via rmid_read::err" is obvious from the patch.

> 
> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx>
> Tested-by: Hongyu Ning <hongyu.ning@xxxxxxxxxxxxxxx>
> ---
>  fs/resctrl/monitor.c | 48 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 73413cb128ea..3e05b0271c58 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -422,9 +422,37 @@ static void mbm_cntr_free(struct rdt_l3_mon_domain *d, int cntr_id)
>  	memset(&d->cntr_cfg[cntr_id], 0, sizeof(*d->cntr_cfg));
>  }
>  
> +/**
> + * cpu_on_correct_domain() - Check if current CPU is in the correct
> + *			      domain for the event.

The domain is not always relevant. When considering "Return: true if the
current CPU can read this event" ... could something like "cpu_can_read_event()"
be more descriptive? 

> + * @rr: The rmid_read structure containing event and domain information.
> + *
> + * Context: Preemptible process context when @rr->evt->any_cpu is set.
> + *          Non-migratable process context (via smp_call_on_cpu()) or
> + *          non-preemptible context (via smp_call_function_any()) when
> + *          the event must be read on a specific CPU.
> + * Return: true if the current CPU can read this event, false otherwise.
> + */
> +static bool cpu_on_correct_domain(struct rmid_read *rr)
> +{
> +	int cpu;
> +
> +	/* Any CPU is OK for this event */
> +	if (rr->evt->any_cpu)
> +		return true;
> +
> +	cpu = smp_processor_id();
> +
> +	/* Single domain. Must be on a CPU in that domain. */
> +	if (rr->hdr)
> +		return cpumask_test_cpu(cpu, &rr->hdr->cpu_mask);
> +
> +	/* Summing domains that share a cache, must be on a CPU for that cache. */
> +	return cpumask_test_cpu(cpu, &rr->ci->shared_cpu_map);
> +}
> +
>  static int __l3_mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
>  {
> -	int cpu = smp_processor_id();
>  	u32 closid = rdtgrp->closid;
>  	u32 rmid = rdtgrp->mon.rmid;
>  	struct rdt_l3_mon_domain *d;
> @@ -457,11 +485,6 @@ static int __l3_mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
>  		return 0;
>  	}
>  
> -	/* Reading a single domain, must be on a CPU in that domain. */
> -	if (!cpumask_test_cpu(cpu, &d->hdr.cpu_mask)) {
> -		rr->err = -EIO;
> -		return -EINVAL;
> -	}
>  	if (rr->is_mbm_cntr)
>  		rr->err = resctrl_arch_cntr_read(rr->r, d, closid, rmid, cntr_id,
>  						 rr->evt->evtid, &tval);
> @@ -479,7 +502,6 @@ static int __l3_mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
>  
>  static int __l3_mon_event_count_sum(struct rdtgroup *rdtgrp, struct rmid_read *rr)
>  {
> -	int cpu = smp_processor_id();
>  	u32 closid = rdtgrp->closid;
>  	u32 rmid = rdtgrp->mon.rmid;
>  	struct rdt_l3_mon_domain *d;
> @@ -497,12 +519,6 @@ static int __l3_mon_event_count_sum(struct rdtgroup *rdtgrp, struct rmid_read *r
>  		return -EINVAL;
>  	}
>  
> -	/* Summing domains that share a cache, must be on a CPU for that cache. */
> -	if (!cpumask_test_cpu(cpu, &rr->ci->shared_cpu_map)) {
> -		rr->err = -EIO;
> -		return -EINVAL;
> -	}
> -
>  	/*
>  	 * Legacy files must report the sum of an event across all
>  	 * domains that share the same L3 cache instance.
> @@ -538,7 +554,11 @@ static int __mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
>  {
>  	switch (rr->r->rid) {
>  	case RDT_RESOURCE_L3:
> -		WARN_ON_ONCE(rr->evt->any_cpu);
> +		if (!cpu_on_correct_domain(rr)) {
> +			rr->err = -EIO;
> +			return -EINVAL;
> +		}
> +
>  		if (rr->hdr)
>  			return __l3_mon_event_count(rdtgrp, rr);
>  		else

Reinette