Re: [PATCH v11 20/31] fs/resctrl: Refactor Sub-NUMA Cluster (SNC) in mkdir/rmdir code flow

["Luck, Tony" <tony.luck@intel.com>]

On Fri, Oct 03, 2025 at 04:58:45PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 9/25/25 1:03 PM, Tony Luck wrote:
> > SNC is only present in the RDT_RESOURCE_L3 domain.
> > 
> > Refactor code that makes and removes directories under "mon_data" to
> 
> "makes and removes directories" -> "makes and removes SNC directories"?
> 
> > special case the L3 resource.
> 
> Why?
> 
> > 
> > Signed-off-by: Tony Luck <tony.luck@intel.com>
> > ---
> >  fs/resctrl/rdtgroup.c | 50 +++++++++++++++++++++++++++----------------
> >  1 file changed, 32 insertions(+), 18 deletions(-)
> > 
> > diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> > index 6e8937f94e7a..cab5cb9e6c93 100644
> > --- a/fs/resctrl/rdtgroup.c
> > +++ b/fs/resctrl/rdtgroup.c
> > @@ -3155,6 +3155,7 @@ static void mon_rmdir_one_subdir(struct kernfs_node *pkn, char *name, char *subn
> >  		return;
> >  	kernfs_put(kn);
> >  
> > +	/* Subdirectories are only present on SNC enabled systems */
> >  	if (kn->dir.subdirs <= 1)
> >  		kernfs_remove(kn);
> >  	else
> > @@ -3171,19 +3172,24 @@ static void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
> >  					   struct rdt_domain_hdr *hdr)
> >  {
> >  	struct rdtgroup *prgrp, *crgrp;
> > -	struct rdt_l3_mon_domain *d;
> > +	int domid = hdr->id;
> >  	char subname[32];
> > -	bool snc_mode;
> >  	char name[32];
> >  
> > -	if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
> > -		return;
> > +	if (r->rid == RDT_RESOURCE_L3) {
> > +		struct rdt_l3_mon_domain *d;
> >  
> > -	d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
> > -	snc_mode = r->mon_scope == RESCTRL_L3_NODE;
> > -	sprintf(name, "mon_%s_%02d", r->name, snc_mode ? d->ci_id : d->hdr.id);
> > -	if (snc_mode)
> > -		sprintf(subname, "mon_sub_%s_%02d", r->name, d->hdr.id);
> > +		if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
> > +			return;
> > +
> > +		d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
> > +		/* SNC mode? */
> > +		if (r->mon_scope == RESCTRL_L3_NODE) {
> > +			domid = d->ci_id;
> > +			sprintf(subname, "mon_sub_%s_%02d", r->name, hdr->id);
> > +		}
> > +	}
> > +	sprintf(name, "mon_%s_%02d", r->name, domid);
> >  
> >  	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
> >  		mon_rmdir_one_subdir(prgrp->mon.mon_data_kn, name, subname);
> > @@ -3213,7 +3219,7 @@ static int mon_add_all_files(struct kernfs_node *kn, struct rdt_domain_hdr *hdr,
> >  		if (ret)
> >  			return ret;
> >  
> > -		if (!do_sum && resctrl_is_mbm_event(mevt->evtid))
> > +		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, true);
> >  	}
> >  
> > @@ -3225,19 +3231,27 @@ static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
> >  				struct rdt_resource *r, struct rdtgroup *prgrp)
> >  {
> >  	struct kernfs_node *kn, *ckn;
> > -	struct rdt_l3_mon_domain *d;
> > +	bool snc_mode = false;
> > +	int domid = hdr->id;
> >  	char name[32];
> > -	bool snc_mode;
> >  	int ret = 0;
> >  
> >  	lockdep_assert_held(&rdtgroup_mutex);
> >  
> > -	if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
> > -		return -EINVAL;
> > +	if (r->rid == RDT_RESOURCE_L3) {
> > +		snc_mode = r->mon_scope == RESCTRL_L3_NODE;
> > +		if (snc_mode) {
> > +			struct rdt_l3_mon_domain *d;
> > +
> > +			if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
> > +				return -EINVAL;
> > +
> > +			d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
> > +			domid = d->ci_id;
> > +		}
> > +	}
> > +	sprintf(name, "mon_%s_%02d", r->name, domid);
> >  
> > -	d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
> > -	snc_mode = r->mon_scope == RESCTRL_L3_NODE;
> > -	sprintf(name, "mon_%s_%02d", r->name, snc_mode ? d->ci_id : d->hdr.id);
> >  	kn = kernfs_find_and_get(parent_kn, name);
> >  	if (kn) {
> >  		/*
> > @@ -3253,7 +3267,7 @@ static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
> >  		ret = rdtgroup_kn_set_ugid(kn);
> >  		if (ret)
> >  			goto out_destroy;
> > -		ret = mon_add_all_files(kn, hdr, r, prgrp, hdr->id, snc_mode);
> > +		ret = mon_add_all_files(kn, hdr, r, prgrp, domid, snc_mode);
> >  		if (ret)
> >  			goto out_destroy;
> >  	}
> 
> mkdir_mondata_subdir(), similar to __mon_event_count(), is now unreasonably
> complicated. Just like in that earlier change this inconsistently adds 
> RDT_RESOURCE_L3 checks, not to separate L3 code but instead to benefit PERF_PKG
> enabling to reach the handful of lines needed by it. 
> Here too I think the best way forward is to split mkdir_mondata_subdir().
> 
> rmdir_mondata_subdir_allrdtgrp() may also do with a split ... most of the
> code within it is dedicated to SNC and mon_rmdir_one_subdir() only exists
> because of SNC ... any other usage can just call kernfs_remove_by_name(), no?
> 
> SNC is already complicated enabling and I think that PERF_PKG trying to wedge
> itself into that is just too confusing. I expect separating this should simplify
> this a lot.

Ok. Splitting these makes sense. I'm terrible at naming. So I
tentatively have:

static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
				struct rdt_domain_hdr *hdr,
				struct rdt_resource *r, struct rdtgroup *prgrp)
{
	lockdep_assert_held(&rdtgroup_mutex);

	if (r->mon_scope == RESCTRL_L3_NODE)
		return mkdir_mondata_subdir_snc(parent_kn, hdr, r, prgrp);
	else
		return mkdir_mondata_subdir_normal(parent_kn, hdr, r, prgrp);
}

and:

static void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
					   struct rdt_domain_hdr *hdr)
{
	if (r->mon_scope == RESCTRL_L3_NODE)
		rmdir_mondata_subdir_allrdtgrp_snc(r, hdr);
	else
		rmdir_mondata_subdir_allrdtgrp_normal(r, hdr);
}

Better suggestions gratefully accepted.

> 
> Reinette
>