[PATCH 0/3] Add and use get_cpu_cacheinfo_level()

[PATCH 0/3] Add and use get_cpu_cacheinfo_level()

[Tony Luck]

[get_maintainer.pl is vague about who owns <linux/cacheinfo.h>
 to I've scattershooted some of the folks that committed changes
 for this file]

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/

Tony Luck (3):
  cacheinfo: Add function to get cacheinfo for a given (cpu, cachelevel)
  x86/resctrl: Replace open code cacheinfo search in
    pseudo_lock_region_init()
  x86/resctrl: Replace open code cacheinfo search in
    rdtgroup_cbm_to_size()

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


base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
-- 
2.45.0

[PATCH 1/3] 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.

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>
---
 include/linux/cacheinfo.h | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

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

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

[Reinette Chatre]

Hi Tony,

On 5/31/24 12:57 PM, Tony Luck wrote:
> 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.
> 
> 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>
> ---
>   include/linux/cacheinfo.h | 21 ++++++++++++++++-----
>   1 file changed, 16 insertions(+), 5 deletions(-)
> 
> 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.

"for cache" -> "for the cache"?

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

Just the one nitpick from me.
Thank you.

| Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH 2/3] x86/resctrl: Replace open code cacheinfo search in pseudo_lock_region_init()

[Tony Luck]

Use get_cpu_cacheinfo_level() instead of open coded search.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 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;
-- 
2.45.0

Re: [PATCH 2/3] x86/resctrl: Replace open code cacheinfo search in pseudo_lock_region_init()

[Reinette Chatre]

On 5/31/24 12:57 PM, Tony Luck wrote:
> Use get_cpu_cacheinfo_level() instead of open coded search.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH 3/3] x86/resctrl: Replace open code cacheinfo search in rdtgroup_cbm_to_size()

[Tony Luck]

Use get_cpu_cacheinfo_level() instead of open coded search.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 02f213f1c51c..4a5ab0f6a412 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;
+	struct cacheinfo *ci;
 	unsigned int size = 0;
-	int num_b, i;
+	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

Re: [PATCH 3/3] x86/resctrl: Replace open code cacheinfo search in rdtgroup_cbm_to_size()

[Reinette Chatre]

On 5/31/24 12:57 PM, Tony Luck wrote:
> Use get_cpu_cacheinfo_level() instead of open coded search.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

RE: [PATCH 3/3] x86/resctrl: Replace open code cacheinfo search in rdtgroup_cbm_to_size()

[Luck, Tony]

>> Signed-off-by: Tony Luck <tony.luck@intel.com>
>> ---
>
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Patch broke firtree local declaration order:

> {
> -       struct cpu_cacheinfo *ci;
> +       struct cacheinfo *ci;
>         unsigned int size = 0;
> -       int num_b, i;
> +       int num_b;

-Tony

Re: [PATCH 0/3] Add and use get_cpu_cacheinfo_level()

[Reinette Chatre]

Dear Maintainers,

On 5/31/24 12:57 PM, Tony Luck wrote:
> [get_maintainer.pl is vague about who owns <linux/cacheinfo.h>
>   to I've scattershooted some of the folks that committed changes
>   for this file]

Thank you to Andrew for picking up this series into the
mm-nonmm-unstable branch. I am not familiar with how patches flow
from this repo/branch but I expect that this inclusion will require some
high level coordination between the big repos since resctrl changes
usually flow upstream through x86/cache branch of tip and I
anticipate some conflicts and also needing to figure out how to deal
with this new dependency in planned resctrl changes.

As Tony mentioned upcoming resctrl changes depend on this new
include/linux/cacheinfo.h addition while some other planned
resctrl changes [2] will conflict with patch #2 and #3 of this series.

A previous change to include/linux/cacheinfo.h (commit
709c4362725a ("cacheinfo: Move resctrl's get_cache_id() to the
cacheinfo header file")) was able to accompany the resctrl
changes via x86/cache of tip. Could it be possible to repeat
such a patch flow?

Looking at mm-nonmm-unstable there is indeed one other pending
change that touches include/linux/cacheinfo.h ("cpumask: make
core headers including cpumask_types.h where possible"). I
confirmed that there is no conflict between it and patch #1
from this series, the two can be applied in any order. Of course,
this means nothing for any other changes to this file
that may arrive later during this cycle.

Could you please provide your guidance on how to achieve a smooth
upstream of this work?

Thank you

Reinette

[2] https://lore.kernel.org/lkml/20240321165106.31602-1-james.morse@arm.com/

> 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/
> 
> Tony Luck (3):
>    cacheinfo: Add function to get cacheinfo for a given (cpu, cachelevel)
>    x86/resctrl: Replace open code cacheinfo search in
>      pseudo_lock_region_init()
>    x86/resctrl: Replace open code cacheinfo search in
>      rdtgroup_cbm_to_size()
> 
>   include/linux/cacheinfo.h                 | 21 ++++++++++++++++-----
>   arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 17 ++++++-----------
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 14 +++++---------
>   3 files changed, 27 insertions(+), 25 deletions(-)
> 
> 
> base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0

Re: [PATCH 0/3] Add and use get_cpu_cacheinfo_level()

[Borislav Petkov]

On Mon, Jun 03, 2024 at 09:55:24AM -0700, Reinette Chatre wrote:
> Thank you to Andrew for picking up this series into the
> mm-nonmm-unstable branch. I am not familiar with how patches flow
> from this repo/branch but I expect that this inclusion will require some
> high level coordination between the big repos since resctrl changes
> usually flow upstream through x86/cache branch of tip and I
> anticipate some conflicts and also needing to figure out how to deal
> with this new dependency in planned resctrl changes.

Which is totally useless coordination if Andrew simply leaves x86
patches to go through the tip tree and there are no conflicts.

> Could you please provide your guidance on how to achieve a smooth
> upstream of this work?

I'd prefer if resctrl patches go only through tip so that there's no
unnecessary headaches with merging and coordination.

Thx.

-- 
Regards/Gruss,
    Boris.

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

RE: [PATCH 0/3] Add and use get_cpu_cacheinfo_level()

[Luck, Tony]

> I'd prefer if resctrl patches go only through tip so that there's no
> unnecessary headaches with merging and coordination.

The two resctrl patches depend on the patch to <linux/cacheinfo.h>
So you'd need to take that patch through tip x86/cache as well.

-Tony

Re: [PATCH 0/3] Add and use get_cpu_cacheinfo_level()

[Borislav Petkov]

On Mon, Jun 03, 2024 at 05:18:37PM +0000, Luck, Tony wrote:
> The two resctrl patches depend on the patch to <linux/cacheinfo.h>
> So you'd need to take that patch through tip x86/cache as well.

Sure, as it is usually done.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 0/3] Add and use get_cpu_cacheinfo_level()

[Andrew Morton]

On Mon, 3 Jun 2024 19:01:06 +0200 Borislav Petkov <bp@alien8.de> wrote:

> > Could you please provide your guidance on how to achieve a smooth
> > upstream of this work?
> 
> I'd prefer if resctrl patches go only through tip so that there's no
> unnecessary headaches with merging and coordination.

No probs.  If/when patches turn up in a different tree (or get
convincingly nacked) I'll drop the mm.git copy.

I'll drop these now as things appear to be in hand.

Re: [PATCH 0/3] Add and use get_cpu_cacheinfo_level()

[Borislav Petkov]

On Tue, Jun 04, 2024 at 07:22:50PM -0700, Andrew Morton wrote:
> No probs.  If/when patches turn up in a different tree (or get
> convincingly nacked) I'll drop the mm.git copy.
> 
> I'll drop these now as things appear to be in hand.

Cool, thanks.

In general, you can safely ignore arch/x86/ patches. We're usually quick
to pick them up when ready. Getting them properly reviewed is a whole
another story... :-\

-- 
Regards/Gruss,
    Boris.

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