[PATCH v2 0/3] cacheinfo: Correctly fallback to using clidr_el1's information

[PATCH v2 0/3] cacheinfo: Correctly fallback to using clidr_el1's information

[Pierre Gondois]

v2:
cacheinfo: Check sib_leaf in cache_leaves_are_shared()
- Reformulate commit message
- Add 'Fixes: f16d1becf96f ("cacheinfo: Use cache identifiers [...]'
cacheinfo: Check cache properties are present in DT
- Use of_property_present()
- Add 'Reported-by: Alexandre Ghiti <alexghiti@rivosinc.com>'
cacheinfo: Add use_arch[|_cache]_info field/function:
- Make use_arch_cache_info() a static inline function

The cache information can be extracted from either a Device
Tree (DT), the PPTT ACPI table, or arch registers (clidr_el1
for arm64).

When the DT is used but no cache properties are advertised,
the current code doesn't correctly fallback to using arch information.

Correct this. Also use the assumption that L1 data/instruction caches
are private and L2/higher caches are shared when the cache information
is coming form clidr_el1.

Pierre Gondois (3):
  cacheinfo: Check sib_leaf in cache_leaves_are_shared()
  cacheinfo: Check cache properties are present in DT
  cacheinfo: Add use_arch[|_cache]_info field/function

 drivers/base/cacheinfo.c  | 48 +++++++++++++++++++++++++++++++++++----
 include/linux/cacheinfo.h | 11 +++++++++
 2 files changed, 55 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] cacheinfo: Check sib_leaf in cache_leaves_are_shared()

[Pierre Gondois]

If 'this_leaf' is a L2 cache (or higher) and 'sib_leaf' is a L1 cache,
the caches are detected as shared. Indeed, cache_leaves_are_shared()
only checks the cache level of 'this_leaf' when 'sib_leaf''s cache
level should also be checked.

Check 'sib_leaf->level'. Also update the comment as the function is
called when populating 'shared_cpu_map'.

Fixes: f16d1becf96f ("cacheinfo: Use cache identifiers to check if the caches are shared if available")
Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 drivers/base/cacheinfo.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index f3903d002819..e7ad6aba5f97 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -38,11 +38,10 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
 {
 	/*
 	 * For non DT/ACPI systems, assume unique level 1 caches,
-	 * system-wide shared caches for all other levels. This will be used
-	 * only if arch specific code has not populated shared_cpu_map
+	 * system-wide shared caches for all other levels.
 	 */
 	if (!(IS_ENABLED(CONFIG_OF) || IS_ENABLED(CONFIG_ACPI)))
-		return !(this_leaf->level == 1);
+		return (this_leaf->level != 1) || (sib_leaf->level != 1);
 
 	if ((sib_leaf->attributes & CACHE_ID) &&
 	    (this_leaf->attributes & CACHE_ID))
-- 
2.25.1

[PATCH v2 2/3] cacheinfo: Check cache properties are present in DT

[Pierre Gondois]

If a Device Tree (DT) is used, the presence of cache properties is
assumed. Not finding any is not considered. For arm64 platforms,
cache information can be fetched from the clidr_el1 register.
Checking whether cache information is available in the DT
allows to switch to using clidr_el1.

init_of_cache_level()
\-of_count_cache_leaves()
will assume there a 2 cache leaves (L1 data/instruction caches), which
can be different from clidr_el1 information.

cache_setup_of_node() tries to read cache properties in the DT.
If there are none, this is considered a success. Knowing no
information was available would allow to switch to using clidr_el1.

Fixes: de0df442ee49 ("cacheinfo: Check 'cache-unified' property to count cache leaves")
Reported-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Link: https://lore.kernel.org/all/20230404-hatred-swimmer-6fecdf33b57a@spud/
Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 drivers/base/cacheinfo.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index e7ad6aba5f97..6749dc6ebf50 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -78,6 +78,9 @@ bool last_level_cache_is_shared(unsigned int cpu_x, unsigned int cpu_y)
 }
 
 #ifdef CONFIG_OF
