Re: [PATCH v3 4/4] cacheinfo: Add use_arch[|_cache]_info field/function

[Pierre Gondois]

On 4/13/23 11:49, Sudeep Holla wrote:
> On Thu, Apr 13, 2023 at 11:14:34AM +0200, Pierre Gondois wrote:
> > 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.
>
> I have tentatively merged first 3 patches along with Radu's series(waiting
> for build tests still before confirming). I am not yet sure on this.
>
> > Signed-off-by: Pierre Gondois <pierre.gondois@xxxxxxx>
> > ---
> >   drivers/base/cacheinfo.c  | 13 ++++++++++++-
> >   include/linux/cacheinfo.h | 10 ++++++++++
> >   2 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> > index 06de9a468958..49dbb4357911 100644
> > --- a/drivers/base/cacheinfo.c
> > +++ b/drivers/base/cacheinfo.c
> > @@ -40,7 +40,8 @@ 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)))
> > +	if (!(IS_ENABLED(CONFIG_OF) || IS_ENABLED(CONFIG_ACPI)) ||
> > +	    this_leaf->use_arch_info)
>
> Can't we just use use_arch_cache_info() here ?

I think that if we use use_arch_cache_info() here, then arm64  platforms
will always return here and never check fw_token/this_leaf->id values.
Indeed, we also need to know that no cache information is available in
DT/ACPI, cf. [1]

> >   		return (this_leaf->level != 1) && (sib_leaf->level != 1);
> >   
> >   	if ((sib_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.

[1]

> > +			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..fed675b251a2 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;
>
> Do you see the need to stash this value as it is either globally true or
> false based on the arch ?

A static variable could be used instead and set to true if we fail to fetch the
cache information from DT/ACPI, cf. [1]. The only possible transition for this
variable would be from false->true. I'll check if this works like this.

> >   	void *fw_token;
> >   	bool disable_sysfs;
> >   	void *priv;
> > @@ -129,4 +130,13 @@ 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
> > +}
> > +
>
> Can we just have it as:
> #ifdef CONFIG_ARM64
> #define use_arch_cache_info()	(true)
> #else
> #define use_arch_cache_info()	(false)
> #endif

Yes sure, I'll post a v4 with this along Conor's requested change.

> >   #endif /* _LINUX_CACHEINFO_H */
> > --
> > 2.25.1