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

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

On Wed, Oct 08, 2025 at 10:12:36AM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 10/6/25 4:10 PM, Luck, Tony wrote:
> > On Fri, Oct 03, 2025 at 04:58:45PM -0700, Reinette Chatre wrote:
> >> On 9/25/25 1:03 PM, Tony Luck wrote:
> 
> >>> @@ -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.
> 
> It is not quite obvious to me how it all will turn out from here with the
> addition of support for PERF_PKG. By just considering the above I think
> that it helps to match the naming pattern with partners,
> for example rmdir_mondata_subdir_allrdtgrp() as you have that matches
> mkdir_mondata_subdir_allrdtgrp() that is not listed here. The problem is
> that the new rmdir_mondata_subdir_allrdtgrp() is L3 specific while 
> mkdir_mondata_subdir_allrdtgrp() remains generic. I thus think that it may make
> the code easier to follow if the L3 specific functions have _l3_ in the
> name as you established in patch #8. So perhaps above should be
> rmdir_l3_mondata_subdir_allrdtgrp() instead and then there may be a new
> rmdir_mondata_subdir_allrdtgrp() that will be the new generic function
> that calls the resource specific ones?
> 
> This could be extended to the new mkdir_mondata_subdir() above where
> it is named mkdir_l3_mondata_subdir() called by generic mkdir_mondata_subdir()?

Reinette

Making and removing the mon_data directories is the same for non-SNC L3
and PERF_PKG. The only "l3" connection is that SNC only occurs on L3.

So maybe my refactor should look like:


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);

	... pruned version of original code without SNC bits ...
}

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);
		return;
	}

	... pruned version of original code without SNC bits ...
}

-Tony