+
+static bool of_check_cache_nodes(struct device_node *np);
+
 /* OF properties to query for a given cache type */
 struct cache_type_info {
 	const char *size_prop;
@@ -205,6 +208,11 @@ static int cache_setup_of_node(unsigned int cpu)
 		return -ENOENT;
 	}
 
+	if (!of_check_cache_nodes(np)) {
+		of_node_put(np);
+		return -ENOENT;
+	}
+
 	prev = np;
 
 	while (index < cache_leaves(cpu)) {
@@ -229,6 +237,25 @@ static int cache_setup_of_node(unsigned int cpu)
 	return 0;
 }
 
+static bool of_check_cache_nodes(struct device_node *np)
+{
+	struct device_node *next;
+
+	if (of_property_present(np, "cache-size")   ||
+	    of_property_present(np, "i-cache-size") ||
+	    of_property_present(np, "d-cache-size") ||
+	    of_property_present(np, "cache-unified"))
+		return true;
+
+	next = of_find_next_cache_node(np);
+	if (next) {
+		of_node_put(next);
+		return true;
+	}
+
+	return false;
+}
+
 static int of_count_cache_leaves(struct device_node *np)
 {
 	unsigned int leaves = 0;
@@ -260,6 +287,9 @@ int init_of_cache_level(unsigned int cpu)
 	struct device_node *prev = NULL;
 	unsigned int levels = 0, leaves, level;
 
+	if (!of_check_cache_nodes(np))
+		goto err_out;
+
 	leaves = of_count_cache_leaves(np);
 	if (leaves > 0)
 		levels = 1;
-- 
2.25.1

[PATCH v2 3/3] cacheinfo: Add use_arch[|_cache]_info field/function

[Pierre Gondois]

The cache information can be extracted from either a Device
Tree (DT), the PPTT ACPI table, or arch registers (clidr_el1
for arm64).

The clidr_el1 register is used only if DT/ACPI information is not
available. It does not states how caches are shared among CPUs.

Add a use_arch_cache_info field/function to identify when the
DT/ACPI doesn't provide cache information. Use this information
to assume L1 caches are privates and L2 and higher are shared among
all CPUs.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 drivers/base/cacheinfo.c  | 15 +++++++++++++--
 include/linux/cacheinfo.h | 11 +++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index 6749dc6ebf50..49dbb4357911 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -40,8 +40,9 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
 	 * For non DT/ACPI systems, assume unique level 1 caches,
 	 * system-wide shared caches for all other levels.
 	 */
-	if (!(IS_ENABLED(CONFIG_OF) || IS_ENABLED(CONFIG_ACPI)))
-		return (this_leaf->level != 1) || (sib_leaf->level != 1);
+	if (!(IS_ENABLED(CONFIG_OF) || IS_ENABLED(CONFIG_ACPI)) ||
+	    this_leaf->use_arch_info)
+		return (this_leaf->level != 1) && (sib_leaf->level != 1);
 
 	if ((sib_leaf->attributes & CACHE_ID) &&
 	    (this_leaf->attributes & CACHE_ID))
@@ -349,6 +350,7 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
 	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
 	struct cacheinfo *this_leaf, *sib_leaf;
 	unsigned int index, sib_index;
+	bool use_arch_info = false;
 	int ret = 0;
 
 	if (this_cpu_ci->cpu_map_populated)
@@ -361,6 +363,12 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
 	 */
 	if (!last_level_cache_is_valid(cpu)) {
 		ret = cache_setup_properties(cpu);
+		if (ret && use_arch_cache_info()) {
+			// Possibility to rely on arch specific information.
+			use_arch_info = true;
+			ret = 0;
+		}
+
 		if (ret)
 			return ret;
 	}
@@ -370,6 +378,9 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
 
 		this_leaf = per_cpu_cacheinfo_idx(cpu, index);
 
+		if (use_arch_info)
+			this_leaf->use_arch_info = true;
+
 		cpumask_set_cpu(cpu, &this_leaf->shared_cpu_map);
 		for_each_online_cpu(i) {
 			struct cpu_cacheinfo *sib_cpu_ci = get_cpu_cacheinfo(i);
diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index 908e19d17f49..853f5d97f1dc 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -66,6 +66,7 @@ struct cacheinfo {
 #define CACHE_ALLOCATE_POLICY_MASK	\
 	(CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
 #define CACHE_ID		BIT(4)
+	bool use_arch_info;
 	void *fw_token;
 	bool disable_sysfs;
 	void *priv;
@@ -129,4 +130,14 @@ static inline int get_cpu_cacheinfo_id(int cpu, int level)
 	return -1;
 }
 
+static inline bool
+use_arch_cache_info(void)
+{
+#if defined(CONFIG_ARM64)
+	return true;
+#else
+	return false;
+#endif
+}
+
 #endif /* _LINUX_CACHEINFO_H */
-- 
2.25.1

Re: [PATCH v2 2/3] cacheinfo: Check cache properties are present in DT

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3234 bytes --]

Hey Pierre!

On Wed, Apr 12, 2023 at 09:18:05AM +0200, Pierre Gondois wrote:
> If a Device Tree (DT) is used, the presence of cache properties is
> assumed. Not finding any is not considered. For arm64 platforms,
> cache information can be fetched from the clidr_el1 register.
> Checking whether cache information is available in the DT
> allows to switch to using clidr_el1.
> 
> init_of_cache_level()
> \-of_count_cache_leaves()
> will assume there a 2 cache leaves (L1 data/instruction caches), which
> can be different from clidr_el1 information.
> 
> cache_setup_of_node() tries to read cache properties in the DT.
> If there are none, this is considered a success. Knowing no
> information was available would allow to switch to using clidr_el1.

Hmm, w/ this series I am still seeing a:
[    0.306736] Early cacheinfo failed, ret = -22

Not finding any cacheinfo is totally valid, right?

A basic RISC-V QEMU setup is sufficient to reproduce, for instance:
| $(qemu) \
| 	-m 2G -smp 5 \
| 	-M virt -nographic \
| 	-kernel $(vmlinux_bin)

Cheers,
Conor.

> Fixes: de0df442ee49 ("cacheinfo: Check 'cache-unified' property to count cache leaves")
> Reported-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> Link: https://lore.kernel.org/all/20230404-hatred-swimmer-6fecdf33b57a@spud/
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>  drivers/base/cacheinfo.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index e7ad6aba5f97..6749dc6ebf50 100644
> --- a/drivers/base/cacheinfo.c
> +++ b/drivers/base/cacheinfo.c
> @@ -78,6 +78,9 @@ bool last_level_cache_is_shared(unsigned int cpu_x, unsigned int cpu_y)
>  }
>  
>  #ifdef CONFIG_OF
> +
> +static bool of_check_cache_nodes(struct device_node *np);
> +
>  /* OF properties to query for a given cache type */
>  struct cache_type_info {
>  	const char *size_prop;
> @@ -205,6 +208,11 @@ static int cache_setup_of_node(unsigned int cpu)
>  		return -ENOENT;
>  	}
>  
> +	if (!of_check_cache_nodes(np)) {
> +		of_node_put(np);
> +		return -ENOENT;
> +	}
> +
>  	prev = np;
>  
>  	while (index < cache_leaves(cpu)) {
> @@ -229,6 +237,25 @@ static int cache_setup_of_node(unsigned int cpu)
>  	return 0;
>  }
>  
> +static bool of_check_cache_nodes(struct device_node *np)
> +{
> +	struct device_node *next;
> +
> +	if (of_property_present(np, "cache-size")   ||
> +	    of_property_present(np, "i-cache-size") ||
> +	    of_property_present(np, "d-cache-size") ||
> +	    of_property_present(np, "cache-unified"))
> +		return true;
> +
> +	next = of_find_next_cache_node(np);
> +	if (next) {
> +		of_node_put(next);
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static int of_count_cache_leaves(struct device_node *np)
>  {
>  	unsigned int leaves = 0;
> @@ -260,6 +287,9 @@ int init_of_cache_level(unsigned int cpu)
>  	struct device_node *prev = NULL;
>  	unsigned int levels = 0, leaves, level;
>  
> +	if (!of_check_cache_nodes(np))
> +		goto err_out;
> +
>  	leaves = of_count_cache_leaves(np);
>  	if (leaves > 0)
>  		levels = 1;
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 2/3] cacheinfo: Check cache properties are present in DT

[Pierre Gondois]

Hello Conor,

On 4/12/23 09:55, Conor Dooley wrote:
> Hey Pierre!
> 
> On Wed, Apr 12, 2023 at 09:18:05AM +0200, Pierre Gondois wrote:
>> If a Device Tree (DT) is used, the presence of cache properties is
>> assumed. Not finding any is not considered. For arm64 platforms,
>> cache information can be fetched from the clidr_el1 register.
>> Checking whether cache information is available in the DT
>> allows to switch to using clidr_el1.
>>
>> init_of_cache_level()
>> \-of_count_cache_leaves()
>> will assume there a 2 cache leaves (L1 data/instruction caches), which
>> can be different from clidr_el1 information.
>>
>> cache_setup_of_node() tries to read cache properties in the DT.
>> If there are none, this is considered a success. Knowing no
>> information was available would allow to switch to using clidr_el1.
> 
> Hmm, w/ this series I am still seeing a:
> [    0.306736] Early cacheinfo failed, ret = -22
> 
> Not finding any cacheinfo is totally valid, right?
> 
> A basic RISC-V QEMU setup is sufficient to reproduce, for instance:
> | $(qemu) \
> | 	-m 2G -smp 5 \
> | 	-M virt -nographic \
> | 	-kernel $(vmlinux_bin)

Sorry I forgot to remove the:
   pr_err("Early cacheinfo failed, ret = %d\n", ret);

I ll wait until tomorrow to send a v3 with this fixed.

Thanks for testing,
Regards,
Pierre

> 
> Cheers,
> Conor.
> 

Re: [PATCH v2 2/3] cacheinfo: Check cache properties are present in DT

[Alexandre Ghiti]

Hi Pierre, Conor,

On Wed, Apr 12, 2023 at 10:12 AM Pierre Gondois <pierre.gondois@arm.com> wrote:
>
> Hello Conor,
>
> On 4/12/23 09:55, Conor Dooley wrote:
> > Hey Pierre!
> >
> > On Wed, Apr 12, 2023 at 09:18:05AM +0200, Pierre Gondois wrote:
> >> If a Device Tree (DT) is used, the presence of cache properties is
> >> assumed. Not finding any is not considered. For arm64 platforms,
> >> cache information can be fetched from the clidr_el1 register.
> >> Checking whether cache information is available in the DT
> >> allows to switch to using clidr_el1.
> >>
> >> init_of_cache_level()
> >> \-of_count_cache_leaves()
> >> will assume there a 2 cache leaves (L1 data/instruction caches), which
> >> can be different from clidr_el1 information.
> >>
> >> cache_setup_of_node() tries to read cache properties in the DT.
> >> If there are none, this is considered a success. Knowing no
> >> information was available would allow to switch to using clidr_el1.
> >
> > Hmm, w/ this series I am still seeing a:
> > [    0.306736] Early cacheinfo failed, ret = -22
> >
> > Not finding any cacheinfo is totally valid, right?
> >
> > A basic RISC-V QEMU setup is sufficient to reproduce, for instance:
> > | $(qemu) \
> > |     -m 2G -smp 5 \
> > |     -M virt -nographic \
> > |     -kernel $(vmlinux_bin)
>
> Sorry I forgot to remove the:
>    pr_err("Early cacheinfo failed, ret = %d\n", ret);

I have just tested it and the messages "cacheinfo: Unable to detect
cache hierarchy for CPU" disappear, I'll add my tested by on the next
version. And just to make sure we agree: this fix should go into
-fixes (for 6.3).

Thanks again Conor and Pierre,

Alex

>
> I ll wait until tomorrow to send a v3 with this fixed.
>
> Thanks for testing,
> Regards,
> Pierre
>
> >
> > Cheers,
> > Conor.
> >

Re: [PATCH v2 1/3] cacheinfo: Check sib_leaf in cache_leaves_are_shared()

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2610 bytes --]

On Wed, Apr 12, 2023 at 09:18:04AM +0200, Pierre Gondois wrote:
> If 'this_leaf' is a L2 cache (or higher) and 'sib_leaf' is a L1 cache,
> the caches are detected as shared. Indeed, cache_leaves_are_shared()
> only checks the cache level of 'this_leaf' when 'sib_leaf''s cache
> level should also be checked.

I have to say, I'm a wee bit confused reading this patch - although it's
likely that I have just confused myself here.

The comment reads "For non DT/ACPI systems, assume unique level 1 caches,
system-wide shared caches for all other levels".
Does this mean all level 1 caches are unique & all level N caches are
shared with all other level N caches, but not with level M caches?
(M != N; M, N > 1)

Is this patches goal to make sure that if this_leaf is level 2 and
sib_leaf is level 1 that these are not detected as shared, since level
one caches are meant to be unique?

The previous logic checked only this_leaf's level, and declared things
shared if this_leaf is not a level 1 cache.
What happens here if this_leaf->level == 1 and sib_leaf->level == 2?
That'll be detected as shared, in a contradiction of the comment above
it, no?

As you never state the actual problem with the current code, I'm not
entirely sure if I am making a fool of myself or not here.

Probably making a fool, that's par for the course ;)

Thanks,
Conor.

> 
> Check 'sib_leaf->level'. Also update the comment as the function is
> called when populating 'shared_cpu_map'.
> 
> Fixes: f16d1becf96f ("cacheinfo: Use cache identifiers to check if the caches are shared if available")
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>  drivers/base/cacheinfo.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index f3903d002819..e7ad6aba5f97 100644
> --- a/drivers/base/cacheinfo.c
> +++ b/drivers/base/cacheinfo.c
> @@ -38,11 +38,10 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
>  {
>  	/*
>  	 * For non DT/ACPI systems, assume unique level 1 caches,
> -	 * system-wide shared caches for all other levels. This will be used
> -	 * only if arch specific code has not populated shared_cpu_map
> +	 * system-wide shared caches for all other levels.
>  	 */
>  	if (!(IS_ENABLED(CONFIG_OF) || IS_ENABLED(CONFIG_ACPI)))
> -		return !(this_leaf->level == 1);
> +		return (this_leaf->level != 1) || (sib_leaf->level != 1);
>  
>  	if ((sib_leaf->attributes & CACHE_ID) &&
>  	    (this_leaf->attributes & CACHE_ID))
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 3/3] cacheinfo: Add use_arch[|_cache]_info field/function

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 237 bytes --]

On Wed, Apr 12, 2023 at 09:18:06AM +0200, Pierre Gondois wrote:

> +static inline bool
> +use_arch_cache_info(void)

Tiny nit, why the newline when `static inline bool
use_arch_cache_info(void)` is only 50ish characters?

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/3] cacheinfo: Check sib_leaf in cache_leaves_are_shared()

