Message-ID: <fb2396c1-3799-4d45-ae24-ce7e6f0d42e2@intel.com>
Date: Mon, 9 Jun 2025 15:39:43 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: "Luck, Tony" <tony.luck@...el.com>
CC: Fenghua Yu <fenghuay@...dia.com>, Maciej Wieczor-Retman
	<maciej.wieczor-retman@...el.com>, Peter Newman <peternewman@...gle.com>,
	James Morse <james.morse@....com>, Babu Moger <babu.moger@....com>, "Drew
 Fustini" <dfustini@...libre.com>, Dave Martin <Dave.Martin@....com>, "Anil
 Keshavamurthy" <anil.s.keshavamurthy@...el.com>, Chen Yu
	<yu.c.chen@...el.com>, <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
	<patches@...ts.linux.dev>
Subject: Re: [PATCH v5 27/29] fs/resctrl: Add file system mechanism for
 architecture info file

Tony,

On 6/9/25 11:49 AM, Luck, Tony wrote:
> On Fri, Jun 06, 2025 at 02:14:56PM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 6/6/25 10:30 AM, Luck, Tony wrote:
>>> On Fri, Jun 06, 2025 at 09:26:06AM -0700, Reinette Chatre wrote:
>>>> With /sys/kernel/debug/resctrl potentially mirroring /sys/fs/resctrl to
>>>> support various debugging scenarios there may later be resource level
>>>> debugging for which a "/sys/kernel/debug/resctrl/info/<resource>/<debugfile>" can
>>>> be used. Considering this it looks to me as though one possible boundary could
>>>> be to isolate arch specific debug to, for example, a new directory named
>>>> "/sys/kernel/debug/resctrl/info/arch_debug_name_tbd/". By placing the
>>>> arch debug in a sub-directory named "info" it avoids collision with resource
>>>> group names with naming that also avoids collision with resource names since
>>>> all these names are controlled by resctrl fs. 
>>>
>>>
>>> That seems like a good path. PoC patch below. Note that I put the dentry
>>> for the debug info directory into struct rdt_resource. So no call from
>>> architecture to file system code needed to access.
>>
>> ok, reading between the lines there is now a switch to per-resource
>> requirement, which fits with the use.
>>
>>>
>>> Directory layout looks like this:
>>>
>>> # tree /sys/kernel/debug/resctrl/
>>> /sys/kernel/debug/resctrl/
>>> └── info
>>>     ├── L2
>>>     ├── L3
>>>     ├── MB
>>>     └── SMBA
>>>
>>
>> This looks like something that needs to be owned and managed by
>> resctrl fs (more below).
>>
>>> 6 directories, 0 files
>>>
>>> -Tony
>>>
>>> ---
>>>
>>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>>> index 5e28e81b35f6..78dd0f8f7ad8 100644
>>> --- a/include/linux/resctrl.h
>>> +++ b/include/linux/resctrl.h
>>> @@ -281,6 +281,7 @@ enum resctrl_schema_fmt {
>>>   * @mbm_cfg_mask:	Bandwidth sources that can be tracked when bandwidth
>>>   *			monitoring events can be configured.
>>>   * @cdp_capable:	Is the CDP feature available on this resource
>>> + * @arch_debug_info:	Debugfs info directory for architecture use
>>>   */
>>>  struct rdt_resource {
>>>  	int			rid;
>>> @@ -297,6 +298,7 @@ struct rdt_resource {
>>>  	enum resctrl_schema_fmt	schema_fmt;
>>>  	unsigned int		mbm_cfg_mask;
>>>  	bool			cdp_capable;
>>> +	struct dentry		*arch_debug_info;
>>>  };
>>
>> ok ... but maybe not quite exactly (more below)
> 
> Would have been useful with the "always create directories" approach.
> As you point out below the name is problematic. Would need separate
> entries for control and monitor resources like RDT_RESOURCE_L3.
> 
> I don't think it is useful in the "only make directories when requested
> by architecture" mode.
> 
>>>  
>>>  /*
>>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>>> index ed4fc45da346..48c587201fb6 100644
>>> --- a/fs/resctrl/rdtgroup.c
>>> +++ b/fs/resctrl/rdtgroup.c
>>> @@ -4274,6 +4274,8 @@ void resctrl_offline_cpu(unsigned int cpu)
>>>   */
>>>  int resctrl_init(void)
>>>  {
>>> +	struct dentry *debuginfodir;
>>> +	struct rdt_resource *r;
>>>  	int ret = 0;
>>>  
>>>  	seq_buf_init(&last_cmd_status, last_cmd_status_buf,
>>> @@ -4320,6 +4322,12 @@ int resctrl_init(void)
>>>  	 */
>>>  	debugfs_resctrl = debugfs_create_dir("resctrl", NULL);
>>>  
>>> +	/* Create debug info directories for each resource */
>>> +	debuginfodir = debugfs_create_dir("info", debugfs_resctrl);
>>> +
>>> +	for_each_rdt_resource(r)
>>> +		r->arch_debug_info = debugfs_create_dir(r->name, debuginfodir);
>>
>> This ignores (*) several of the boundaries my response aimed to establish. 
>>
>> Here are some red flags:
>> - This creates the resource named directory and hands off that pointer to the
>>   arch. As I mentioned the arch should not have control over resctrl's debugfs.
>>   I believe this is the type of information that should be in control of resctrl fs
>>   since, as I mentioned, resctrl fs may need to add debugging that mirrors /sys/fs/resctrl. 
>> - Blindly creating these directories (a) without the resource even existing on the
>>   system, and (b) without being used/requested by the architecture does not create a good
>>   interface in my opinion. User space will see a bunch of empty directories
>>   associated with resources that are not present on the system.
>> - The directories created do not even match /sys/fs/resctrl/info when it comes
>>   to the resources. Note that the directories within /sys/fs/resctrl/info are created
>>   from the schema for control resources and appends _MON to monitor resources. Like
>>   I mentioned in my earlier response there should ideally be space for a future
>>   resctrl fs extension to mirror layout of /sys/fs/resctrl for resctrl fs debug
>>   in debugfs. This solution ignores all of that.
>>   
>> I still think that the architecture should request the debugfs directory from resctrl fs.
>> This avoids resctrl fs needing to create directories/files that are never used and
>> does not present user space with an empty tree. Considering that the new PERF_PKG
>> resource may not come online until resctrl mount this should be something that can be
>> called at any time.
>>
>> One possibility, that supports intended use while keeping the door open to support
>> future resctrl fs use of the debugfs, could be  a new resctrl fs function,
>> for example resctrl_create_mon_resource_debugfs(struct rdt_resource *r), that will initialize
>> rdt_resource::arch_debug_info(*) to point to the dentry of newly created
>> /sys/kernel/debug/resctrl/info/<rdt_resource::name>_MON/arch_debug_name_TBD *if*
>> the associated resource is capable of monitoring ... or do you think an architecture
>> may want to add debugging information before a resource is discovered/enabled?
>> If doing this then rdt_resource::arch_debug_info is no longer appropriate since it needs
>> to be specific to the monitoring resource. Perhaps then rdt_resource::arch_mon_debugfs
>> that would eventually live in [1]?
>>
>> This is feeling rushed and I am sharing some top of mind ideas. I will give this
>> more thought.
>>
>> Reinette
>>
>> [1] https://lore.kernel.org/lkml/cb8425c73f57280b0b4f22e089b2912eede42f7a.1747349530.git.babu.moger@amd.com/
>>
>> (*) I have now asked several times to stop ignoring feedback. This should not even
>>     be necessary in the first place. I do not require you to agree with me and I do not claim
>>     to always be right, please just stop ignoring feedback. The way forward I plan to ignore
>>     messages that ignores feedback.
> 
> So here's a second PoC. Takes into account all of the points you make
> above with the following adjustments:
> 
> 1) Not adding the rdt_resource::arch_mon_debugfs field.  Just returning
> the "struct dentry *" looks to be adequate for existing use case.
> 
> Having the pointer in "struct resource" would be useful if some future
> use case needed to access the debugfs locations from calls to
> architecture code that pass in the rdt_resource pointer. Could be
> added if ever needed.
> 
> 2) I can't envision a need for debugfs entries for resources
> pre-discovery, or when not enabled. So keep things simple for
> now.
> 
> 3) I think the function name resctrl_debugfs_mon_info_mkdir() is a bit
> more descriptive (it is making a directory and we usually have such
> functions include "mkdir" in the name).
> 
> -Tony
> 
> ---
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 8bec8f766b01..771e69c0c5c1 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -564,6 +564,12 @@ void resctrl_arch_reset_all_ctrls(struct rdt_resource *r);
>  extern unsigned int resctrl_rmid_realloc_threshold;
>  extern unsigned int resctrl_rmid_realloc_limit;
>  
> +/**
> + * resctrl_debugfs_mon_info_mkdir() - Create a debugfs info directory.
> + * @r:	Resource (must be mon_capable).
> + */
> +struct dentry *resctrl_debugfs_mon_info_mkdir(struct rdt_resource *r);
> +
>  int resctrl_init(void);
>  void resctrl_exit(void);
>  
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 8d094a3acf2f..0f11b8d0ce0b 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -4344,6 +4344,22 @@ int resctrl_init(void)
>  	return ret;
>  }
>  
> +struct dentry *resctrl_debugfs_mon_info_mkdir(struct rdt_resource *r)
> +{
> +	static struct dentry *debugfs_resctrl_info;
> +	char name[32];
> +
> +	if (!r->mon_capable)
> +		return NULL;
> +
> +	if (!debugfs_resctrl_info)
> +		debugfs_resctrl_info = debugfs_create_dir("info", debugfs_resctrl);
> +
> +	sprintf(name, "%s_MON", r->name);
> +
> +	return debugfs_create_dir(name, debugfs_resctrl_info);
> +}
> +
>  static bool resctrl_online_domains_exist(void)
>  {
>  	struct rdt_resource *r;


Why do you keep insisting without motivation on handing control of what
should be resctrl fs managed directories to architecture? Twice have I suggested
that an arch private directory be created for the arch debugfs and every
time you create a patch without motivation where arch gets control of what
should be resctrl fs managed. Again, if my suggestions are flawed it is an
opportunity for a teaching moment, never should be ignored. I highligted that
this is not ideal in the message you are responding to. I'm done. 

Reinette

Powered by blists - more mailing lists