Re: [PATCH v9 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 8/29/25 12:33 PM, Tony Luck wrote:
...

> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index f01db2034d08..b31794c5dcd4 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -217,20 +217,30 @@ static u64 mbm_overflow_count(u64 prev_msr, u64 cur_msr, unsigned int width)
>  	return chunks >> shift;
>  }
>  
> -int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mon_domain *d,
> +int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain_hdr *hdr,
>  			   u32 unused, u32 rmid, enum resctrl_event_id eventid,
>  			   u64 *val, void *ignored)
>  {
> -	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
> -	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
> -	int cpu = cpumask_any(&d->hdr.cpu_mask);
> +	int cpu = cpumask_any(&hdr->cpu_mask);
> +	struct rdt_hw_mon_domain *hw_dom;
> +	struct rdt_hw_resource *hw_res;
>  	struct arch_mbm_state *am;
> +	struct rdt_mon_domain *d;
>  	u64 msr_val, chunks;
>  	u32 prmid;
>  	int ret;
>  
>  	resctrl_arch_rmid_read_context_check();
>  
> +	if (r->rid != RDT_RESOURCE_L3)
> +		return -EINVAL;
> +

What is benefit of these additional errors added? From what I understand it is in
the "should never happen" category that the domain_header_is_valid() that follows
already handles?

> +	if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
> +		return -EINVAL;
> +
> +	d = container_of(hdr, struct rdt_mon_domain, hdr);
> +	hw_dom = resctrl_to_arch_mon_dom(d);
> +	hw_res = resctrl_to_arch_res(r);
>  	prmid = logical_rmid_to_physical_rmid(cpu, rmid);
>  	ret = __rmid_read_phys(prmid, eventid, &msr_val);
>  	if (ret)
> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index a7d60e74a29d..1c1c0e7bbc11 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -547,7 +547,7 @@ struct rdt_domain_hdr *resctrl_find_domain(struct list_head *h, int id,
>  }
>  
>  void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> -		    struct rdt_mon_domain *d, struct rdtgroup *rdtgrp,
> +		    struct rdt_domain_hdr *hdr, struct rdtgroup *rdtgrp,
>  		    cpumask_t *cpumask, int evtid, int first)
>  {
>  	int cpu;
> @@ -561,7 +561,7 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
>  	rr->rgrp = rdtgrp;
>  	rr->evtid = evtid;
>  	rr->r = r;
> -	rr->d = d;
> +	rr->hdr = hdr;
>  	rr->first = first;
>  	rr->arch_mon_ctx = resctrl_arch_mon_ctx_alloc(r, evtid);
>  	if (IS_ERR(rr->arch_mon_ctx)) {


> @@ -592,7 +592,6 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  	enum resctrl_event_id evtid;
>  	struct rdt_domain_hdr *hdr;
>  	struct rmid_read rr = {0};
> -	struct rdt_mon_domain *d;
>  	struct rdtgroup *rdtgrp;
>  	int domid, cpu, ret = 0;
>  	struct rdt_resource *r;
> @@ -617,6 +616,12 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  	r = resctrl_arch_get_resource(resid);
>  
>  	if (md->sum) {
> +		struct rdt_mon_domain *d;
> +
> +		if (WARN_ON_ONCE(resid != RDT_RESOURCE_L3)) {
> +			ret = -EIO;
> +			goto out;
> +		}

Apart from what is described in changelog this patch also adds a couple of new warnings
that seems to focus on the SNC code. Could the changelog please add a motivation for
how these fit in with the change as described?

>  		/*
>  		 * This file requires summing across all domains that share
>  		 * the L3 cache id that was provided in the "domid" field of the
> @@ -647,8 +652,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  			ret = -ENOENT;
>  			goto out;
>  		}
> -		d = container_of(hdr, struct rdt_mon_domain, hdr);

Above change introduces domain_header_is_valid() inconsistency by keeping the check
but removing the "checked" container_of(). Again I'll try to point these out as I see
them but I would like to repeat my request made in v8 that you review the related
changes to ensure the pattern is consistent.

> -		mon_event_read(&rr, r, d, rdtgrp, &d->hdr.cpu_mask, evtid, false);
> +		mon_event_read(&rr, r, hdr, rdtgrp, &hdr->cpu_mask, evtid, false);
>  	}
>  
>  checkresult:
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index dcc6c00eb362..713396cbda32 100644
> --- 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,22 @@ 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 (!domain_header_is_valid(rr->hdr, RESCTRL_MON_DOMAIN, 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 +390,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;
> +

Another SNC related warning.

>  	/* 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 +409,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 +438,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 (!domain_header_is_valid(rr->hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
> +		return;
> +	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;
>  
> @@ -608,7 +618,7 @@ static void mbm_update_one_event(struct rdt_resource *r, struct rdt_mon_domain *
>  	struct rmid_read rr = {0};
>  
>  	rr.r = r;
> -	rr.d = d;
> +	rr.hdr = &d->hdr;
>  	rr.evtid = evtid;
>  	rr.arch_mon_ctx = resctrl_arch_mon_ctx_alloc(rr.r, rr.evtid);
>  	if (IS_ERR(rr.arch_mon_ctx)) {
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 05438e15e2ca..2cff82208327 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -2887,7 +2887,8 @@ static void rmdir_all_sub(void)
>   * @rid:    The resource id for the event file being created.
>   * @domid:  The domain id for the event file being created.
>   * @mevt:   The type of event file being created.
> - * @do_sum: Whether SNC summing monitors are being created.
> + * @do_sum: Whether SNC summing monitors are being created. Only set
> + *          when @rid == RDT_RESOURCE_L3.
>   */
>  static struct mon_data *mon_get_kn_priv(enum resctrl_res_level rid, int domid,
>  					struct mon_evt *mevt,
> @@ -2897,6 +2898,9 @@ static struct mon_data *mon_get_kn_priv(enum resctrl_res_level rid, int domid,
>  
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
> +	if (WARN_ON_ONCE(do_sum && rid != RDT_RESOURCE_L3))
> +		return NULL;

... and another new SNC warning. Could you please help reader understand how these changes fit
with the domain -> header conversion?

> +
>  	list_for_each_entry(priv, &mon_data_kn_priv_list, list) {
>  		if (priv->rid == rid && priv->domid == domid &&
>  		    priv->sum == do_sum && priv->evtid == mevt->evtid)
> @@ -3024,17 +3028,27 @@ static void mon_rmdir_one_subdir(struct kernfs_node *pkn, char *name, char *subn
>   * when last domain being summed is removed.
>   */

Reinette