Received-SPF: pass (google.com: domain of linux-kernel+bounces-196075-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 30 May 2024 17:26:03 -0700
From: Tony Luck <tony.luck@intel.com>
To: Reinette Chatre <reinette.chatre@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>,
	Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>,
	Peter Newman <peternewman@google.com>,
	James Morse <james.morse@arm.com>, Babu Moger <babu.moger@amd.com>,
	Drew Fustini <dfustini@baylibre.com>,
	Dave Martin <Dave.Martin@arm.com>, x86@kernel.org,
	linux-kernel@vger.kernel.org, patches@lists.linux.dev
Subject: Re: [PATCH v19 08/20] x86/resctrl: Prepare for new Sub-NUMA Cluster
 (SNC) monitor files
Message-ID: <ZlkZG5bgsU6hY0Ok@agluck-desk3.sc.intel.com>
References: <20240528222006.58283-1-tony.luck@intel.com>
 <20240528222006.58283-9-tony.luck@intel.com>
 <fe9a4cd5-f74d-4c89-8667-8b5b1841b84e@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <fe9a4cd5-f74d-4c89-8667-8b5b1841b84e@intel.com>

On Thu, May 30, 2024 at 01:21:01PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 5/28/24 3:19 PM, Tony Luck wrote:
> > When SNC is enabled monitoring data is collected at the SNC node
> > granularity, but must be reported at L3-cache granularity for
> > backwards compatibility in addition to reporting at the node
> > level.
> > 
> > Add a "ci" field to the rdt_mon_domain structure to save the
> > cache information about the enclosing L3 cache for the domain.
> > This provides:
> > 
> > 1) The cache id which is needed to compose the name of the legacy
> > monitoring directory, and to determine which domains should be
> > summed to provide L3-scoped data.
> > 
> > 2) The shared_cpu_map which is needed to determine which CPUs can
> > be used to read the RMID counters with the MSR interface.
> > 
> > This is the first step to an eventual goal of monitor reporting files
> > like this (for a system with two SNC nodes per L3):
> > 
> > $ cd /sys/fs/resctrl/mon_data
> > $ tree mon_L3_00
> > mon_L3_00			<- 00 here is L3 cache id
> > ├── llc_occupancy		\  These files provide legacy support
> > ├── mbm_local_bytes		 > for non-SNC aware monitor apps
> > ├── mbm_total_bytes		/  that expect data at L3 cache level
> > ├── mon_sub_L3_00		<- 00 here is SNC node id
> > │   ├── llc_occupancy		\  These files are finer grained
> > │   ├── mbm_local_bytes		 > data from each SNC node
> > │   └── mbm_total_bytes		/
> > └── mon_sub_L3_01
> >      ├── llc_occupancy		\
> >      ├── mbm_local_bytes		 > As above, but for node 1.
> >      └── mbm_total_bytes		/
> > 
> > Signed-off-by: Tony Luck <tony.luck@intel.com>
> > ---
> >   include/linux/resctrl.h                   |  2 ++
> >   arch/x86/kernel/cpu/resctrl/internal.h    | 21 +++++++++++++++++++++
> >   arch/x86/kernel/cpu/resctrl/core.c        |  7 ++++++-
> >   arch/x86/kernel/cpu/resctrl/pseudo_lock.c |  1 -
> >   arch/x86/kernel/cpu/resctrl/rdtgroup.c    |  1 -
> >   5 files changed, 29 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> > index 64b6ad1b22a1..d733e1f6485d 100644
> > --- a/include/linux/resctrl.h
> > +++ b/include/linux/resctrl.h
> > @@ -96,6 +96,7 @@ struct rdt_ctrl_domain {
> >   /**
> >    * struct rdt_mon_domain - group of CPUs sharing a resctrl monitor resource
> >    * @hdr:		common header for different domain types
> > + * @ci:			cache info for this domain
> >    * @rmid_busy_llc:	bitmap of which limbo RMIDs are above threshold
> >    * @mbm_total:		saved state for MBM total bandwidth
> >    * @mbm_local:		saved state for MBM local bandwidth
> > @@ -106,6 +107,7 @@ struct rdt_ctrl_domain {
> >    */
> >   struct rdt_mon_domain {
> >   	struct rdt_domain_hdr		hdr;
> > +	struct cacheinfo		*ci;
> >   	unsigned long			*rmid_busy_llc;
> >   	struct mbm_state		*mbm_total;
> >   	struct mbm_state		*mbm_local;
> > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> > index 135190e0711c..eb70d3136ced 100644
> > --- a/arch/x86/kernel/cpu/resctrl/internal.h
> > +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> > @@ -2,6 +2,7 @@
> >   #ifndef _ASM_X86_RESCTRL_INTERNAL_H
> >   #define _ASM_X86_RESCTRL_INTERNAL_H
> > +#include <linux/cacheinfo.h>
> >   #include <linux/resctrl.h>
> >   #include <linux/sched.h>
> >   #include <linux/kernfs.h>
> > @@ -509,6 +510,26 @@ static inline bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l)
> >   int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
> > +/*
> > + * Get the cacheinfo structure of the cache associated with @cpu at level @level.
> > + * cpuhp lock must be held.
> > + */
> > +static inline struct cacheinfo *get_cpu_cacheinfo_level(int cpu, int level)
> > +{
> > +	struct cpu_cacheinfo *ci = get_cpu_cacheinfo(cpu);
> > +	int i;
> > +
> > +	for (i = 0; i < ci->num_leaves; i++) {
> > +		if (ci->info_list[i].level == level) {
> > +			if (ci->info_list[i].attributes & CACHE_ID)
> > +				return &ci->info_list[i];
> > +			break;
> > +		}
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> 
> This does not belong in resctrl. It really looks to partner well with existing
> cache helpers in include/linux/cacheinfo.h that already contains get_cpu_cacheinfo_id().
> Considering the existing naming get_cpu_cacheinfo() may be more appropriate.

Reinette,

The name get_cpu_cacheinfo() already exists and does something different
(returns a "struct cpu_cacheinfo *" rather than a "struct cacheinfo *").

How does this look for the change to <linux/cacheinfo.h> ... add a new
function get_cpu_cacheinfo_level() and then use it as a helper for the
existing get_cpu_cacheinfo_id()

-Tony

---

diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index 2cb15fe4fe12..301b0b24f446 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -113,10 +113,10 @@ int acpi_get_cache_info(unsigned int cpu,
 const struct attribute_group *cache_get_priv_group(struct cacheinfo *this_leaf);
 
 /*
- * Get the id of the cache associated with @cpu at level @level.
+ * Get the cacheinfo structure for cache associated with @cpu at level @level.
  * cpuhp lock must be held.
  */
-static inline int get_cpu_cacheinfo_id(int cpu, int level)
+static inline struct cacheinfo *get_cpu_cacheinfo_level(int cpu, int level)
 {
 	struct cpu_cacheinfo *ci = get_cpu_cacheinfo(cpu);
 	int i;
@@ -124,12 +124,23 @@ static inline int get_cpu_cacheinfo_id(int cpu, int level)
 	for (i = 0; i < ci->num_leaves; i++) {
 		if (ci->info_list[i].level == level) {
 			if (ci->info_list[i].attributes & CACHE_ID)
-				return ci->info_list[i].id;
-			return -1;
+				return &ci->info_list[i];
+			return NULL;
 		}
 	}
 
-	return -1;
+	return NULL;
+}
+
+/*
+ * Get the id of the cache associated with @cpu at level @level.
+ * cpuhp lock must be held.
+ */
+static inline int get_cpu_cacheinfo_id(int cpu, int level)
+{
+	struct cacheinfo *ci = get_cpu_cacheinfo_level(cpu, level);
+
+	return ci ? ci->id : -1;
 }
 
 #ifdef CONFIG_ARM64