SNC is only present in the RDT_RESOURCE_L3 domain.

Refactor code that makes and removes directories under "mon_data" to
special case the L3 resource.

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;
 	}
-- 
2.51.0

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.

Reinette

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
>

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

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

Hi Tony,

On 10/8/25 2:15 PM, Luck, Tony wrote:
> 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.

Thank you for clarifying. I was not able to keep this flow in my head.

> 
> 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 ...
> }

Indeed, this will keep the functions generic in the sense that it operates
on all resource types. This looks good since I think once the SNC code is taken
what remains should be easy to follow. 
I think it may also help to (in addition to the mon_scope check) add a RDT_RESOURCE_L3
check before the SNC code to keep the pattern that SNC only applies to the L3 resource.

Reinette

> > 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 ...
> > }
>
> Indeed, this will keep the functions generic in the sense that it operates
> on all resource types. This looks good since I think once the SNC code is taken
> what remains should be easy to follow.
> I think it may also help to (in addition to the mon_scope check) add a RDT_RESOURCE_L3
> check before the SNC code to keep the pattern that SNC only applies to the L3 resource.

Reinette,

The SNC versions to make and remove directories need to get the rdt_l3_mon_domain from
hdr. So they both begin with the standard:

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

        d = container_of(hdr, struct rdt_l3_mon_domain, hdr);

