Skip to contentSkip to conversation

cacheinfo: move get_cpu_cacheinfo_id() back out

PATCH In review Jakub Kicinski 5 participants 7 messages Nov 20, 2021 – Nov 29, 2021

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 cacheinfo: move get_cpu_cacheinfo_id() back out

This commit more or less reverts commit 709c4362725a ("cacheinfo:
Move resctrl's get_cache_id() to the cacheinfo header file").

There are no users of the static inline helper outside of resctrl/core.c and cpu.h is a pretty heavy include, it pulls in device.h etc. This trips up architectures like riscv which want to access cacheinfo in low level headers like elf.h.

---
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]

x86 resctrl folks, does this look okay?

I'd like to do some bpf header cleanups in -next which this is blocking. How would you like to handle that? This change looks entirely harmless, can I get an ack and take this via bpf/netdev to Linus ASAP so it propagates to all trees?

---
 arch/x86/kernel/cpu/resctrl/core.c | 20 ++++++++++++++++++++
 include/linux/cacheinfo.h          | 21 ---------------------
 2 files changed, 20 insertions(+), 21 deletions(-)
arch/x86/kernel/cpu/resctrl/core.c +20 -0
index bb1c3f5f60c8..3c0b2c34be23 100644
@@ -284,6 +284,26 @@ static void rdt_get_cdp_l2_config(void)
 	rdt_get_cdp_config(RDT_RESOURCE_L2);
 }
 
+/*
+ * Get the id of the cache associated with @cpu at level @level.
+ * cpuhp lock must be held.
+ */
+static int get_cpu_cacheinfo_id(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].id;
+			return -1;
+		}
+	}
+
+	return -1;
+}
+
 static void
 mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r)
 {
include/linux/cacheinfo.h +0 -21
index 2f909ed084c6..c8c71eea237d 100644
@@ -3,7 +3,6 @@
 #define _LINUX_CACHEINFO_H
 
 #include <linux/bitops.h>
-#include <linux/cpu.h>
 #include <linux/cpumask.h>
 #include <linux/smp.h>
 
@@ -102,24 +101,4 @@ int acpi_find_last_cache_level(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.
- * cpuhp lock must be held.
- */
-static inline int get_cpu_cacheinfo_id(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].id;
-			return -1;
-		}
-	}
-
-	return -1;
-}
-
 #endif /* _LINUX_CACHEINFO_H */
Signature · 1 line
2.31.1
reviewer
#1 lore raw
Quoted 27 lines · Jakub Kicinski

This commit more or less reverts commit 709c4362725a ("cacheinfo:
Move resctrl's get_cache_id() to the cacheinfo header file").

There are no users of the static inline helper outside of resctrl/core.c and cpu.h is a pretty heavy include, it pulls in device.h etc. This trips up architectures like riscv which want to access cacheinfo in low level headers like elf.h.

---
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]

Quoted 6 lines

x86 resctrl folks, does this look okay?

I'd like to do some bpf header cleanups in -next which this is blocking. How would you like to handle that? This change looks entirely harmless, can I get an ack and take this via bpf/netdev to Linus ASAP so it propagates to all trees?

Does this patch target the bpf tree, or the bpf-next tree? If we want to unblock bpf header cleanup in -next, we can simply include it in a set for bpf-next.

Thanks,
Song

Quoted 76 lines
---
 arch/x86/kernel/cpu/resctrl/core.c | 20 ++++++++++++++++++++
 include/linux/cacheinfo.h          | 21 ---------------------
 2 files changed, 20 insertions(+), 21 deletions(-)
arch/x86/kernel/cpu/resctrl/core.c +20 -0
index bb1c3f5f60c8..3c0b2c34be23 100644
@@ -284,6 +284,26 @@ static void rdt_get_cdp_l2_config(void)
        rdt_get_cdp_config(RDT_RESOURCE_L2);
 }
 
+/*
+ * Get the id of the cache associated with @cpu at level @level.
+ * cpuhp lock must be held.
+ */
+static int get_cpu_cacheinfo_id(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].id;
+                       return -1;
+               }
+       }
+
+       return -1;
+}
+
 static void
 mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r)
 {
include/linux/cacheinfo.h +0 -22
index 2f909ed084c6..c8c71eea237d 100644
@@ -3,7 +3,6 @@
 #define _LINUX_CACHEINFO_H
 
 #include <linux/bitops.h>
-#include <linux/cpu.h>
 #include <linux/cpumask.h>
 #include <linux/smp.h>
 
@@ -102,24 +101,4 @@ int acpi_find_last_cache_level(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.
- * cpuhp lock must be held.
- */
-static inline int get_cpu_cacheinfo_id(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].id;
-                       return -1;
-               }
-       }
-
-       return -1;
-}
-
 #endif /* _LINUX_CACHEINFO_H */
