[PATCH v3 0/2] Add and use get_cpu_cacheinfo_level()

[PATCH v3 0/2] Add and use get_cpu_cacheinfo_level()

[Tony Luck]

This helper function came up in discussion of the resctrl patches
for Sub-NUMA Cluster (SNC) support. Reinette pointed out[1] that there
are already two places where it would clean things up by avoiding
open coding. The SNC patches will add two additional call sites.

So rather than have this jammed up as part of the SNC series, I'm
posting it as a simple standalone cleanup.

Signed-off-by: Tony Luck <tony.luck@intel.com>

[1] https://lore.kernel.org/all/050c64b3-20b3-4db6-b782-f5124ebaab31@intel.com/

---
Changes since v2:

Add lockdep_assert_cpus_held() to enforce what was simply a comment.

Combine patches 2 & 3. Both functions using get_cpu_cacheinfo_level()
are called inside rdtgroup_kn_lock_live()/rdtgroup_kn_unlock() code
blocks. So cpuhp lock is held.

Tony Luck (2):
  cacheinfo: Add function to get cacheinfo for a given (cpu, cachelevel)
  x86/resctrl: Replace open code cacheinfo searches

 include/linux/cacheinfo.h                 | 25 ++++++++++++++++++-----
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 17 ++++++---------
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 14 +++++--------
 3 files changed, 31 insertions(+), 25 deletions(-)


base-commit: c3f38fa61af77b49866b006939479069cd451173
-- 
2.45.0

[PATCH v3 1/2] cacheinfo: Add function to get cacheinfo for a given (cpu, cachelevel)

[Tony Luck]

Resctrl code open codes a search for information about a given cache
level in a couple of places (and more are on the way).

Provide a new inline function get_cpu_cacheinfo_level() in
<linux/cacheinfo.h> to do the search and return a pointer to
the cacheinfo structure.

Add lockdep_assert_cpus_held() to enforce the comment that cpuhp
lock must be held.

Simplify the existing get_cpu_cacheinfo_id() by using this new
function to do the search.

Signed-off-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
---
 include/linux/cacheinfo.h | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index 2cb15fe4fe12..530a16980aea 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -3,6 +3,7 @@
 #define _LINUX_CACHEINFO_H
 
 #include <linux/bitops.h>
+#include <linux/cpu.h>
 #include <linux/cpumask.h>
 #include <linux/smp.h>
 
@@ -113,23 +114,37 @@ 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 the 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;
 
+	lockdep_assert_cpus_held();
+
 	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
-- 
2.45.0

RE: [PATCH v3 1/2] cacheinfo: Add function to get cacheinfo for a given (cpu, cachelevel)

[Luck, Tony]

+	lockdep_assert_cpus_held();
+

lkp says this breaks riscv-allnoconfig and riscv-defconfig

   In file included from arch/riscv/include/asm/cacheinfo.h:9:
>> include/linux/cacheinfo.h:126:2: error: call to undeclared function 'lockdep_assert_cpus_held'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     126 |         lockdep_assert_cpus_held();

but I'm not really sure why. I added "#include <linux/cpu.h>" to <linux/cpuinfo,h> to deal with similar warning when building for x86.

-Tony

RE: [PATCH v3 1/2] cacheinfo: Add function to get cacheinfo for a given (cpu, cachelevel)

[Luck, Tony]

> +     lockdep_assert_cpus_held();
> +
>
> lkp says this breaks riscv-allnoconfig and riscv-defconfig
>
>    In file included from arch/riscv/include/asm/cacheinfo.h:9:
> >> include/linux/cacheinfo.h:126:2: error: call to undeclared function 'lockdep_assert_cpus_held'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>      126 |         lockdep_assert_cpus_held();
>
> but I'm not really sure why. I added "#include <linux/cpu.h>" to <linux/cpuinfo,h> to deal with similar warning when building for x86.

Ah. The full trace from lkp explains:

   In file included from io_uring/io_uring.c:45:
   In file included from include/linux/syscalls.h:93:
   In file included from include/trace/syscall.h:5:
   In file included from include/linux/tracepoint.h:22:
   In file included from include/linux/static_call.h:135:
   In file included from include/linux/cpu.h:17:                    <<<< only part way through cpu.h, lockdep_assert_cpus_held not defined yet
   In file included from include/linux/node.h:18:
   In file included from include/linux/device.h:32:
   In file included from include/linux/device/driver.h:21:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/riscv/include/asm/elf.h:16:
   In file included from arch/riscv/include/asm/cacheinfo.h:9:   <<< doesn't matter that this includes cpu.h because of #ifndef _LINUX_CPU_H_

>> include/linux/cacheinfo.h:126:2: error: call to undeclared function 'lockdep_assert_cpus_held'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     126 |         lockdep_assert_cpus_held();

So this is #include hell that many others have complained about.


How much do you *REALLY* want that lockdep_assert_cpus_held() to be in <linux/cacheinfo.h>

The lack of an assert did not come up when James added get_cpu_cacheinfo_id()

   https://lkml.kernel.org/r/20200708163929.2783-11-james.morse@arm.com

-Tony

Re: [PATCH v3 1/2] cacheinfo: Add function to get cacheinfo for a given (cpu, cachelevel)

[Borislav Petkov]

On Thu, Jun 06, 2024 at 07:58:29PM +0000, Luck, Tony wrote:
> So this is #include hell that many others have complained about.

Starts much earlier:

