mkdir_mondata_subdir() is unreasonably complicated because of the support for
Sub-NUMA Cluster (SNC) mode.

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)
 {
 	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);
 		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);
+	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)
+		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;
-- 
2.51.0

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

On Thu, Oct 23, 2025 at 10:45:06AM -0700, Reinette Chatre wrote:
> > +	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?

I looked at this. But the helper needs a lot of arguments to cover these
three cases. Something like:

static struct kernfs_node *make_and_fill_mondir(struct kernfs_node *parent_kn,
						char *name, umode_t mode,
						struct rdtgroup *prgrp,
						struct rdt_domain_hdr *hdr,
						struct rdt_resource *r,
						int domid)
{
	code here
}

A subset of this repeated pattern (just the kernfs_create_dir(),
rdtgroup_kn_set_ugid()) happens on every(?) mkdir. Perhaps a better helper (for
cleanup patch unrelated to this series) would build the rdtgroup_kn_set_ugid()
into a new rdtgroup_add_dir() helper?

-Tony

Hi Tony,

On 10/27/25 4:00 PM, Luck, Tony wrote:
> On Thu, Oct 23, 2025 at 10:45:06AM -0700, Reinette Chatre wrote:
>>> +	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?
> 
> I looked at this. But the helper needs a lot of arguments to cover these
> three cases. Something like:
> 
> static struct kernfs_node *make_and_fill_mondir(struct kernfs_node *parent_kn,
> 						char *name, umode_t mode,

I aimed to preempt a response like this in the text you quoted that notes that
a "mode" argument does not seem necessary. Are you hinting that mode is indeed
required? If not, without "mode" there are six arguments which is just one more
than mon_add_all_files() that will be called by it.
What is the threshold for there being too many arguments? This series does not seem
to have trouble pushing an arch API call resctrl_arch_rmid_read() to eight arguments
while there is a concern with the number of arguments of this static call that has
fewer?

> 						struct rdtgroup *prgrp,
> 						struct rdt_domain_hdr *hdr,
> 						struct rdt_resource *r,
> 						int domid)
> {
> 	code here
> }
> 
> A subset of this repeated pattern (just the kernfs_create_dir(),
> rdtgroup_kn_set_ugid()) happens on every(?) mkdir. Perhaps a better helper (for
> cleanup patch unrelated to this series) would build the rdtgroup_kn_set_ugid()
> into a new rdtgroup_add_dir() helper?

rdtgroup_add_dir() sounds like a useful helper. It is not clear to me how its motivation
is unique though. That is, why use motivation "rdtgroup_add_dir() is a helper that supports
two calls that are often repeated: kernfs_create_dir() followed by rdtgroup_kn_set_ugid()."
while, what will follow, two calls that are often repeated rdtgroup_add_dir() followed by
mon_add_all_files() does not need a helper?

Reinette