--

2.31.1

#2 lore raw
Quoted 9 lines · On Tue, 23 Nov 2021 07:45:41 -1000 Song Liu wrote:
Quoted 6 lines

x86 resctrl folks, does this look okay?

I'd like to do some bpf header cleanups in -next which this is blocking. How would you like to handle that? This change looks entirely harmless, can I get an ack and take this via bpf/netdev to Linus ASAP so it propagates to all trees?

Does this patch target the bpf tree, or the bpf-next tree? If we want to unblock bpf header cleanup in -next, we can simply include it in a set for bpf-next.

No strong preference. Since this is very much not a bpf patch and should be safe my intuition is to ship it to Linus ASAP and avoid any potential conflicts with other -next trees. Also riscv people may appreciate it getting fixed sooner rather than later. If they trip up the same dependency later on in the cycle and we've already committed this patch to bpf-next they'd have to duplicate the patch in their tree.

reviewer
#3 lore raw
In reply to Song Liu

Hello,

Quoted 16 lines · Song Liu
Quoted 15 lines · Jakub Kicinski

This commit more or less reverts commit 709c4362725a ("cacheinfo:
Move resctrl's get_cache_id() to the cacheinfo header file").

There are no users of the static inline helper outside of resctrl/core.c and cpu.h is a pretty heavy include, it pulls in device.h etc. This trips up architectures like riscv which want to access cacheinfo in low level headers like elf.h.

---

x86 resctrl folks, does this look okay?

Quoted 7 lines
Quoted 4 lines

I'd like to do some bpf header cleanups in -next which this is blocking. How would you like to handle that? This change looks entirely harmless, can I get an ack and take this via bpf/netdev to Linus ASAP so it propagates to all trees?

Does this patch target the bpf tree, or the bpf-next tree? If we want to unblock bpf header cleanup in -next, we can simply include it in a set for bpf-next.

Some background: this is part of the mpam tree that wires up resctrl for arm64. This patch floated to the top and got merged with some cleanup as it was independent of the wider resctrl changes.

If the cpu.h include is the problem, I can't see what that is needed for. It almost certainly came in with a lockdep annotation that got replaced by a comment instead.

Thanks,

James

Quoted 40 lines
Quoted 40 lines
include/linux/cacheinfo.h +0 -22
index 2f909ed084c6..c8c71eea237d 100644
@@ -3,7 +3,6 @@
 #define _LINUX_CACHEINFO_H
 
 #include <linux/bitops.h>
-#include <linux/cpu.h>
 #include <linux/cpumask.h>
 #include <linux/smp.h>
 
@@ -102,24 +101,4 @@ int acpi_find_last_cache_level(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.
- * cpuhp lock must be held.
- */
-static inline int get_cpu_cacheinfo_id(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].id;
-                       return -1;
-               }
-       }
-
-       return -1;
-}
-
 #endif /* _LINUX_CACHEINFO_H */
--

2.31.1

reviewer Acked-by
#4 lore raw
Quoted 29 lines · James Morse

Hello,

Quoted 16 lines · Song Liu
Quoted 15 lines · Jakub Kicinski