In file included from ./include/linux/cpu.h:17,
                 from ./include/linux/cacheinfo.h:6,
                 from ./arch/riscv/include/asm/cacheinfo.h:9,
                 from ./arch/riscv/include/asm/elf.h:16,
                 from ./include/linux/elf.h:6,
                 from ./include/linux/module.h:19,
                 from ./include/linux/device/driver.h:21,
                 from ./include/linux/device.h:32,
                 from ./include/linux/acpi.h:14,
                 from drivers/irqchip/irqchip.c:11:
./include/linux/node.h:96:25: error: field 'dev' has incomplete type
   96 |         struct device   dev;
      |

And yeah, this is an abomination.

> How much do you *REALLY* want that lockdep_assert_cpus_held() to be in <linux/cacheinfo.h>

If we had an easy fix sure but this really is a nightmare after trying
it a bit here too.

Oh well, some other time.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: [PATCH v3 1/2] cacheinfo: Add function to get cacheinfo for a given (cpu, cachelevel)

[Luck, Tony]

> And yeah, this is an abomination.
>
>> How much do you *REALLY* want that lockdep_assert_cpus_held() to be in <linux/cacheinfo.h>
>
> If we had an easy fix sure but this really is a nightmare after trying
> it a bit here too.

I tried something too. I moved the cpu hotplug lock bits out of <linux/cpu.h>
into their own file <linux/cpuhplock.h>. Made cpu.h include this new file to
keep things the same for anything that includes cpu.h.

In the change to cacheinfo.h I just include the new cpuhplock.h as all
it needs is the lockdep_assert_cpus_held() declaration.

I haven't heard back from lkp yet ... but it may be promising. Code
is the top three commits in:

git://git.kernel.org/pub/scm/linux/kernel/git/aegl/linux.git get_cpu_cacheinfo_level


Does this look like an OK direction?

Any better name for the new header?

While I'm moving those declarations, should I zap the "extern " from the
function ones to match current fashion for this (checkpatch barfs all over
them)?

-Tony

Re: [PATCH v3 1/2] cacheinfo: Add function to get cacheinfo for a given (cpu, cachelevel)

[Borislav Petkov]

On Sat, Jun 08, 2024 at 01:27:30PM +0000, Luck, Tony wrote:
> Does this look like an OK direction?

Yap, actually pretty nice and straight-forward at a quick glance.

> Any better name for the new header?

Considering how we call that machinery "cpuhp" everywhere, I think it
actually fits perfectly.

> While I'm moving those declarations, should I zap the "extern " from the
> function ones to match current fashion for this (checkpatch barfs all over
> them)?

Yeah, you can ignore checkpatch or simply do another patch ontop which
does just that. Code movement should be only mechanical movement, with
no other changes for ease of review.

In any case, thanks for doing that, that looks real good to me.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH v3 2/2] x86/resctrl: Replace open code cacheinfo searches

[Tony Luck]

pseudo_lock_region_init() and rdtgroup_cbm_to_size() open code
a search for details of a particular cache level.

Replace with get_cpu_cacheinfo_level()

Signed-off-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 17 ++++++-----------
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 14 +++++---------
 2 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index aacf236dfe3b..1bbfd3c1e300 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -292,9 +292,8 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
  */
 static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
 {
-	struct cpu_cacheinfo *ci;
+	struct cacheinfo *ci;
 	int ret;
-	int i;
 
 	/* Pick the first cpu we find that is associated with the cache. */
 	plr->cpu = cpumask_first(&plr->d->cpu_mask);
@@ -306,15 +305,11 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
 		goto out_region;
 	}
 
-	ci = get_cpu_cacheinfo(plr->cpu);
-
-	plr->size = rdtgroup_cbm_to_size(plr->s->res, plr->d, plr->cbm);
-
-	for (i = 0; i < ci->num_leaves; i++) {
-		if (ci->info_list[i].level == plr->s->res->cache_level) {
-			plr->line_size = ci->info_list[i].coherency_line_size;
-			return 0;
-		}
+	ci = get_cpu_cacheinfo_level(plr->cpu, plr->s->res->cache_level);
+	if (ci) {
+		plr->line_size = ci->coherency_line_size;
+		plr->size = rdtgroup_cbm_to_size(plr->s->res, plr->d, plr->cbm);
+		return 0;
 	}
 
 	ret = -1;
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 02f213f1c51c..cb68a121dabb 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1450,18 +1450,14 @@ static ssize_t rdtgroup_mode_write(struct kernfs_open_file *of,
 unsigned int rdtgroup_cbm_to_size(struct rdt_resource *r,
 				  struct rdt_domain *d, unsigned long cbm)
 {
-	struct cpu_cacheinfo *ci;
 	unsigned int size = 0;
-	int num_b, i;
+	struct cacheinfo *ci;
+	int num_b;
 
 	num_b = bitmap_weight(&cbm, r->cache.cbm_len);
-	ci = get_cpu_cacheinfo(cpumask_any(&d->cpu_mask));
-	for (i = 0; i < ci->num_leaves; i++) {
-		if (ci->info_list[i].level == r->cache_level) {
-			size = ci->info_list[i].size / r->cache.cbm_len * num_b;
-			break;
-		}
-	}
+	ci = get_cpu_cacheinfo_level(cpumask_any(&d->cpu_mask), r->cache_level);
+	if (ci)
+		size = ci->size / r->cache.cbm_len * num_b;
 
 	return size;
 }
-- 
2.45.0