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

+	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

> +     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

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

> 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

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