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

[Sudeep Holla]

On Thu, Apr 13, 2023 at 12:17:09PM +0200, Pierre Gondois wrote:
> 
> 
> 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]
>

Ah right, I missed to see that. I was sure there is a reason but couldn't
figure out myself quickly.

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

Yes that would be good.

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

Sure.

-- 
Regards,
Sudeep