Message-ID: <aEcsxjWroliWf3G0@agluck-desk3>
Date: Mon, 9 Jun 2025 11:49:42 -0700
From: "Luck, Tony" <tony.luck@...el.com>
To: Reinette Chatre <reinette.chatre@...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

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

Powered by blists - more mailing lists