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

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

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