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

[Reinette Chatre]

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@xxxxxxxxx>
> ---
>  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.

Reinette