[Pierre Gondois]

Hello Conor,

On 4/12/23 13:27, Conor Dooley wrote:
> On Wed, Apr 12, 2023 at 09:18:04AM +0200, Pierre Gondois wrote:
>> If 'this_leaf' is a L2 cache (or higher) and 'sib_leaf' is a L1 cache,
>> the caches are detected as shared. Indeed, cache_leaves_are_shared()
>> only checks the cache level of 'this_leaf' when 'sib_leaf''s cache
>> level should also be checked.
> 
> I have to say, I'm a wee bit confused reading this patch - although it's
> likely that I have just confused myself here.
> 
> The comment reads "For non DT/ACPI systems, assume unique level 1 caches,
> system-wide shared caches for all other levels".
> Does this mean all level 1 caches are unique & all level N caches are
> shared with all other level N caches, but not with level M caches?
> (M != N; M, N > 1)

I think the real answer to your question is in the last paragraph,
but just in case:

Each CPU manages the list of cacheinfo struct it has access to,
and this list is per-CPU.
cache_shared_cpu_map_setup() checks whether two cacheinfo struct are
representing the same cache (for 2 CPU lists). If yes, their
shared_cpu_map is updated.

If there is DT/ACPI information, a cacheid/fw_token is associated
with each cacheinfo struct. This allows to easily check when two
struct are representing the same cache.