This commit more or less reverts commit 709c4362725a ("cacheinfo:
Move resctrl's get_cache_id() to the cacheinfo header file").

There are no users of the static inline helper outside of resctrl/core.c and cpu.h is a pretty heavy include, it pulls in device.h etc. This trips up architectures like riscv which want to access cacheinfo in low level headers like elf.h.

---

x86 resctrl folks, does this look okay?

Quoted 7 lines
Quoted 4 lines

I'd like to do some bpf header cleanups in -next which this is blocking. How would you like to handle that? This change looks entirely harmless, can I get an ack and take this via bpf/netdev to Linus ASAP so it propagates to all trees?

Does this patch target the bpf tree, or the bpf-next tree? If we want to unblock bpf header cleanup in -next, we can simply include it in a set for bpf-next.

Quoted 6 lines

Some background: this is part of the mpam tree that wires up resctrl for arm64. This patch floated to the top and got merged with some cleanup as it was independent of the wider resctrl changes.

If the cpu.h include is the problem, I can't see what that is needed for. It almost certainly came in with a lockdep annotation that got replaced by a comment instead.

Thanks for the information.

I can ack the patch for the patch itself.

But I am not sure whether we should ship it via bpf tree. It seems to me that the
only reason we ship it via bpf tree is to get it to upstream ASAP?

Alexei/Daniel/Andrii, what do you think about this?

Thanks,
Song

#5 lore raw
Quoted 42 lines · Song Liu
Quoted 29 lines · James Morse

Hello,

Quoted 16 lines · Song Liu
Quoted 15 lines · Jakub Kicinski

This commit more or less reverts commit 709c4362725a ("cacheinfo:
Move resctrl's get_cache_id() to the cacheinfo header file").

There are no users of the static inline helper outside of resctrl/core.c and cpu.h is a pretty heavy include, it pulls in device.h etc. This trips up architectures like riscv which want to access cacheinfo in low level headers like elf.h.

---

x86 resctrl folks, does this look okay?

Quoted 7 lines
Quoted 4 lines

I'd like to do some bpf header cleanups in -next which this is blocking. How would you like to handle that? This change looks entirely harmless, can I get an ack and take this via bpf/netdev to Linus ASAP so it propagates to all trees?

Does this patch target the bpf tree, or the bpf-next tree? If we want to unblock bpf header cleanup in -next, we can simply include it in a set for bpf-next.

Quoted 6 lines

Some background: this is part of the mpam tree that wires up resctrl for arm64. This patch floated to the top and got merged with some cleanup as it was independent of the wider resctrl changes.

If the cpu.h include is the problem, I can't see what that is needed for. It almost certainly came in with a lockdep annotation that got replaced by a comment instead.

Thanks for the information.

I can ack the patch for the patch itself.

Quoted 7 lines

But I am not sure whether we should ship it via bpf tree. It seems to me that the
only reason we ship it via bpf tree is to get it to upstream ASAP?

Alexei/Daniel/Andrii, what do you think about this?

I don't completely understand why it cannot go via -next along with other patches, but if Jakub needs it asap here is my

and probably the fastest is for Jakub to take it via net tree directly.

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

maintainer
#6 lore raw

Hi Everybody,

Quoted 49 lines · Alexei Starovoitov
Quoted 48 lines · Song Liu
Quoted 29 lines · James Morse

Hello,

Quoted 16 lines · Song Liu
Quoted 15 lines · Jakub Kicinski

This commit more or less reverts commit 709c4362725a ("cacheinfo:
Move resctrl's get_cache_id() to the cacheinfo header file").

There are no users of the static inline helper outside of resctrl/core.c and cpu.h is a pretty heavy include, it pulls in device.h etc. This trips up architectures like riscv which want to access cacheinfo in low level headers like elf.h.

---

x86 resctrl folks, does this look okay?

Quoted 7 lines
Quoted 4 lines

I'd like to do some bpf header cleanups in -next which this is blocking. How would you like to handle that? This change looks entirely harmless, can I get an ack and take this via bpf/netdev to Linus ASAP so it propagates to all trees?

Does this patch target the bpf tree, or the bpf-next tree? If we want to unblock bpf header cleanup in -next, we can simply include it in a set for bpf-next.

Quoted 6 lines

Some background: this is part of the mpam tree that wires up resctrl for arm64. This patch floated to the top and got merged with some cleanup as it was independent of the wider resctrl changes.

If the cpu.h include is the problem, I can't see what that is needed for. It almost certainly came in with a lockdep annotation that got replaced by a comment instead.

Thanks for the information.

I can ack the patch for the patch itself.

But I am not sure whether we should ship it via bpf tree. It seems to me that the
only reason we ship it via bpf tree is to get it to upstream ASAP?

Quoted 6 lines
Quoted 1 line

Alexei/Daniel/Andrii, what do you think about this?

I don't completely understand why it cannot go via -next along with other patches, but if Jakub needs it asap here is my

and probably the fastest is for Jakub to take it via net tree directly.

These responses seems to ignore the information provided by James.

How about just the hunk below. It is not needed by the other parts removed and addresses the issue described in the changelog.

include/linux/cacheinfo.h +0 -1
@@ -3,7 +3,6 @@
  #define _LINUX_CACHEINFO_H
 
  #include <linux/bitops.h>
-#include <linux/cpu.h>
  #include <linux/cpumask.h>
  #include <linux/smp.h>
 

Reinette

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv