Re: [PATCH v6 13/30] x86,fs/resctrl: Handle events that can be read from any CPU

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 6/26/25 9:49 AM, Tony Luck wrote:

> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 6d4191eff391..aec26457d82c 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -356,11 +356,30 @@ static struct mbm_state *get_mbm_state(struct rdt_l3_mon_domain *d, u32 closid,
>  	return state ? &state[idx] : NULL;
>  }
>  


Could you please add a function comment for cpu_on_correct_domain(()
to document the different contexts that this function needs to be able
to handle? I think it is a bit subtle how the function is designed to be
run in preemptible as well as non-preemptible context. This will be helpful
when somebody aims to change/use this later.

> +static bool cpu_on_correct_domain(struct rmid_read *rr)
> +{
> +	struct cacheinfo *ci;
> +	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. */
> +	ci = get_cpu_cacheinfo_level(cpu, RESCTRL_L3_CACHE);
> +
> +	return ci && ci->id == rr->ci_id;
> +}
> +
>  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 cacheinfo *ci;
>  	struct mbm_state *m;
>  	int err, ret;
>  	u64 tval = 0;
> @@ -378,9 +397,10 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
>  	}
>  
>  	if (rr->hdr) {
> -		/* Reading a single domain, must be on a CPU in that domain. */
> -		if (!cpumask_test_cpu(cpu, &rr->hdr->cpu_mask))
> +		/* Single domain. */
> +		if (!cpu_on_correct_domain(rr))
>  			return -EINVAL;

cpu_on_correct_domain() duplicates the logic of __mon_event_count() so it
seems redundant to call cpu_on_correct_domain() in these paths. Since
cpu_on_correct_domain() contains complete logic it can be called just
once at the beginning of __mon_event_count() and thus also cover the
rr->first block?

> +
>  		rr->err = resctrl_arch_rmid_read(rr->r, rr->hdr, closid, rmid,
>  						 rr->evt->evtid, &tval, rr->arch_mon_ctx);
>  		if (rr->err)
> @@ -394,9 +414,8 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
>  	if (WARN_ON_ONCE(rr->r->rid != RDT_RESOURCE_L3))
>  		return -EINVAL;
>  
> -	/* Summing domains that share a cache, must be on a CPU for that cache. */
> -	ci = get_cpu_cacheinfo_level(cpu, RESCTRL_L3_CACHE);
> -	if (!ci || ci->id != rr->ci_id)
> +	/* Sum across multiple domains. */
> +	if (!cpu_on_correct_domain(rr))
>  		return -EINVAL;
>  
>  	/*
> @@ -878,7 +897,7 @@ struct mon_evt mon_event_all[QOS_NUM_EVENTS] = {
>  	},
>  };
>  
> -void resctrl_enable_mon_event(enum resctrl_event_id eventid)
> +void resctrl_enable_mon_event(enum resctrl_event_id eventid, bool any_cpu)
>  {
>  	if (WARN_ON_ONCE(eventid < QOS_FIRST_EVENT || eventid >= QOS_NUM_EVENTS))
>  		return;
> @@ -887,6 +906,7 @@ void resctrl_enable_mon_event(enum resctrl_event_id eventid)
>  		return;
>  	}
>  
> +	mon_event_all[eventid].any_cpu = any_cpu;
>  	mon_event_all[eventid].enabled = true;
>  }
>  

Reinette