Otherwise it is assumed here that L1 caches are private (so not shared)
and other L2-N caches are shared, i.e. the interface below advertise the
cache as available from other CPUs.
/sys/devices/system/cpu/cpu0/cache/indexX/shared_cpu_list

> 
> Is this patches goal to make sure that if this_leaf is level 2 and
> sib_leaf is level 1 that these are not detected as shared, since level
> one caches are meant to be unique?

Yes exact.

> 
> The previous logic checked only this_leaf's level, and declared things
> shared if this_leaf is not a level 1 cache.
> What happens here if this_leaf->level == 1 and sib_leaf->level == 2?
> That'll be detected as shared, in a contradiction of the comment above
> it, no?

Yes, there is a contradiction. The condition should be '&&':
   (this_leaf->level != 1) && (sib_leaf->level != 1)
I made a bad rebase and the corrected code ended up in PATCH 3/3.
Sorry for that. I ll correct it in the v3.

Thanks for the review,
Regards,
Pierre

> 
> As you never state the actual problem with the current code, I'm not
> entirely sure if I am making a fool of myself or not here.
> 
> Probably making a fool, that's par for the course ;)
> 
> Thanks,
> Conor.
> 
>>
>> Check 'sib_leaf->level'. Also update the comment as the function is
>> called when populating 'shared_cpu_map'.
>>
>> Fixes: f16d1becf96f ("cacheinfo: Use cache identifiers to check if the caches are shared if available")
>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
>> ---
>>   drivers/base/cacheinfo.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
>> index f3903d002819..e7ad6aba5f97 100644
>> --- a/drivers/base/cacheinfo.c
>> +++ b/drivers/base/cacheinfo.c
>> @@ -38,11 +38,10 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
>>   {
>>   	/*
>>   	 * For non DT/ACPI systems, assume unique level 1 caches,
>> -	 * system-wide shared caches for all other levels. This will be used
>> -	 * only if arch specific code has not populated shared_cpu_map
>> +	 * system-wide shared caches for all other levels.
>>   	 */
>>   	if (!(IS_ENABLED(CONFIG_OF) || IS_ENABLED(CONFIG_ACPI)))
>> -		return !(this_leaf->level == 1);
>> +		return (this_leaf->level != 1) || (sib_leaf->level != 1);
>>   
>>   	if ((sib_leaf->attributes & CACHE_ID) &&
>>   	    (this_leaf->attributes & CACHE_ID))
>> -- 
>> 2.25.1
>>

Re: [PATCH v2 3/3] cacheinfo: Add use_arch[|_cache]_info field/function

[Pierre Gondois]

On 4/12/23 13:47, Conor Dooley wrote:
> On Wed, Apr 12, 2023 at 09:18:06AM +0200, Pierre Gondois wrote:
> 
>> +static inline bool
>> +use_arch_cache_info(void)
> 
> Tiny nit, why the newline when `static inline bool
> use_arch_cache_info(void)` is only 50ish characters?

Ok sure, I ll do that.

Regards,
Pierre

> 
> Cheers,
> Conor.

Re: [PATCH v2 1/3] cacheinfo: Check sib_leaf in cache_leaves_are_shared()

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2919 bytes --]

