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 <[email protected]>'
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
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.
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 <[email protected]>
>> ---
>> 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
>>
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.
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.
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.
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.