lkml.org 
[lkml]   [2023]   [Apr]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3 4/4] cacheinfo: Add use_arch[|_cache]_info field/function
From


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@arm.com>
>> ---
>> 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
>>
>

\
 
 \ /
  Last update: 2023-04-13 19:06    [W:0.045 / U:1.540 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site