On Wed, Apr 12, 2023 at 02:34:11PM +0200, Pierre Gondois wrote:
> Hello Conor,
> 
> On 4/12/23 13:27, Conor Dooley wrote:
> > On Wed, Apr 12, 2023 at 09:18:04AM +0200, Pierre Gondois wrote:
> > > If 'this_leaf' is a L2 cache (or higher) and 'sib_leaf' is a L1 cache,
> > > the caches are detected as shared. Indeed, cache_leaves_are_shared()
> > > only checks the cache level of 'this_leaf' when 'sib_leaf''s cache
> > > level should also be checked.
> > 
> > I have to say, I'm a wee bit confused reading this patch - although it's
> > likely that I have just confused myself here.
> > 
> > The comment reads "For non DT/ACPI systems, assume unique level 1 caches,
> > system-wide shared caches for all other levels".
> > Does this mean all level 1 caches are unique & all level N caches are
> > shared with all other level N caches, but not with level M caches?
> > (M != N; M, N > 1)
> 
> I think the real answer to your question is in the last paragraph,
> but just in case:
> 
> Each CPU manages the list of cacheinfo struct it has access to,
> and this list is per-CPU.
> cache_shared_cpu_map_setup() checks whether two cacheinfo struct are
> representing the same cache (for 2 CPU lists). If yes, their
> shared_cpu_map is updated.
> 
> If there is DT/ACPI information, a cacheid/fw_token is associated
> with each cacheinfo struct. This allows to easily check when two
> struct are representing the same cache.
> 
> Otherwise it is assumed here that L1 caches are private (so not shared)
> and other L2-N caches are shared, i.e. the interface below advertise the
> cache as available from other CPUs.
> /sys/devices/system/cpu/cpu0/cache/indexX/shared_cpu_list