[though rmdir function returns void, so doesn't have that "-EINVAL".]

-Tony

Hi Tony,

On 10/8/25 3:29 PM, Luck, Tony wrote:
>>> 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 ...
>>> }
>>
>> Indeed, this will keep the functions generic in the sense that it operates
>> on all resource types. This looks good since I think once the SNC code is taken
>> what remains should be easy to follow.
>> I think it may also help to (in addition to the mon_scope check) add a RDT_RESOURCE_L3
>> check before the SNC code to keep the pattern that SNC only applies to the L3 resource.
> 
> Reinette,
> 
> The SNC versions to make and remove directories need to get the rdt_l3_mon_domain from
> hdr. So they both begin with the standard:
> 
>         if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
>                 return -EINVAL;
> 
>         d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
> 
> [though rmdir function returns void, so doesn't have that "-EINVAL".]

Understood. This is not about correctness but making the code easier to understand.
What I am aiming for is consistency in the code where the pattern
in existing flows use the resource ID as check to direct code flow to resource
specific code. In the above flow it uses the monitoring scope. This works of course,
but it is an implicit check because the L3 resource is the only one that currently
supports the "node" scope and does so when SNC is enabled. 
My preference is for the code to be consistent in patterns used and find doing so
makes the code easier to read and understand.

Reinette

On Wed, Oct 08, 2025 at 07:16:07PM -0700, Reinette Chatre wrote:
> Understood. This is not about correctness but making the code easier to understand.
> What I am aiming for is consistency in the code where the pattern
> in existing flows use the resource ID as check to direct code flow to resource
> specific code. In the above flow it uses the monitoring scope. This works of course,
> but it is an implicit check because the L3 resource is the only one that currently
> supports the "node" scope and does so when SNC is enabled. 
> My preference is for the code to be consistent in patterns used and find doing so
> makes the code easier to read and understand.
> 
Reinette,

Should I address this "only one that currently" issue now? Maybe by
adding a bool "rdt_resource:snc_mode" so the implicit

	if (r->mon_scope == RESCTRL_L3_NODE)

changes to:

	if (r->snc_mode)

There are only two places where SNC mode is checked in this way. The
others rely on seeing that mon_data::sum is set, or that rr->hdr is
NULL. So it seems like a very small improvement.

If we ever add a node scoped resource that isn't related to SNC, it
would be needed at that point. But I'm not sure why hardware would
ever do that.

-Tony

Hi Tony,

On 10/9/25 10:45 AM, Luck, Tony wrote:
> On Wed, Oct 08, 2025 at 07:16:07PM -0700, Reinette Chatre wrote:
>> Understood. This is not about correctness but making the code easier to understand.
>> What I am aiming for is consistency in the code where the pattern
>> in existing flows use the resource ID as check to direct code flow to resource
>> specific code. In the above flow it uses the monitoring scope. This works of course,
>> but it is an implicit check because the L3 resource is the only one that currently
>> supports the "node" scope and does so when SNC is enabled. 
>> My preference is for the code to be consistent in patterns used and find doing so
>> makes the code easier to read and understand.
>>
> Reinette,
> 
> Should I address this "only one that currently" issue now? Maybe by
> adding a bool "rdt_resource:snc_mode" so the implicit
> 
> 	if (r->mon_scope == RESCTRL_L3_NODE)
> 
> changes to:
> 
> 	if (r->snc_mode)
> 
> There are only two places where SNC mode is checked in this way. The
> others rely on seeing that mon_data::sum is set, or that rr->hdr is
> NULL. So it seems like a very small improvement.

This is not about SNC mode or not but instead about this code being L3
resource specific. 

I see the mon_data::sum and rr->hdr checks as supporting a separate
feature that was introduced to support SNC - it should not be used as
a check for SNC support even though it currently implies this due to SNC
being the only user. Could we not, hypothetically, even use these properties
in the region aware MBM work?

> If we ever add a node scoped resource that isn't related to SNC, it
> would be needed at that point. But I'm not sure why hardware would
> ever do that.

Right. This is not about just what is needed to enable this feature but
about making the code easy to follow for those that attempt to understand,
debug, and/or build on top.

Reinette

> > There are only two places where SNC mode is checked in this way. The
> > others rely on seeing that mon_data::sum is set, or that rr->hdr is
> > NULL. So it seems like a very small improvement.
>
> This is not about SNC mode or not but instead about this code being L3
> resource specific.
>
> I see the mon_data::sum and rr->hdr checks as supporting a separate
> feature that was introduced to support SNC - it should not be used as
> a check for SNC support even though it currently implies this due to SNC
> being the only user. Could we not, hypothetically, even use these properties
> in the region aware MBM work?
>
> > If we ever add a node scoped resource that isn't related to SNC, it
> > would be needed at that point. But I'm not sure why hardware would
> > ever do that.
>
> Right. This is not about just what is needed to enable this feature but
> about making the code easy to follow for those that attempt to understand,
> debug, and/or build on top.

Reinette,

Region aware MBM work will need to sum things to support legacy resctrl
"mbm_total_bytes". But while SNC sums across domains that share the
same cache id inside the same resource, we may be summing across
different resources (assuming we go with separate resources per region)
or summing across regions within a domain (if we bundle the regions into a new
struct rdt_region_mon_domain).

So __mon_event_count() will need to get additional refactoring and helper
functions, and struct mon_data an additional field to say that this other
"sum" function must be used.

-Tony

Hi Tony,

On 10/9/25 2:31 PM, Luck, Tony wrote:
>>> There are only two places where SNC mode is checked in this way. The
>>> others rely on seeing that mon_data::sum is set, or that rr->hdr is
>>> NULL. So it seems like a very small improvement.
>>
>> This is not about SNC mode or not but instead about this code being L3
>> resource specific.
>>
>> I see the mon_data::sum and rr->hdr checks as supporting a separate
>> feature that was introduced to support SNC - it should not be used as
>> a check for SNC support even though it currently implies this due to SNC
>> being the only user. Could we not, hypothetically, even use these properties
>> in the region aware MBM work?
>>
>>> If we ever add a node scoped resource that isn't related to SNC, it
>>> would be needed at that point. But I'm not sure why hardware would
>>> ever do that.
>>
>> Right. This is not about just what is needed to enable this feature but
>> about making the code easy to follow for those that attempt to understand,
>> debug, and/or build on top.
> 
> Reinette,
> 
> Region aware MBM work will need to sum things to support legacy resctrl
> "mbm_total_bytes". But while SNC sums across domains that share the
> same cache id inside the same resource, we may be summing across
> different resources (assuming we go with separate resources per region)
> or summing across regions within a domain (if we bundle the regions into a new
> struct rdt_region_mon_domain).
> 
> So __mon_event_count() will need to get additional refactoring and helper
> functions, and struct mon_data an additional field to say that this other
> "sum" function must be used.
> 

I did not mean to imply that this can be supported without refactoring. It does
seem as though you agree that mon_data::sum may be used for something
other than SNC and thus that using mon_data::sum as a check for SNC is not ideal.

Reinette

> I did not mean to imply that this can be supported without refactoring. It does
> seem as though you agree that mon_data::sum may be used for something
> other than SNC and thus that using mon_data::sum as a check for SNC is not ideal.

Reinette,

Yes, we are in agreement about non-SNC future usage.

Is it sufficient that I plant some WARN_ON_ONCE() in places where the
code assumes that mon_data::sum is only used by RDT_RESOURCE_L3
or for SNC?

Such code can be fixed by future patches that want to use mon_data::sum
for other things.

-Tony

Hi Tony,

On 10/9/25 3:08 PM, Luck, Tony wrote:
>> I did not mean to imply that this can be supported without refactoring. It does
>> seem as though you agree that mon_data::sum may be used for something
>> other than SNC and thus that using mon_data::sum as a check for SNC is not ideal.
> 
> Reinette,
> 
> Yes, we are in agreement about non-SNC future usage.
> 
> Is it sufficient that I plant some WARN_ON_ONCE() in places where the
> code assumes that mon_data::sum is only used by RDT_RESOURCE_L3
> or for SNC?

From what I understand this series does this already? I think this only applies to
rdtgroup_mondata_show() that does below ("L3 specific" comments added by me just for this example)
in this series:

	rdtgroup_mondata_show() 
	{
		...
		if (md->sum) {
			struct rdt_l3_mon_domain *d;

			if (WARN_ON_ONCE(resid != RDT_RESOURCE_L3)) {
				...
			}

			list_for_each_entry(d, &r->mon_domains, hdr.list) {
				if (d->ci_id == domid) { /* L3 specific field */
					...
					/* L3 specific */
					ci = get_cpu_cacheinfo_level(cpu, RESCTRL_L3_CACHE);
				}
			}
		...
	}

This seems reasonable since the flow is different from the typical "check resource"
followed by a domain_header_is_valid() that a refactor to support another resource
would probably do as you state below.

> 
> Such code can be fixed by future patches that want to use mon_data::sum
> for other things.

This discussion digressed a bit. The discussion started with a request to add a check
for the L3 resource before calling rmdir_mondata_subdir_allrdtgrp_snc(). 
I see this as something like:
	if (r->rid == RDT_RESOURCE_L3 && r->mon_scope == RESCTRL_L3_NODE) {
		rmdir_mondata_subdir_allrdtgrp_snc(r, hdr);
		...
	}

I understand that rmdir_mondata_subdir_allrdtgrp_snc() may look something like below
but I still find the flow easier to follow if a resource check is done before calling
rmdir_mondata_subdir_allrdtgrp_snc().

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

	        d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
		...

	}

Reinette

On Thu, Oct 09, 2025 at 05:16:00PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 10/9/25 3:08 PM, Luck, Tony wrote:
> >> I did not mean to imply that this can be supported without refactoring. It does
> >> seem as though you agree that mon_data::sum may be used for something
> >> other than SNC and thus that using mon_data::sum as a check for SNC is not ideal.
> > 
> > Reinette,
> > 
> > Yes, we are in agreement about non-SNC future usage.
> > 
> > Is it sufficient that I plant some WARN_ON_ONCE() in places where the
> > code assumes that mon_data::sum is only used by RDT_RESOURCE_L3
> > or for SNC?
> 
> From what I understand this series does this already? I think this only applies to
> rdtgroup_mondata_show() that does below ("L3 specific" comments added by me just for this example)
> in this series:
> 
> 	rdtgroup_mondata_show() 
> 	{
> 		...
> 		if (md->sum) {
> 			struct rdt_l3_mon_domain *d;
> 
> 			if (WARN_ON_ONCE(resid != RDT_RESOURCE_L3)) {

Exactly what I now have.
> 				...
My "..." is:
				return -EINVAL;
> 			}
> 
> 			list_for_each_entry(d, &r->mon_domains, hdr.list) {
> 				if (d->ci_id == domid) { /* L3 specific field */
> 					...
> 					/* L3 specific */
> 					ci = get_cpu_cacheinfo_level(cpu, RESCTRL_L3_CACHE);
> 				}
> 			}
> 		...
> 	}
> 
> This seems reasonable since the flow is different from the typical "check resource"
> followed by a domain_header_is_valid() that a refactor to support another resource
> would probably do as you state below.

I looked around to see if there were any other places that needed this,
but they all have checks for RDT_RESOURCE_L3 by the end of the series.
I've added a check in __mon_event_count() in patch 13 that gets deleted
in patch 18 when the L3 code is split out into a separate function.

> > 
> > Such code can be fixed by future patches that want to use mon_data::sum
> > for other things.
> 
> This discussion digressed a bit. The discussion started with a request to add a check
> for the L3 resource before calling rmdir_mondata_subdir_allrdtgrp_snc(). 
> I see this as something like:
> 	if (r->rid == RDT_RESOURCE_L3 && r->mon_scope == RESCTRL_L3_NODE) {

I'll add this. Same is needed in mkdir_mondata_subdir().

> 		rmdir_mondata_subdir_allrdtgrp_snc(r, hdr);
> 		...
> 	}
> 
> I understand that rmdir_mondata_subdir_allrdtgrp_snc() may look something like below
> but I still find the flow easier to follow if a resource check is done before calling
> rmdir_mondata_subdir_allrdtgrp_snc().
> 
> 	rmdir_mondata_subdir_allrdtgrp_snc(r, hdr)
> 	{
> 		if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
>                 return -EINVAL;
> 
> 	        d = container_of(hdr, struct rdt_l3_mon_domain, hdr);
> 		...
> 
> 	}
> 
> Reinette

-Tony

Hi Tony,

On 10/9/25 6:14 PM, Luck, Tony wrote:
> On Thu, Oct 09, 2025 at 05:16:00PM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 10/9/25 3:08 PM, Luck, Tony wrote:
>>>> I did not mean to imply that this can be supported without refactoring. It does
>>>> seem as though you agree that mon_data::sum may be used for something
>>>> other than SNC and thus that using mon_data::sum as a check for SNC is not ideal.
>>>
>>> Reinette,
>>>
>>> Yes, we are in agreement about non-SNC future usage.
>>>
>>> Is it sufficient that I plant some WARN_ON_ONCE() in places where the
>>> code assumes that mon_data::sum is only used by RDT_RESOURCE_L3
>>> or for SNC?
>>
>> From what I understand this series does this already? I think this only applies to
>> rdtgroup_mondata_show() that does below ("L3 specific" comments added by me just for this example)
>> in this series:
>>
>> 	rdtgroup_mondata_show() 
>> 	{
>> 		...
>> 		if (md->sum) {
>> 			struct rdt_l3_mon_domain *d;
>>
>> 			if (WARN_ON_ONCE(resid != RDT_RESOURCE_L3)) {
> 
> Exactly what I now have.
>> 				...
> My "..." is:
> 				return -EINVAL;
>> 			}
>>
>> 			list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> 				if (d->ci_id == domid) { /* L3 specific field */
>> 					...
>> 					/* L3 specific */
>> 					ci = get_cpu_cacheinfo_level(cpu, RESCTRL_L3_CACHE);
>> 				}
>> 			}
>> 		...
>> 	}
>>
>> This seems reasonable since the flow is different from the typical "check resource"
>> followed by a domain_header_is_valid() that a refactor to support another resource
>> would probably do as you state below.
> 
> I looked around to see if there were any other places that needed this,
> but they all have checks for RDT_RESOURCE_L3 by the end of the series.

Thank you for checking. This seems like a good pattern to use consistently.

> I've added a check in __mon_event_count() in patch 13 that gets deleted
> in patch 18 when the L3 code is split out into a separate function.
> 
>>>
>>> Such code can be fixed by future patches that want to use mon_data::sum
>>> for other things.
>>
>> This discussion digressed a bit. The discussion started with a request to add a check
>> for the L3 resource before calling rmdir_mondata_subdir_allrdtgrp_snc(). 
>> I see this as something like:
>> 	if (r->rid == RDT_RESOURCE_L3 && r->mon_scope == RESCTRL_L3_NODE) {
> 
> I'll add this. Same is needed in mkdir_mondata_subdir().
> 

Thank you very much.

Reinette