Re: [PATCH v7 09/31] x86,fs/resctrl: Use struct rdt_domain_hdr instead of struct rdt_mon_domain

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

Hi Tony,

On 7/11/25 4:53 PM, Tony Luck wrote:
> Historically all monitoring events have been associated with
> the L3 resource and it made sense to use the L3 specific "struct
> rdt_mon_domain *" arguments to functions manipulating domains.

Above is context describing current implementation so can be in present
tense. Needs imperative tone, eg. "All monitoring events are associated ..."


But
> the addition of monitor events tied to other resources changes this
> assumption.
> 
> To enable enumeration of domains for events in other resources, change
> the calling sequence to use the generic struct rdt_domain_hdr for domain
> addition and deletion to preserve as much common code as possible.
> 
> Same change to allow reading events in other resources. In this case
> the code flow passes from mon_event_read() via smp_call*() eventually
> to __mon_event_count() so the rmid_read::d field is replaced with
> the new rmid_read::hdr field.
> 
> The mon_data structure is unchanged, but documentation is updated
> to note that mon_data::sum is only used for RDT_RESOURCE_L3.

Needs imperative tone.

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---

...

> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -159,7 +159,7 @@ void __check_limbo(struct rdt_mon_domain *d, bool force_free)
>  			break;
>  
>  		entry = __rmid_entry(idx);
> -		if (resctrl_arch_rmid_read(r, d, entry->closid, entry->rmid,
> +		if (resctrl_arch_rmid_read(r, &d->hdr, entry->closid, entry->rmid,
>  					   QOS_L3_OCCUP_EVENT_ID, &val,
>  					   arch_mon_ctx)) {
>  			rmid_dirty = true;
> @@ -365,19 +365,23 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
>  	int err, ret;
>  	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);
> +	if (rr->r->rid == RDT_RESOURCE_L3 && rr->first) {
> +		if (WARN_ON_ONCE(!domain_header_is_valid(rr->hdr, RESCTRL_MON_DOMAIN,


This seems like a doube WARN_ON_ONCE() considering the one within domain_header_is_valid().

> +							 RDT_RESOURCE_L3)))
> +			return -EINVAL;
> +		d = container_of(rr->hdr, struct rdt_mon_domain, hdr);
> +		resctrl_arch_reset_rmid(rr->r, d, closid, rmid, rr->evtid);
> +		m = get_mbm_state(d, closid, rmid, rr->evtid);
>  		if (m)
>  			memset(m, 0, sizeof(struct mbm_state));
>  		return 0;
>  	}
>  
> -	if (rr->d) {
> +	if (rr->hdr) {
>  		/* Reading a single domain, must be on a CPU in that domain. */
> -		if (!cpumask_test_cpu(cpu, &rr->d->hdr.cpu_mask))
> +		if (!cpumask_test_cpu(cpu, &rr->hdr->cpu_mask))
>  			return -EINVAL;
> -		rr->err = resctrl_arch_rmid_read(rr->r, rr->d, closid, rmid,
> +		rr->err = resctrl_arch_rmid_read(rr->r, rr->hdr, closid, rmid,
>  						 rr->evtid, &tval, rr->arch_mon_ctx);
>  		if (rr->err)
>  			return rr->err;
> @@ -387,6 +391,9 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
>  		return 0;
>  	}
>  
> +	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)
> @@ -403,7 +410,7 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
>  	list_for_each_entry(d, &rr->r->mon_domains, hdr.list) {
>  		if (d->ci_id != rr->ci_id)
>  			continue;
> -		err = resctrl_arch_rmid_read(rr->r, d, closid, rmid,
> +		err = resctrl_arch_rmid_read(rr->r, &d->hdr, closid, rmid,
>  					     rr->evtid, &tval, rr->arch_mon_ctx);
>  		if (!err) {
>  			rr->val += tval;
> @@ -432,9 +439,13 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
>  static void mbm_bw_count(u32 closid, u32 rmid, struct rmid_read *rr)
>  {
>  	u64 cur_bw, bytes, cur_bytes;
> +	struct rdt_mon_domain *d;
>  	struct mbm_state *m;
>  
> -	m = get_mbm_state(rr->d, closid, rmid, rr->evtid);
> +	if (WARN_ON_ONCE(domain_header_is_valid(rr->hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3)))
> +		return;

Double WARN_ON_ONCE()?

> +	d = container_of(rr->hdr, struct rdt_mon_domain, hdr);
> +	m = get_mbm_state(d, closid, rmid, rr->evtid);
>  	if (WARN_ON_ONCE(!m))
>  		return;
>  

...

> @@ -3065,26 +3078,38 @@ static int mon_add_all_files(struct kernfs_node *kn, struct rdt_mon_domain *d,
>  		if (ret)
>  			return ret;
>  
> -		if (!do_sum && resctrl_is_mbm_event(mevt->evtid))
> -			mon_event_read(&rr, r, d, prgrp, &d->hdr.cpu_mask, mevt->evtid, true);
> +		if (r->rid == RDT_RESOURCE_L3 && !do_sum && resctrl_is_mbm_event(mevt->evtid))
> +			mon_event_read(&rr, r, hdr, prgrp, &hdr->cpu_mask, mevt->evtid, true);
>  	}
>  
>  	return 0;
>  }
>  
>  static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
> -				struct rdt_mon_domain *d,
> +				struct rdt_domain_hdr *hdr,
>  				struct rdt_resource *r, struct rdtgroup *prgrp)
>  {
>  	struct kernfs_node *kn, *ckn;
> +	bool snc_mode = false;
> +	int domid = hdr->id;
>  	char name[32];
> -	bool snc_mode;
>  	int ret = 0;
>  
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
> -	snc_mode = r->mon_scope == RESCTRL_L3_NODE;
> -	sprintf(name, "mon_%s_%02d", r->name, snc_mode ? d->ci_id : d->hdr.id);
> +	if (r->rid == RDT_RESOURCE_L3) {
> +		if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
> +			return -EINVAL;

The domain header check only seems necessary when needing to run the container_of()
below that depends on SNC mode so can be moved into the if() below?

> +		snc_mode = r->mon_scope == RESCTRL_L3_NODE;
> +		if (snc_mode) {
> +			struct rdt_mon_domain *d;
> +
> +			d = container_of(hdr, struct rdt_mon_domain, hdr);
> +			domid = d->ci_id;
> +		}
> +	}
> +	sprintf(name, "mon_%s_%02d", r->name, domid);
> +
>  	kn = kernfs_find_and_get(parent_kn, name);
>  	if (kn) {
>  		/*

Reinette