Another silly question:
For two caches of level M & N; M != N; M, N > 1 should they be detected
as shared in the absence of any information in DT/ACPI?
The comment (to me) reads as if they should not, but it is rather vague.

> 
> > 
> > Is this patches goal to make sure that if this_leaf is level 2 and
> > sib_leaf is level 1 that these are not detected as shared, since level
> > one caches are meant to be unique?
> 
> Yes exact.
> 
> > 
> > The previous logic checked only this_leaf's level, and declared things
> > shared if this_leaf is not a level 1 cache.
> > What happens here if this_leaf->level == 1 and sib_leaf->level == 2?
> > That'll be detected as shared, in a contradiction of the comment above
> > it, no?
> 
> Yes, there is a contradiction. The condition should be '&&':
>   (this_leaf->level != 1) && (sib_leaf->level != 1)
> I made a bad rebase and the corrected code ended up in PATCH 3/3.
> Sorry for that. I ll correct it in the v3.

Good to know I am not losing my marbles, I was trying to reconcile the
intent with the patch & without the explicit statement of what was wrong
in the commit message I found it hard!

> Thanks for the review,

nw chief.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/3] cacheinfo: Check sib_leaf in cache_leaves_are_shared()

[Pierre Gondois]

On 4/12/23 14:47, Conor Dooley wrote:
> On Wed, Apr 12, 2023 at 02:34:11PM +0200, Pierre Gondois wrote:
>> Hello Conor,
>>
>> On 4/12/23 13:27, Conor Dooley wrote:
>>> On Wed, Apr 12, 2023 at 09:18:04AM +0200, Pierre Gondois wrote:
>>>> If 'this_leaf' is a L2 cache (or higher) and 'sib_leaf' is a L1 cache,
>>>> the caches are detected as shared. Indeed, cache_leaves_are_shared()
>>>> only checks the cache level of 'this_leaf' when 'sib_leaf''s cache
>>>> level should also be checked.
>>>
>>> I have to say, I'm a wee bit confused reading this patch - although it's
>>> likely that I have just confused myself here.
>>>
>>> The comment reads "For non DT/ACPI systems, assume unique level 1 caches,
>>> system-wide shared caches for all other levels".
>>> Does this mean all level 1 caches are unique & all level N caches are
>>> shared with all other level N caches, but not with level M caches?
>>> (M != N; M, N > 1)
>>
>> I think the real answer to your question is in the last paragraph,
>> but just in case:
>>
>> Each CPU manages the list of cacheinfo struct it has access to,
>> and this list is per-CPU.
>> cache_shared_cpu_map_setup() checks whether two cacheinfo struct are
>> representing the same cache (for 2 CPU lists). If yes, their
>> shared_cpu_map is updated.
>>
>> If there is DT/ACPI information, a cacheid/fw_token is associated
>> with each cacheinfo struct. This allows to easily check when two
>> struct are representing the same cache.
>>
>> Otherwise it is assumed here that L1 caches are private (so not shared)
>> and other L2-N caches are shared, i.e. the interface below advertise the
>> cache as available from other CPUs.
>> /sys/devices/system/cpu/cpu0/cache/indexX/shared_cpu_list
> 
> Another silly question:
> For two caches of level M & N; M != N; M, N > 1 should they be detected
> as shared in the absence of any information in DT/ACPI?
> The comment (to me) reads as if they should not, but it is rather vague.

