Re: [PATCH v12 20/31] fs/resctrl: Refactor mkdir_mondata_subdir()

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

Hi Tony,

On 10/13/25 3:33 PM, Tony Luck wrote:
> mkdir_mondata_subdir() is unreasonably complicated because of the support for
> Sub-NUMA Cluster (SNC) mode.

"mkdir_mondata_subdir()" -> "Population of a monitor group's mon_data directory"?
(to not describe code but what the code does)

> 
> Split out the SNC code into a helper function to make it easier to add support
> for a new telemetry resource.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  fs/resctrl/rdtgroup.c | 83 +++++++++++++++++++++++++++----------------
>  1 file changed, 52 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index c9d2cc1fd8bf..b7eaa4388768 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -3196,22 +3196,16 @@ static void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
>  
>  static int mon_add_all_files(struct kernfs_node *kn, struct rdt_domain_hdr *hdr,
>  			     struct rdt_resource *r, struct rdtgroup *prgrp,
> -			     bool do_sum)
> +			     int domid, bool do_sum)

I think there is some redundant information in the parameters that can be simplified.
It is already familiar that a domain/header of NULL implies that this is SNC/summing of domains.
For example, mon_event_read() and rmid_read::hdr. I think it will make things simpler and consistent
to do the same here where a hdr value of NULL implies sum of events. With that do_sum
will no longer be needed.

>  {
>  	struct rmid_read rr = {0};
> -	struct rdt_l3_mon_domain *d;
>  	struct mon_data *priv;
>  	struct mon_evt *mevt;
> -	int ret, domid;
> -
> -	if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
> -		return -EINVAL;
> +	int ret;
>  
> -	d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
>  	for_each_mon_event(mevt) {
>  		if (mevt->rid != r->rid || !mevt->enabled)
>  			continue;
> -		domid = do_sum ? d->ci_id : d->hdr.id;
>  		priv = mon_get_kn_priv(r->rid, domid, mevt, do_sum);
>  		if (WARN_ON_ONCE(!priv))
>  			return -EINVAL;
> @@ -3227,24 +3221,20 @@ static int mon_add_all_files(struct kernfs_node *kn, struct rdt_domain_hdr *hdr,
>  	return 0;
>  }
>  
> -static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
> -				struct rdt_domain_hdr *hdr,
> -				struct rdt_resource *r, struct rdtgroup *prgrp)
> +static int mkdir_mondata_subdir_snc(struct kernfs_node *parent_kn,
> +				    struct rdt_domain_hdr *hdr,
> +				    struct rdt_resource *r, struct rdtgroup *prgrp)
>  {
>  	struct kernfs_node *kn, *ckn;
>  	struct rdt_l3_mon_domain *d;
>  	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;
>  
>  	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);
> +	sprintf(name, "mon_%s_%02d", r->name, d->ci_id);
>  	kn = kernfs_find_and_get(parent_kn, name);
>  	if (kn) {
>  		/*
> @@ -3260,27 +3250,58 @@ 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, snc_mode);
> +		ret = mon_add_all_files(kn, hdr, r, prgrp, d->ci_id, true);

Continuing from the earlier mon_add_all_files() comment, passing a value for hdr here
does not seem right since there is no single domain attached to these files being created.
Now that this code is made to be more specific it can do so more clearly by passing NULL instead.

>  		if (ret)
>  			goto out_destroy;
>  	}
>  
> -	if (snc_mode) {
> -		sprintf(name, "mon_sub_%s_%02d", r->name, hdr->id);
> -		ckn = kernfs_create_dir(kn, name, parent_kn->mode, prgrp);
> -		if (IS_ERR(ckn)) {
> -			ret = -EINVAL;
> -			goto out_destroy;
> -		}
> +	sprintf(name, "mon_sub_%s_%02d", r->name, hdr->id);
> +	ckn = kernfs_create_dir(kn, name, parent_kn->mode, prgrp);

Noting here that kn was created earlier with mode of parent_kn->mode. It thus looks to me like
above can also be written as:
	ckn = kernfs_create_dir(kn, name, kn->mode, prgrp);

The reason I mention this is that this patch adds a third copy of a very similar code snippet
(kernfs_create_dir(), rdtgroup_kn_set_ugid(), mon_add_all_files()) that looks like a good
candidate for a helper?

> +	if (IS_ERR(ckn)) {
> +		ret = -EINVAL;
> +		goto out_destroy;
> +	}
>  
> -		ret = rdtgroup_kn_set_ugid(ckn);
> -		if (ret)
> -			goto out_destroy;
> +	ret = rdtgroup_kn_set_ugid(ckn);
> +	if (ret)
> +		goto out_destroy;
>  
> -		ret = mon_add_all_files(ckn, hdr, r, prgrp, false);
> -		if (ret)
> -			goto out_destroy;
> -	}
> +	ret = mon_add_all_files(ckn, hdr, r, prgrp, hdr->id, false);
> +	if (ret)
> +		goto out_destroy;
> +
> +	kernfs_activate(kn);
> +	return 0;
> +
> +out_destroy:
> +	kernfs_remove(kn);
> +	return ret;
> +}
> +
> +static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
> +				struct rdt_domain_hdr *hdr,
> +				struct rdt_resource *r, struct rdtgroup *prgrp)
> +{
> +	struct kernfs_node *kn;
> +	char name[32];
> +	int ret = 0;
> +
> +	lockdep_assert_held(&rdtgroup_mutex);
> +
> +	if (r->mon_scope == RESCTRL_L3_NODE)

What happened between the time you indicated that the above will receive a resource check 
and this patch?
https://lore.kernel.org/lkml/aOhd_A5L8PV0OYba@agluck-desk3/

> +		return mkdir_mondata_subdir_snc(parent_kn, hdr, r, prgrp);
> +
> +	sprintf(name, "mon_%s_%02d", r->name, hdr->id);
> +	kn = kernfs_create_dir(parent_kn, name, parent_kn->mode, prgrp);
> +	if (IS_ERR(kn))
> +		return PTR_ERR(kn);
> +
> +	ret = rdtgroup_kn_set_ugid(kn);
> +	if (ret)
> +		goto out_destroy;
> +	ret = mon_add_all_files(kn, hdr, r, prgrp, hdr->id, false);
> +	if (ret)
> +		goto out_destroy;
>  
>  	kernfs_activate(kn);
>  	return 0;

Reinette