Re: [PATCH v5 27/29] fs/resctrl: Add file system mechanism for architecture info file

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

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)

>  
>  /*
> 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.