On Tue, Oct 28, 2025 at 09:00:46AM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 10/27/25 4:00 PM, Luck, Tony wrote:
> > On Thu, Oct 23, 2025 at 10:45:06AM -0700, Reinette Chatre wrote:
> >>> +	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?
> > 
> > I looked at this. But the helper needs a lot of arguments to cover these
> > three cases. Something like:
> > 
> > static struct kernfs_node *make_and_fill_mondir(struct kernfs_node *parent_kn,
> > 						char *name, umode_t mode,
> 
> I aimed to preempt a response like this in the text you quoted that notes that
> a "mode" argument does not seem necessary. Are you hinting that mode is indeed
> required? If not, without "mode" there are six arguments which is just one more
> than mon_add_all_files() that will be called by it.
> What is the threshold for there being too many arguments? This series does not seem
> to have trouble pushing an arch API call resctrl_arch_rmid_read() to eight arguments
> while there is a concern with the number of arguments of this static call that has
> fewer?

Ok. you have me convinced. Since this helper will be the only caller
of mon_add_all_files(), would it be good to just add the extra args
needed to this function and have it create the directory and add the
files? It would need a new name to describe the added functions it
performs. Maybe mon_add_domain_dir()? But better suggestions welcomed.

-Tony

Hi Tony,

On 10/28/25 10:14 AM, Luck, Tony wrote:
> On Tue, Oct 28, 2025 at 09:00:46AM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 10/27/25 4:00 PM, Luck, Tony wrote:
>>> On Thu, Oct 23, 2025 at 10:45:06AM -0700, Reinette Chatre wrote:
>>>>> +	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?
>>>
>>> I looked at this. But the helper needs a lot of arguments to cover these
>>> three cases. Something like:
>>>
>>> static struct kernfs_node *make_and_fill_mondir(struct kernfs_node *parent_kn,
>>> 						char *name, umode_t mode,
>>
>> I aimed to preempt a response like this in the text you quoted that notes that
>> a "mode" argument does not seem necessary. Are you hinting that mode is indeed
>> required? If not, without "mode" there are six arguments which is just one more
>> than mon_add_all_files() that will be called by it.
>> What is the threshold for there being too many arguments? This series does not seem
>> to have trouble pushing an arch API call resctrl_arch_rmid_read() to eight arguments
>> while there is a concern with the number of arguments of this static call that has
>> fewer?
> 
> Ok. you have me convinced. Since this helper will be the only caller
> of mon_add_all_files(), would it be good to just add the extra args
> needed to this function and have it create the directory and add the
> files? It would need a new name to describe the added functions it
> performs. Maybe mon_add_domain_dir()? But better suggestions welcomed.

Modifying mon_add_all_files() sounds good. I assume the node activation (kernfs_active())
will still be done by the caller which would have this new function return a struct kernfs_node *?
If so, I think it will make code easier to read if name implies that a new kn is created.
Since the caller is already mkdir_mondata_subdir(), what do you think of _mkdir_mondata_subdir()?

Reinette

On Tue, Oct 28, 2025 at 10:40:44AM -0700, Reinette Chatre wrote:
> Modifying mon_add_all_files() sounds good. I assume the node activation (kernfs_active())
> will still be done by the caller which would have this new function return a struct kernfs_node *?
> If so, I think it will make code easier to read if name implies that a new kn is created.
> Since the caller is already mkdir_mondata_subdir(), what do you think of _mkdir_mondata_subdir()?

Reinette,

This section of fs/resctrl/rdtgroup.c now looks like this. Call sites
are now free of duplicated code. Thanks for the nudge to do this.

-Tony

---

/*
 * Create a directory for a domain and populate it with monitor files.
 */
static struct kernfs_node *_mkdir_mondata_subdir(struct kernfs_node *parent_kn, char *name,
						 struct rdt_domain_hdr *hdr,
						 struct rdt_resource *r,
						 struct rdtgroup *prgrp, int domid)
{
	struct rmid_read rr = {0};
	struct kernfs_node *kn;
	struct mon_data *priv;
	struct mon_evt *mevt;
	int ret;

	kn = kernfs_create_dir(parent_kn, name, parent_kn->mode, prgrp);
	if (IS_ERR(kn))
		return kn;

	ret = rdtgroup_kn_set_ugid(kn);
	if (ret)
		goto out_destroy;

	for_each_mon_event(mevt) {
		if (mevt->rid != r->rid || !mevt->enabled)
			continue;
		priv = mon_get_kn_priv(r->rid, domid, mevt, !hdr);
		if (WARN_ON_ONCE(!priv)) {
			ret = -EINVAL;
			goto out_destroy;
		}

		ret = mon_addfile(kn, mevt->name, priv);
		if (ret)
			goto out_destroy;

		if (hdr && resctrl_is_mbm_event(mevt->evtid))
			mon_event_read(&rr, r, hdr, prgrp, &hdr->cpu_mask, mevt, true);
	}

	return kn;
out_destroy:
	kernfs_remove(kn);
	return ERR_PTR(ret);
}

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 *ckn, *kn;
	struct rdt_l3_mon_domain *d;
	char name[32];

	if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
		return -EINVAL;

	d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
	sprintf(name, "mon_%s_%02d", r->name, d->ci_id);
	kn = kernfs_find_and_get(parent_kn, name);
	if (kn) {
		/*
		 * rdtgroup_mutex will prevent this directory from being
		 * removed. No need to keep this hold.
		 */
		kernfs_put(kn);
	} else {
		kn = _mkdir_mondata_subdir(parent_kn, name, NULL, r, prgrp, d->ci_id);
		if (IS_ERR(kn))
			return PTR_ERR(kn);
	}

	sprintf(name, "mon_sub_%s_%02d", r->name, hdr->id);
	ckn = _mkdir_mondata_subdir(kn, name, hdr, r, prgrp, hdr->id);
	if (IS_ERR(ckn)) {
		kernfs_remove(kn);
		return PTR_ERR(ckn);
	}

	kernfs_activate(kn);
	return 0;
}

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

	lockdep_assert_held(&rdtgroup_mutex);

	if (r->rid == RDT_RESOURCE_L3 && r->mon_scope == RESCTRL_L3_NODE)
		return mkdir_mondata_subdir_snc(parent_kn, hdr, r, prgrp);

	sprintf(name, "mon_%s_%02d", r->name, hdr->id);
	kn = _mkdir_mondata_subdir(parent_kn, name, hdr, r, prgrp, hdr->id);
	if (IS_ERR(kn))
		return PTR_ERR(kn);

	kernfs_activate(kn);
	return 0;
}

Hi Tony,

On 10/28/25 11:40 AM, Luck, Tony wrote:
> On Tue, Oct 28, 2025 at 10:40:44AM -0700, Reinette Chatre wrote:
>> Modifying mon_add_all_files() sounds good. I assume the node activation (kernfs_active())
>> will still be done by the caller which would have this new function return a struct kernfs_node *?
>> If so, I think it will make code easier to read if name implies that a new kn is created.
>> Since the caller is already mkdir_mondata_subdir(), what do you think of _mkdir_mondata_subdir()?
> 
> Reinette,
> 
> This section of fs/resctrl/rdtgroup.c now looks like this. Call sites
> are now free of duplicated code. Thanks for the nudge to do this.
> 

> 
> /*
>  * Create a directory for a domain and populate it with monitor files.

Could you please add a short snippet about the different meanings of @hdr? It does not
have to be entirely kernel-doc style, just a note for the developer trying to understand
this function. Something like: "Create summing monitors when @hdr is NULL. No need to
initialize summing monitors."

>  */

...

I find this new version much easier to parse. Thank you very much.

Reinette