I think they should. The naming of cache_leaves_are_shared() might be
misleading. The function is more trying to find out if 2 cache leaves struct
are representing the same cache. So maybe renaming the function to
cache_leaves_identical() might be better ?

In cache_shared_cpu_map_setup(), cache_leaves_are_shared() is used called
on each cache leaf a sibling CPU in order to try to find a matching cache leaf.
It loops until a match is detected.

If there is a DT/ACPI, cache leaves have an id/fw_token allowing to uniquely
identify them, and trying to find a matching leaf makes sense.
If there is no DT/ACPI, it is not possible to identify whether 2 cache leaves
are representing the same cache. The desired behaviour is just:
- If this_leaf or sib_leaf is a L1 cache, then the caches are not identical
   (or shared if we use this wording)
So the meaning of cache_leaves_identical() is a bit bent for this
configuration.

> 
>>
>>>
>>> Is this patches goal to make sure that if this_leaf is level 2 and
>>> sib_leaf is level 1 that these are not detected as shared, since level
>>> one caches are meant to be unique?
>>
>> Yes exact.
>>
>>>
>>> The previous logic checked only this_leaf's level, and declared things
>>> shared if this_leaf is not a level 1 cache.
>>> What happens here if this_leaf->level == 1 and sib_leaf->level == 2?
>>> That'll be detected as shared, in a contradiction of the comment above
>>> it, no?
>>
>> Yes, there is a contradiction. The condition should be '&&':
>>    (this_leaf->level != 1) && (sib_leaf->level != 1)
>> I made a bad rebase and the corrected code ended up in PATCH 3/3.
>> Sorry for that. I ll correct it in the v3.
> 
> Good to know I am not losing my marbles, I was trying to reconcile the
> intent with the patch & without the explicit statement of what was wrong
> in the commit message I found it hard!

Ok, I ll try to add more details in the commit message to be clearer.

Regards,
Pierre

> 
>> Thanks for the review,
> 
> nw chief.

Re: [PATCH v2 1/3] cacheinfo: Check sib_leaf in cache_leaves_are_shared()

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1223 bytes --]

On Wed, Apr 12, 2023 at 03:20:19PM +0200, Pierre Gondois wrote:
> > On Wed, Apr 12, 2023 at 02:34:11PM +0200, Pierre Gondois wrote:

> > Another silly question:
> > For two caches of level M & N; M != N; M, N > 1 should they be detected
> > as shared in the absence of any information in DT/ACPI?
> > The comment (to me) reads as if they should not, but it is rather vague.
> 
> I think they should. The naming of cache_leaves_are_shared() might be
> misleading. The function is more trying to find out if 2 cache leaves struct
> are representing the same cache. So maybe renaming the function to
> cache_leaves_identical() might be better?

Nah, I don't think this is really the fault of anything other than the
!DT && !ACPI situation.
I'm just trying to make sure I understand the intended behaviour in that
scenario, that's all.

> If there is no DT/ACPI, it is not possible to identify whether 2 cache leaves
> are representing the same cache. The desired behaviour is just:
> - If this_leaf or sib_leaf is a L1 cache, then the caches are not identical
>   (or shared if we use this wording)

> So the meaning of cache_leaves_identical() is a bit bent for this
> configuration.

Fair enough.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]