ACPI/PPTT: Handle architecturally unknown cache types
Subsystems (2)
PATCH 1/1 ACPI/PPTT: Handle architecturally unknown cache types
The type of a cache might not be specified by architectural mechanisms (ie system registers), but its type might be specified in the PPTT. In this case, following the PPTT specification, we should identify the cache as the type specified by PPTT.
This fixes the following lscpu issue where only the cache type sysfs file is missing which results in no output providing a poor user experience in the above system configuration-
lscpu: cannot open /sys/devices/system/cpu/cpu0/cache/index3/type: No such file or directory
--- drivers/acpi/pptt.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
drivers/acpi/pptt.c +15 -0
@@ -401,6 +401,21 @@ static void update_cache_properties(struct cacheinfo *this_leaf,break ; } } +if ((this_leaf->type ==CACHE_TYPE_NOCACHE ) && + (found_cache->flags &ACPI_PPTT_CACHE_TYPE_VALID )) { +switch (found_cache->attributes &ACPI_PPTT_MASK_CACHE_TYPE ) { +case ACPI_PPTT_CACHE_TYPE_DATA : + this_leaf->type =CACHE_TYPE_DATA ; +break ; +case ACPI_PPTT_CACHE_TYPE_INSTR : + this_leaf->type =CACHE_TYPE_INST ; +break ; +case ACPI_PPTT_CACHE_TYPE_UNIFIED : +case ACPI_PPTT_CACHE_TYPE_UNIFIED_ALT : + this_leaf->type =CACHE_TYPE_UNIFIED ; +break ; + } + }/* * If the above flags are valid, and the cache type is NOCACHE * update the cache type as well.
Signature · 3 lines
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Hi Jeffrey,
(+Sudeep)
Quoted 35 lines · Jeffrey Hugo
The type of a cache might not be specified by architectural mechanisms (ie system registers), but its type might be specified in the PPTT. In this case, following the PPTT specification, we should identify the cache as the type specified by PPTT.
This fixes the following lscpu issue where only the cache type sysfs file is missing which results in no output providing a poor user experience in the above system configuration-
lscpu: cannot open/sys/devices/system/cpu/cpu0/cache/index3/type: No such file or directoryFixes 2bd00bcd73e5 (ACPI/PPTT: Add Processor Properties Topology Table parsing)Reported-by Vijaya Kumar K <[email protected]>Signed-off-by Jeffrey Hugo <[email protected]>--- drivers/acpi/pptt.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)drivers/acpi/pptt.c +8
-0@@ -401,6 +401,21 @@ static void update_cache_properties(struct cacheinfo *this_leaf,break ; } } +if ((this_leaf->type ==CACHE_TYPE_NOCACHE ) && + (found_cache->flags &ACPI_PPTT_CACHE_TYPE_VALID )) { +switch (found_cache->attributes &ACPI_PPTT_MASK_CACHE_TYPE ) { +case ACPI_PPTT_CACHE_TYPE_DATA : + this_leaf->type =CACHE_TYPE_DATA ; +break ; +case ACPI_PPTT_CACHE_TYPE_INSTR : + this_leaf->type =CACHE_TYPE_INST ;
drivers/acpi/pptt.c static void update_cache_properties(struct cacheinfo *this_leaf, · line 421
Quoted 10 lines
drivers/acpi/pptt.c +7
-0+break ; +case ACPI_PPTT_CACHE_TYPE_UNIFIED : +case ACPI_PPTT_CACHE_TYPE_UNIFIED_ALT : + this_leaf->type =CACHE_TYPE_UNIFIED ; +break ; + } + }/* * If the above flags are valid, and the cache type is NOCACHE * update the cache type as well.
If you look at the next line of code following this comment its going to update the cache type for fully populated PPTT nodes. Although with the suggested change its only going to activate if someone completely fills out the node and fails to set the valid flag on the cache type.
What I suspect is happening in the reported case is that the nodes in the PPTT table are missing fields we consider to be important. Since that data isn't being filled out anywhere else, so we leave the cache type alone too. This has the effect of hiding sysfs nodes with incomplete information.
Also, the lack of the DATA/INST fields is based on the assumption that the only nodes which need their type field updated are outside of the CPU core itself so they are pretty much guaranteed to be UNIFIED. Are you hitting this case?
Thanks,
Quoted 45 lines · Jeremy Linton
Hi Jeffrey,
(+Sudeep)
Quoted 40 lines · Jeffrey Hugo
The type of a cache might not be specified by architectural mechanisms (ie system registers), but its type might be specified in the PPTT. In this case, following the PPTT specification, we should identify the cache as the type specified by PPTT.
This fixes the following lscpu issue where only the cache type sysfs file is missing which results in no output providing a poor user experience in the above system configuration-
lscpu: cannot open/sys/devices/system/cpu/cpu0/cache/index3/type: No such file or directoryFixes 2bd00bcd73e5 (ACPI/PPTT: Add Processor Properties Topology Table parsing)Reported-by Vijaya Kumar K <[email protected]>Signed-off-by Jeffrey Hugo <[email protected]>---
drivers/acpi/pptt.c| 15 +++++++++++++++1 file changed, 15 insertions(+)drivers/acpi/pptt.c +13
-0@@ -401,6 +401,21 @@ static void update_cache_properties(structcacheinfo *this_leaf, break; } } + if ((this_leaf->type == CACHE_TYPE_NOCACHE) && + (found_cache->flags & ACPI_PPTT_CACHE_TYPE_VALID)) { + switch (found_cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) { + case ACPI_PPTT_CACHE_TYPE_DATA: + this_leaf->type = CACHE_TYPE_DATA; + break; + case ACPI_PPTT_CACHE_TYPE_INSTR: + this_leaf->type = CACHE_TYPE_INST; + break; + case ACPI_PPTT_CACHE_TYPE_UNIFIED: + case ACPI_PPTT_CACHE_TYPE_UNIFIED_ALT: + this_leaf->type = CACHE_TYPE_UNIFIED; + break;
Quoted 8 lines
Quoted 6 lines
drivers/acpi/pptt.c +2
-0+ } + } /** If the above flags are valid, and the cache type is NOCACHE * update the cache type as well.
If you look at the next line of code following this comment its going to update the cache type for fully populated PPTT nodes. Although with the suggested change its only going to activate if someone completely fills out the node and fails to set the valid flag on the cache type.
Yes, however that case doesn't apply to the scenario we are concerned about, doesn't seem to be fully following the PPTT spec, and seems odd that Linux just assumes that a "fully specified" cache is unified.
Quoted 4 lines
What I suspect is happening in the reported case is that the nodes in the PPTT table are missing fields we consider to be important. Since that data isn't being filled out anywhere else, so we leave the cache type alone too. This has the effect of hiding sysfs nodes with incomplete information.
Also, the lack of the DATA/INST fields is based on the assumption that the only nodes which need their type field updated are outside of the CPU core itself so they are pretty much guaranteed to be UNIFIED. Are you hitting this case?
Yes. Without this change, we hit the lscpu error in the commit message, and get zero output about the system. We don't even get information about the caches which are architecturally specified or how many cpus are present. With this change, we get what we expect out of lscpu (and also lstopo) including the cache(s) which are not architecturally specified.
I guess I still don't understand why its important for PPTT to list, for example, the sets/ways of a cache in all scenarios. In the case of a "transparent" cache (implementation defined as not reported per section D3.4.2 of the ARM ARM where the cache cannot be managed by SW), there may not be valid values for sets/ways. I would argue its better to not report that information, rather than provide bogus information just to make Linux happy, which may break other OSes and provide means for which a user to hang themselves.
However, in the case of a transparent cache, the size/type/level/write policy/etc (whatever the firmware provider deems relevant) might be valuable information for the OS to make scheduling decisions, and is certainly valuable for user space utilities for cross-machine/data center level job scheduling.
Signature · 4 lines
Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Hi,
Quoted 55 lines · Jeffrey Hugo
Quoted 45 lines · Jeremy Linton
Hi Jeffrey,
(+Sudeep)
Quoted 40 lines · Jeffrey Hugo
The type of a cache might not be specified by architectural mechanisms (ie system registers), but its type might be specified in the PPTT. In this case, following the PPTT specification, we should identify the cache as the type specified by PPTT.
This fixes the following lscpu issue where only the cache type sysfs file is missing which results in no output providing a poor user experience in the above system configuration-
lscpu: cannot open/sys/devices/system/cpu/cpu0/cache/index3/type: No such file or directoryFixes 2bd00bcd73e5 (ACPI/PPTT: Add Processor Properties Topology Table parsing)Reported-by Vijaya Kumar K <[email protected]>Signed-off-by Jeffrey Hugo <[email protected]>---
drivers/acpi/pptt.c| 15 +++++++++++++++1 file changed, 15 insertions(+)drivers/acpi/pptt.c +13
-0@@ -401,6 +401,21 @@ static void update_cache_properties(structcacheinfo *this_leaf, break; } } + if ((this_leaf->type == CACHE_TYPE_NOCACHE) && + (found_cache->flags & ACPI_PPTT_CACHE_TYPE_VALID)) { + switch (found_cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) { + case ACPI_PPTT_CACHE_TYPE_DATA: + this_leaf->type = CACHE_TYPE_DATA; + break; + case ACPI_PPTT_CACHE_TYPE_INSTR: + this_leaf->type = CACHE_TYPE_INST; + break; + case ACPI_PPTT_CACHE_TYPE_UNIFIED: + case ACPI_PPTT_CACHE_TYPE_UNIFIED_ALT: + this_leaf->type = CACHE_TYPE_UNIFIED; + break;Quoted 8 lines
Quoted 6 lines
drivers/acpi/pptt.c +2
-0+ } + } /** If the above flags are valid, and the cache type is NOCACHE * update the cache type as well.
If you look at the next line of code following this comment its going to update the cache type for fully populated PPTT nodes. Although with the suggested change its only going to activate if someone completely fills out the node and fails to set the valid flag on the cache type.
Quoted 1 line
Yes, however that case doesn't apply to the scenario we are concerned about, doesn't seem to be fully following the PPTT spec, and seems odd that Linux just assumes that a "fully specified" cache is unified.
Because, the architecturally specified ones won't be type NOCACHE?
Quoted 7 lines
Quoted 5 lines
What I suspect is happening in the reported case is that the nodes in the PPTT table are missing fields we consider to be important. Since that data isn't being filled out anywhere else, so we leave the cache type alone too. This has the effect of hiding sysfs nodes with incomplete information.
Also, the lack of the DATA/INST fields is based on the assumption that the only nodes which need their type field updated are outside of the CPU core itself so they are pretty much guaranteed to be UNIFIED. Are you hitting this case?
Yes. Without this change, we hit the lscpu error in the commit message, and get zero output about the system. We don't even get information about the caches which are architecturally specified or how many cpus are present. With this change, we get what we expect out of lscpu (and also lstopo) including the cache(s) which are not architecturally specified.
I'm a bit surprised this changes the behavior of the architecturally specified ones. As I mentioned above, those shouldn't be NOCACHE. We use the level/type as a key for matching a PPTT node to an architecturally described cache. If that is mismatched, something more fundamental is happening. About the only case I can think of that can cause this is if the CLIDR type fields are incorrect.
In which case I might suggest we move your switch() for the INST/DATA into the check below that comment.
Quoted 2 lines
I guess I still don't understand why its important for PPTT to list, for example, the sets/ways of a cache in all scenarios. In the case of a "transparent" cache (implementation defined as not reported per section D3.4.2 of the ARM ARM where the cache cannot be managed by SW), there may not be valid values for sets/ways. I would argue its better to not report that information, rather than provide bogus information just to make Linux happy, which may break other OSes and provide means for which a user to hang themselves.
This doesn't really have anything to do with the Arm/ARM's definition of set/way as it pertains to cache maint. Its more to assist userspace software with an understanding of the system cache architecture so it can make intelligent decisions about loop tiling and the like.
What we want is a consistent dependable view for software to utilize. An unreliable sysfs field is one that tends not to get used.
Quoted 4 lines
However, in the case of a transparent cache, the size/type/level/write policy/etc (whatever the firmware provider deems relevant) might be valuable information for the OS to make scheduling decisions, and is certainly valuable for user space utilities for cross-machine/data ou center level job scheduling.
Quoted 58 lines · Jeremy Linton
Hi,
Quoted 55 lines · Jeffrey Hugo
Quoted 53 lines · Jeremy Linton
Hi Jeffrey,
(+Sudeep)
Quoted 44 lines · Jeffrey Hugo
The type of a cache might not be specified by architectural mechanisms (ie system registers), but its type might be specified in the PPTT. In this case, following the PPTT specification, we should identify the cache as the type specified by PPTT.
This fixes the following lscpu issue where only the cache type sysfs file is missing which results in no output providing a poor user experience in the above system configuration-
lscpu: cannot open/sys/devices/system/cpu/cpu0/cache/index3/type: No such file or directoryFixes 2bd00bcd73e5 (ACPI/PPTT: Add Processor Properties Topology Table parsing)Reported-by Vijaya Kumar K <[email protected]>Signed-off-by Jeffrey Hugo <[email protected]>---
drivers/acpi/pptt.c| 15 +++++++++++++++1 file changed, 15 insertions(+)drivers/acpi/pptt.c +15
-0@@ -401,6 +401,21 @@ static void update_cache_properties(structcacheinfo *this_leaf, break; } } + if ((this_leaf->type == CACHE_TYPE_NOCACHE) && + (found_cache->flags & ACPI_PPTT_CACHE_TYPE_VALID)) { + switch (found_cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) { + case ACPI_PPTT_CACHE_TYPE_DATA: + this_leaf->type = CACHE_TYPE_DATA; + break; + case ACPI_PPTT_CACHE_TYPE_INSTR: + this_leaf->type = CACHE_TYPE_INST; + break; + case ACPI_PPTT_CACHE_TYPE_UNIFIED: + case ACPI_PPTT_CACHE_TYPE_UNIFIED_ALT: + this_leaf->type = CACHE_TYPE_UNIFIED; + break; + } + } /** If the above flags are valid, and the cache type is NOCACHE Quoted 2 lines
* update the cache type as well.
If you look at the next line of code following this comment its going to update the cache type for fully populated PPTT nodes. Although with the suggested change its only going to activate if someone completely fills out the node and fails to set the valid flag on the cache type.
Quoted 3 lines
Quoted 1 line
Yes, however that case doesn't apply to the scenario we are concerned about, doesn't seem to be fully following the PPTT spec, and seems odd that Linux just assumes that a "fully specified" cache is unified.
Because, the architecturally specified ones won't be type NOCACHE?
Correct. However, what if you have a NOCACHE (not architecturally specified), that is fully described in PPTT, as a non-unified cache (data only)? Unlikely? Maybe. Still seem possible though, therefore I feel this assumption is suspect.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
What I suspect is happening in the reported case is that the nodes in the PPTT table are missing fields we consider to be important. Since that data isn't being filled out anywhere else, so we leave the cache type alone too. This has the effect of hiding sysfs nodes with incomplete information.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Also, the lack of the DATA/INST fields is based on the assumption that the only nodes which need their type field updated are outside of the CPU core itself so they are pretty much guaranteed to be UNIFIED. Are you hitting this case?
Yes. Without this change, we hit the lscpu error in the commit message, and get zero output about the system. We don't even get information about the caches which are architecturally specified or how many cpus are present. With this change, we get what we expect out of lscpu (and also lstopo) including the cache(s) which are not architecturally specified.
I'm a bit surprised this changes the behavior of the architecturally specified ones. As I mentioned above, those shouldn't be NOCACHE. We use the level/type as a key for matching a PPTT node to an architecturally described cache. If that is mismatched, something more fundamental is happening. About the only case I can think of that can cause this is if the CLIDR type fields are incorrect.
In which case I might suggest we move your
switch()for the INST/DATA into the check below that comment.
This change was not intended to impact architecturally specified ones, however I can see how that is the case. That would be the case if the architecturally specified type is X and PPTT indicates Y. According to the PPTT spec, the cache should be treated as type Y then (ie the contents of the PPTT override the architectural mechanisms).
I can move my change below the current NOCACHE handling, which would be valid for my scenario, but it seems odd. If I do that, then the current assumption will take priority. IE, if a cache is "fully specified" in PPTT, but is NOCACHE, then it will be treated as unified, regardless of what PPTT says (maybe instruction, or data only).
Quoted 7 lines
Quoted 3 lines
I guess I still don't understand why its important for PPTT to list, for example, the sets/ways of a cache in all scenarios. In the case of a "transparent" cache (implementation defined as not reported per section D3.4.2 of the ARM ARM where the cache cannot be managed by SW), there may not be valid values for sets/ways. I would argue its better to not report that information, rather than provide bogus information just to make Linux happy, which may break other OSes and provide means for which a user to hang themselves.
This doesn't really have anything to do with the Arm/ARM's definition of set/way as it pertains to cache maint. Its more to assist userspace software with an understanding of the system cache architecture so it can make intelligent decisions about loop tiling and the like.
What we want is a consistent dependable view for software to utilize. An unreliable sysfs field is one that tends not to get used.
I guess my argument is this -
The cache is not specified architecturally because it cannot be managed by software (see the ARM ARM section I referenced).
However, its important that the user be able to "see" it, I'm not a marketing guy, but I assume its going to be listed on the "data sheet".
Therefore, the firmware is providing some information about the cache (via SMBIOS tables and PPTT).
The HW designers have indicated that there is no sane way to provide sets/ways information to software, even on an informational basis (ie not for cache maintenance, but for performance optimizations). Therefore the firmware will not provide this information because it will be wrong.
So, therefore, we should still be able to tell the user that a cache exists at the relevant level, and what size it is. On the concerned system, we cannot do that currently.
Quoted 5 lines
Quoted 5 lines
However, in the case of a transparent cache, the size/type/level/write policy/etc (whatever the firmware provider deems relevant) might be valuable information for the OS to make scheduling decisions, and is certainly valuable for user space utilities for cross-machine/data ou center level job scheduling.
Signature · 4 lines
Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 75 lines · Jeffrey Hugo
Quoted 74 lines · Jeremy Linton
Hi,
Quoted 70 lines · Jeffrey Hugo
Quoted 57 lines · Jeremy Linton
Hi Jeffrey,
(+Sudeep)
Quoted 52 lines · Jeffrey Hugo
The type of a cache might not be specified by architectural mechanisms (ie
system registers), but its type might be specified in the PPTT. In this
case, following the PPTT specification, we should identify the cache as
the type specified by PPTT.This fixes the following lscpu issue where only the cache type sysfs file
is missing which results in no output providing a poor user experience in
the above system configuration-
lscpu: cannot open/sys/devices/system/cpu/cpu0/cache/index3/type:
No such
file or directoryFixes 2bd00bcd73e5 (ACPI/PPTT: Add Processor Properties TopologyTable parsing)
Reported-by Vijaya Kumar K <[email protected]>Signed-off-by Jeffrey Hugo <[email protected]>---
drivers/acpi/pptt.c| 15 +++++++++++++++1 file changed, 15 insertions(+)drivers/acpi/pptt.c +15
-0@@ -401,6 +401,21 @@ static void update_cache_properties(structcacheinfo *this_leaf, break; } } + if ((this_leaf->type == CACHE_TYPE_NOCACHE) && + (found_cache->flags & ACPI_PPTT_CACHE_TYPE_VALID)) { + switch (found_cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) { + case ACPI_PPTT_CACHE_TYPE_DATA: + this_leaf->type = CACHE_TYPE_DATA; + break; + case ACPI_PPTT_CACHE_TYPE_INSTR: + this_leaf->type = CACHE_TYPE_INST; + break; + case ACPI_PPTT_CACHE_TYPE_UNIFIED: + case ACPI_PPTT_CACHE_TYPE_UNIFIED_ALT: + this_leaf->type = CACHE_TYPE_UNIFIED; + break; + } + } /** If the above flags are valid, and the cache type is NOCACHE
Quoted 8 lines
Quoted 2 lines
* update the cache type as well.
If you look at the next line of code following this comment its going to update the cache type for fully populated PPTT nodes. Although with the suggested change its only going to activate if someone completely fills out the node and fails to set the valid flag on the cache type.
Yes, however that case doesn't apply to the scenario we are concerned about, doesn't seem to be fully following the PPTT spec, and seems odd that Linux just assumes that a "fully specified" cache is unified.
Quoted 7 lines
Quoted 1 line
Because, the architecturally specified ones won't be type NOCACHE?
Correct. However, what if you have a NOCACHE (not architecturally specified), that is fully described in PPTT, as a non-unified cache (data only)? Unlikely? Maybe. Still seem possible though, therefore I feel this assumption is suspect.
Yes, we have other issues if the architecturally not specified cache is not unified irrespective of what PPTT says. So we may need to review and see if that assumption is removed everywhere.
Until then why can't a simple change fix the issue you have:
-->8
drivers/acpi/pptt.c +0 -0
@@ -406,7 +406,8 @@ static void update_cache_properties(struct cacheinfo
*this_leaf,
* update the cache type as well.
*/
if (this_leaf->type == CACHE_TYPE_NOCACHE &&
- valid_flags == PPTT_CHECKED_ATTRIBUTES)
+ (valid_flags == PPTT_CHECKED_ATTRIBUTES ||
+ found_cache->flags & ACPI_PPTT_CACHE_TYPE_VALID))
this_leaf->type = CACHE_TYPE_UNIFIED;}
[...]
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Yes. Without this change, we hit the lscpu error in the commit message, and get zero output about the system. We don't even get information about the caches which are architecturally specified or how many cpus are present.
The above issues is purely lscpu to blame and nothing to do with kernel change. If kernel doesn't report level 3 cache info and lscpu fails to report level 1/2 cache info and kernel is to blame here ? That's unfair.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
With this change, we get what we expect
out of lscpu (and also lstopo) including the cache(s) which are not architecturally specified.
Ofcourse, but we can't attribute that as kernel bug. I agree we should not show incorrect info for cache type and that needs to be fixed but disagree with lscpu issue as kernel issue. Kernel is not breaking any ABI as cacheinfo itself is optional and the absence of it needs to be dealt with.
Quoted 16 lines
Quoted 9 lines
I'm a bit surprised this changes the behavior of the architecturally specified ones. As I mentioned above, those shouldn't be NOCACHE. We use the level/type as a key for matching a PPTT node to an architecturally described cache. If that is mismatched, something more fundamental is happening. About the only case I can think of that can cause this is if the CLIDR type fields are incorrect.
In which case I might suggest we move your
switch()for the INST/DATA into the check below that comment.This change was not intended to impact architecturally specified ones, however I can see how that is the case. That would be the case if the architecturally specified type is X and PPTT indicates Y. According to the PPTT spec, the cache should be treated as type Y then (ie the contents of the PPTT override the architectural mechanisms).
Quoted 6 lines
I can move my change below the current NOCACHE handling, which would be valid for my scenario, but it seems odd. If I do that, then the current assumption will take priority. IE, if a cache is "fully specified" in PPTT, but is NOCACHE, then it will be treated as unified, regardless of what PPTT says (maybe instruction, or data only).
Yes it should be that way for above mentioned reasons. You need to fix other things if you want to support separate data and inst cache for architecturally unspecified cache.
[...]
Quoted 4 lines
However, its important that the user be able to "see" it, I'm not a marketing guy, but I assume its going to be listed on the "data sheet".
Quoted 8 lines
Therefore, the firmware is providing some information about the cache (via SMBIOS tables and PPTT).
The HW designers have indicated that there is no sane way to provide sets/ways information to software, even on an informational basis (ie not for cache maintenance, but for performance optimizations). Therefore the firmware will not provide this information because it will be wrong.
Fair enough.
Quoted 4 lines
So, therefore, we should still be able to tell the user that a cache exists at the relevant level, and what size it is. On the concerned system, we cannot do that currently.
Again agreed.
Signature · 2 lines
Regards, Sudeep
Quoted 3 lines · Sudeep Holla
On 12/09/18 15:41, Jeffrey Hugo wrote:
[...]
Quoted 20 lines
Quoted 6 lines
Correct. However, what if you have a NOCACHE (not architecturally specified), that is fully described in PPTT, as a non-unified cache (data only)? Unlikely? Maybe. Still seem possible though, therefore I feel this assumption is suspect.
Yes, we have other issues if the architecturally not specified cache is not unified irrespective of what PPTT says. So we may need to review and see if that assumption is removed everywhere.
Until then why can't a simple change fix the issue you have:
-->8
drivers/acpi/pptt.c +0
-0
drivers/acpi/pptt.c static void update_cache_properties(struct cacheinfo · line 411
Quoted 7 lines
drivers/acpi/pptt.c +2
-1@@ -406,7 +406,8 @@ static void update_cache_properties(struct cacheinfo*this_leaf, * update the cache type as well. */ if (this_leaf->type ==CACHE_TYPE_NOCACHE && - valid_flags ==PPTT_CHECKED_ATTRIBUTES ) + (valid_flags ==PPTT_CHECKED_ATTRIBUTES || + found_cache->flags &ACPI_PPTT_CACHE_TYPE_VALID ))
Looking at this again, if we are supporting just presence of cache type and size(may be), then we can drop the whole valid_flags thing here.
Quoted 3 lines
drivers/acpi/pptt.c +0
-0this_leaf->type =CACHE_TYPE_UNIFIED ; }
Signature · 2 lines
Regards, Sudeep
Quoted 24 lines · Sudeep Holla
Quoted 3 lines · Sudeep Holla
On 12/09/18 15:41, Jeffrey Hugo wrote:
[...]
Quoted 15 lines
Quoted 6 lines
Correct. However, what if you have a NOCACHE (not architecturally specified), that is fully described in PPTT, as a non-unified cache (data only)? Unlikely? Maybe. Still seem possible though, therefore I feel this assumption is suspect.
Yes, we have other issues if the architecturally not specified cache is not unified irrespective of what PPTT says. So we may need to review and see if that assumption is removed everywhere.
Until then why can't a simple change fix the issue you have:
-->8
Quoted 19 lines
drivers/acpi/pptt.c static void update_cache_properties(struct cacheinfo · line 411
Quoted 12 lines
drivers/acpi/pptt.c +2
-1@@ -406,7 +406,8 @@ static void update_cache_properties(struct cacheinfo*this_leaf, * update the cache type as well. */ if (this_leaf->type ==CACHE_TYPE_NOCACHE && - valid_flags ==PPTT_CHECKED_ATTRIBUTES ) + (valid_flags ==PPTT_CHECKED_ATTRIBUTES || + found_cache->flags &ACPI_PPTT_CACHE_TYPE_VALID ))Looking at this again, if we are supporting just presence of cache type and size(may be), then we can drop the whole
valid_flagsthing here.Quoted 3 lines
drivers/acpi/pptt.c +0
-0this_leaf->type =CACHE_TYPE_UNIFIED ; }
Yes, this change fixes my usecase. I think it invalidates the comment, and really, the comment should probably mention that we assume unified type because there are other issues in supporting architecturally not specified inst/data only caches.
Do you want a V2 with this? If so, do you want the fixes tag removed since you seem to view this as not a bug?
I don't think I clearly understand the purpose of the valid flags, therefore I feel as though I'm not sure if it can be dropped or not. Is it fair to say that what the valid flags is accomplishing is identifying if we have a sufficient level of information to support this cache? If not, then should the cacheinfo driver not expose any sysfs information about the cache?
Signature · 4 lines
Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 41 lines · Jeffrey Hugo
Quoted 40 lines · Sudeep Holla
Quoted 3 lines · Sudeep Holla
On 12/09/18 15:41, Jeffrey Hugo wrote:
[...]
Quoted 20 lines
Quoted 6 lines
Correct. However, what if you have a NOCACHE (not architecturally specified), that is fully described in PPTT, as a non-unified cache (data only)? Unlikely? Maybe. Still seem possible though, therefore I feel this assumption is suspect.
Yes, we have other issues if the architecturally not specified cache is not unified irrespective of what PPTT says. So we may need to review and see if that assumption is removed everywhere.
Until then why can't a simple change fix the issue you have:
-->8
drivers/acpi/pptt.c +0
-0drivers/acpi/pptt.c static void update_cache_properties(struct cacheinfo · line 411
Quoted 7 lines
drivers/acpi/pptt.c +2
-1@@ -406,7 +406,8 @@ static void update_cache_properties(struct cacheinfo*this_leaf, * update the cache type as well. */ if (this_leaf->type ==CACHE_TYPE_NOCACHE && - valid_flags ==PPTT_CHECKED_ATTRIBUTES ) + (valid_flags ==PPTT_CHECKED_ATTRIBUTES || + found_cache->flags &ACPI_PPTT_CACHE_TYPE_VALID ))Looking at this again, if we are supporting just presence of cache type and size(may be), then we can drop the whole
valid_flagsthing here.
Quoted 9 lines
Quoted 3 lines
drivers/acpi/pptt.c static void update_cache_properties(struct cacheinfo · line 413
Quoted 3 lines
drivers/acpi/pptt.c +0
-0this_leaf->type =CACHE_TYPE_UNIFIED ; }Yes, this change fixes my usecase. I think it invalidates the comment, and really, the comment should probably mention that we assume unified type because there are other issues in supporting architecturally not specified inst/data only caches.
Agreed.
Quoted 3 lines
Do you want a V2 with this? If so, do you want the fixes tag removed since you seem to view this as not a bug?
Yes please, I am fine to retain fixes tag but that's my opinion.
Quoted 6 lines
I don't think I clearly understand the purpose of the valid flags, therefore I feel as though I'm not sure if it can be dropped or not. Is it fair to say that what the valid flags is accomplishing is identifying if we have a sufficient level of information to support this cache? If not, then should the cacheinfo driver not expose any sysfs information about the cache?
I don't see the use of the flag if we *have to* support the case where all the cache geometry is not present but just cache type (and maybe size?) is present. If that's the case we can drop valid flags entirely. I really don't like the idea of supporting that, but I don't have strong reasons to defend my idea, so I am fine with that.
Further, I think in your case with NOCACHE type set, sysfs dir shouldn't have been created at the first place ideally. I think we need something like below to fix that.
-->8
drivers/base/cacheinfo.c +2 -0
@@ -615,6 +615,8 @@ static int cache_add_dev(unsigned int cpu) this_leaf = this_cpu_ci->info_list + i;if (this_leaf->disable_sysfs)continue ; +if (this_leaf->type ==CACHE_TYPE_NOCACHE ) +break ; cache_groups =cache_get_attribute_groups (this_leaf); ci_dev =cpu_device_create (parent, this_leaf, cache_groups,"index%1u" , i);
Quoted 85 lines · Sudeep Holla
Quoted 41 lines · Jeffrey Hugo
Quoted 40 lines · Sudeep Holla
Quoted 3 lines · Sudeep Holla
On 12/09/18 15:41, Jeffrey Hugo wrote:
[...]
Quoted 20 lines
Quoted 6 lines
Correct. However, what if you have a NOCACHE (not architecturally specified), that is fully described in PPTT, as a non-unified cache (data only)? Unlikely? Maybe. Still seem possible though, therefore I feel this assumption is suspect.
Yes, we have other issues if the architecturally not specified cache is not unified irrespective of what PPTT says. So we may need to review and see if that assumption is removed everywhere.
Until then why can't a simple change fix the issue you have:
-->8
drivers/acpi/pptt.c +0
-0drivers/acpi/pptt.c static void update_cache_properties(struct cacheinfo · line 411
Quoted 7 lines
drivers/acpi/pptt.c +2
-1@@ -406,7 +406,8 @@ static void update_cache_properties(struct cacheinfo*this_leaf, * update the cache type as well. */ if (this_leaf->type ==CACHE_TYPE_NOCACHE && - valid_flags ==PPTT_CHECKED_ATTRIBUTES ) + (valid_flags ==PPTT_CHECKED_ATTRIBUTES || + found_cache->flags &ACPI_PPTT_CACHE_TYPE_VALID ))Looking at this again, if we are supporting just presence of cache type and size(may be), then we can drop the whole
valid_flagsthing here.Quoted 9 lines
Quoted 3 lines
drivers/acpi/pptt.c static void update_cache_properties(struct cacheinfo · line 413
Quoted 3 lines
drivers/acpi/pptt.c +0
-0this_leaf->type =CACHE_TYPE_UNIFIED ; }Yes, this change fixes my usecase. I think it invalidates the comment, and really, the comment should probably mention that we assume unified type because there are other issues in supporting architecturally not specified inst/data only caches.
Agreed.
Quoted 3 lines
Do you want a V2 with this? If so, do you want the fixes tag removed since you seem to view this as not a bug?
Yes please, I am fine to retain fixes tag but that's my opinion.
Quoted 6 lines
I don't think I clearly understand the purpose of the valid flags, therefore I feel as though I'm not sure if it can be dropped or not. Is it fair to say that what the valid flags is accomplishing is identifying if we have a sufficient level of information to support this cache? If not, then should the cacheinfo driver not expose any sysfs information about the cache?
I don't see the use of the flag if we *have to* support the case where all the cache geometry is not present but just cache type (and maybe size?) is present. If that's the case we can drop valid flags entirely. I really don't like the idea of supporting that, but I don't have strong reasons to defend my idea, so I am fine with that.
Further, I think in your case with NOCACHE type set, sysfs dir shouldn't have been created at the first place ideally. I think we need something like below to fix that.
-->8
drivers/base/cacheinfo.c +0
-0
drivers/base/cacheinfo.c static int cache_add_dev(unsigned int cpu) · line 622
Quoted 8 lines
drivers/base/cacheinfo.c +2
-0@@ -615,6 +615,8 @@ static int cache_add_dev(unsigned int cpu) this_leaf = this_cpu_ci->info_list + i;if (this_leaf->disable_sysfs)continue ; +if (this_leaf->type ==CACHE_TYPE_NOCACHE ) +break ; cache_groups =cache_get_attribute_groups (this_leaf); ci_dev =cpu_device_create (parent, this_leaf, cache_groups,"index%1u" , i);
Ok, let me test this out, and send out a v2.
Signature · 4 lines
Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Hi,
Quoted 3 lines · Jeffrey Hugo
The HW designers have indicated that there is no sane way to provide sets/ways information to software, even on an informational basis (ie not for cache maintenance, but for performance optimizations). Therefore the firmware will not provide this information because it will be wrong.
So, therefore, we should still be able to tell the user that a cache exists at the relevant level, and what size it is. On the concerned system, we cannot do that currently.
Ok, so set the fields to zero in firmware node, and mark them valid.
That logically says that there isn't any set/way information for the cache (which implies direct mapped).
Quoted 9 lines · Jeremy Linton
Hi,
Quoted 3 lines · Jeffrey Hugo
The HW designers have indicated that there is no sane way to provide sets/ways information to software, even on an informational basis (ie not for cache maintenance, but for performance optimizations). Therefore the firmware will not provide this information because it will be wrong.
So, therefore, we should still be able to tell the user that a cache exists at the relevant level, and what size it is. On the concerned system, we cannot do that currently.
Ok, so set the fields to zero in firmware node, and mark them valid.
Is that what the PPTT spec says to do?
Quoted 1 line
That logically says that there isn't any set/way information for the cache (which implies direct mapped).
Making inferences such as that have gotten folks into trouble when interpreting other specs. Unfortunately I am not allowed to detail more about this specific system, however implying that the affected cache(s) are direct mapped is incorrect. Officially, what you have is a cache or multiple caches at certain levels that have a specified size. You can make no inferences about the exact behavior or implementation of the cache beyond what FW explicitly provides. I'm not particularly a fan of it, but its what I have to deal with.
Signature · 4 lines
Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 5 lines · Jeffrey Hugo
Quoted 4 lines · Jeremy Linton
Hi Jeffrey,
(+Sudeep)
[..]
Quoted 10 lines
Quoted 5 lines
If you look at the next line of code following this comment its going to update the cache type for fully populated PPTT nodes. Although with the suggested change its only going to activate if someone completely fills out the node and fails to set the valid flag on the cache type.
Yes, however that case doesn't apply to the scenario we are concerned about, doesn't seem to be fully following the PPTT spec, and seems odd that Linux just assumes that a "fully specified" cache is unified.
Agreed, but adding code that will never get used is also not so good. Do you have systems where this is needed ?
Quoted 12 lines
Quoted 11 lines
What I suspect is happening in the reported case is that the nodes in the PPTT table are missing fields we consider to be important. Since that data isn't being filled out anywhere else, so we leave the cache type alone too. This has the effect of hiding sysfs nodes with incomplete information.
Also, the lack of the DATA/INST fields is based on the assumption that the only nodes which need their type field updated are outside of the CPU core itself so they are pretty much guaranteed to be UNIFIED. Are you hitting this case?
Quoted 7 lines
Yes. Without this change, we hit the lscpu error in the commit message, and get zero output about the system. We don't even get information about the caches which are architecturally specified or how many cpus are present. With this change, we get what we expect out of lscpu (and also lstopo) including the cache(s) which are not architecturally specified.
lscpu and lstopo are so broken. They just assume everything on CPU0. If you hotplug them out, you start seeing issues. So reading and file that doesn't exist and then bail out on other essential info though they are present, hmmm ...
Quoted 9 lines
I guess I still don't understand why its important for PPTT to list, for example, the sets/ways of a cache in all scenarios. In the case of a "transparent" cache (implementation defined as not reported per section D3.4.2 of the ARM ARM where the cache cannot be managed by SW), there may not be valid values for sets/ways. I would argue its better to not report that information, rather than provide bogus information just to make Linux happy, which may break other OSes and provide means for which a user to hang themselves.
While I agree presenting wrong info is not good, but one of the reasons the cache info is present is to provide those info for some applications to do optimization based on that(I am told so and not sure if just type and size will be good enough) and you seem to agree with that below.
Quoted 6 lines
However, in the case of a transparent cache, the size/type/level/write policy/etc (whatever the firmware provider deems relevant) might be valuable information for the OS to make scheduling decisions, and is certainly valuable for user space utilities for cross-machine/data center level job scheduling.
Signature · 2 lines
Regards, Sudeep
Quoted 17 lines · Sudeep Holla
Quoted 5 lines · Jeffrey Hugo
Quoted 4 lines · Jeremy Linton
Hi Jeffrey,
(+Sudeep)
[..]
Quoted 6 lines
Quoted 5 lines
If you look at the next line of code following this comment its going to update the cache type for fully populated PPTT nodes. Although with the suggested change its only going to activate if someone completely fills out the node and fails to set the valid flag on the cache type.
Quoted 7 lines
Quoted 4 lines
Yes, however that case doesn't apply to the scenario we are concerned about, doesn't seem to be fully following the PPTT spec, and seems odd that Linux just assumes that a "fully specified" cache is unified.
Agreed, but adding code that will never get used is also not so good. Do you have systems where this is needed ?
Yes.
Quoted 21 lines
Quoted 13 lines
Quoted 7 lines
What I suspect is happening in the reported case is that the nodes in the PPTT table are missing fields we consider to be important. Since that data isn't being filled out anywhere else, so we leave the cache type alone too. This has the effect of hiding sysfs nodes with incomplete information.
Quoted 5 lines
Also, the lack of the DATA/INST fields is based on the assumption that the only nodes which need their type field updated are outside of the CPU core itself so they are pretty much guaranteed to be UNIFIED. Are you hitting this case?
Quoted 7 lines
Yes. Without this change, we hit the lscpu error in the commit message, and get zero output about the system. We don't even get information about the caches which are architecturally specified or how many cpus are present. With this change, we get what we expect out of lscpu (and also lstopo) including the cache(s) which are not architecturally specified.
Quoted 4 lines
lscpu and lstopo are so broken. They just assume everything on CPU0. If you hotplug them out, you start seeing issues. So reading and file that doesn't exist and then bail out on other essential info though they are present, hmmm ...
lscpu/lstopo being broken seems orthogonal to me.
The kernel is not providing the type file because the kernel thinks the type is NOCACHE, despite PPTT providing a type. In an ideal world, I think the kernel should be fixed (this change), and any deficiencies or bad assumptions in lscpu/lstopo should also be fixed.
Quoted 11 lines
Quoted 10 lines
I guess I still don't understand why its important for PPTT to list, for example, the sets/ways of a cache in all scenarios. In the case of a "transparent" cache (implementation defined as not reported per section D3.4.2 of the ARM ARM where the cache cannot be managed by SW), there may not be valid values for sets/ways. I would argue its better to not report that information, rather than provide bogus information just to make Linux happy, which may break other OSes and provide means for which a user to hang themselves.
Quoted 4 lines
While I agree presenting wrong info is not good, but one of the reasons the cache info is present is to provide those info for some applications to do optimization based on that(I am told so and not sure if just type and size will be good enough) and you seem to agree with that below.
Yes, but if its not entirely clear, on my system, we have the information but its not being presented. This change fixes that. I'm willing to discuss an alternative fix, but to ignore the issue is not viable in my opinion.
What do we need to move forward here?
Quoted 8 lines
Quoted 7 lines
However, in the case of a transparent cache, the size/type/level/write policy/etc (whatever the firmware provider deems relevant) might be valuable information for the OS to make scheduling decisions, and is certainly valuable for user space utilities for cross-machine/data center level job scheduling.
Signature · 4 lines
Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 23 lines · Jeffrey Hugo
Quoted 22 lines · Sudeep Holla
Quoted 5 lines · Jeffrey Hugo
Quoted 4 lines · Jeremy Linton
Hi Jeffrey,
(+Sudeep)
[..]
Quoted 10 lines
Quoted 5 lines
If you look at the next line of code following this comment its going to update the cache type for fully populated PPTT nodes. Although with the suggested change its only going to activate if someone completely fills out the node and fails to set the valid flag on the cache type.
Yes, however that case doesn't apply to the scenario we are concerned about, doesn't seem to be fully following the PPTT spec, and seems odd that Linux just assumes that a "fully specified" cache is unified.
Quoted 5 lines
Quoted 2 lines
Agreed, but adding code that will never get used is also not so good. Do you have systems where this is needed ?
Yes.
Not exactly IMO. I was referring to architecturally not specified separate data/inst cache.
Quoted 21 lines
Quoted 21 lines
Quoted 20 lines
Quoted 7 lines
What I suspect is happening in the reported case is that the nodes in the PPTT table are missing fields we consider to be important. Since that data isn't being filled out anywhere else, so we leave the cache type alone too. This has the effect of hiding sysfs nodes with incomplete information.
Quoted 5 lines
Also, the lack of the DATA/INST fields is based on the assumption that the only nodes which need their type field updated are outside of the CPU core itself so they are pretty much guaranteed to be UNIFIED. Are you hitting this case?
Yes. Without this change, we hit the lscpu error in the commit message, and get zero output about the system. We don't even get information about the caches which are architecturally specified or how many cpus are present. With this change, we get what we expect out of lscpu (and also lstopo) including the cache(s) which are not architecturally specified.
Quoted 6 lines
Quoted 4 lines
lscpu and lstopo are so broken. They just assume everything on CPU0. If you hotplug them out, you start seeing issues. So reading and file that doesn't exist and then bail out on other essential info though they are present, hmmm ...
lscpu/lstopo being broken seems orthogonal to me.
Agreed.
Quoted 5 lines
The kernel is not providing the type file because the kernel thinks the type is NOCACHE, despite PPTT providing a type. In an ideal world, I think the kernel should be fixed (this change), and any deficiencies or bad assumptions in lscpu/lstopo should also be fixed.
Again agreed as mentioned in the other mail. I just didn't like the attribution of that issue to this kernel bug.
Quoted 16 lines
Quoted 11 lines
Quoted 10 lines
I guess I still don't understand why its important for PPTT to list, for example, the sets/ways of a cache in all scenarios. In the case of a "transparent" cache (implementation defined as not reported per section D3.4.2 of the ARM ARM where the cache cannot be managed by SW), there may not be valid values for sets/ways. I would argue its better to not report that information, rather than provide bogus information just to make Linux happy, which may break other OSes and provide means for which a user to hang themselves.
Quoted 4 lines
While I agree presenting wrong info is not good, but one of the reasons the cache info is present is to provide those info for some applications to do optimization based on that(I am told so and not sure if just type and size will be good enough) and you seem to agree with that below.
Quoted 5 lines
Yes, but if its not entirely clear, on my system, we have the information but its not being presented. This change fixes that. I'm willing to discuss an alternative fix, but to ignore the issue is not viable in my opinion.
Agreed.
Quoted 2 lines
What do we need to move forward here?
Will the simple change I posted address the issue ? I don't want to give an illusion that separate data/inst cache is support for architecturally not specified caches. If it needs to be supported, then it needs to be fixed correctly everywhere not just here.
Signature · 2 lines
Regards, Sudeep
Le 12/09/2018 à 11:49, Sudeep Holla a écrit :
Quoted 8 lines
Quoted 8 lines
Yes. Without this change, we hit the lscpu error in the commit message, and get zero output about the system. We don't even get information about the caches which are architecturally specified or how many cpus are present. With this change, we get what we expect out of lscpu (and also lstopo) including the cache(s) which are not architecturally specified.
Quoted 4 lines
lscpu and lstopo are so broken. They just assume everything on CPU0. If you hotplug them out, you start seeing issues. So reading and file that doesn't exist and then bail out on other essential info though they are present, hmmm ...
Can you elaborate?
I am not sure cpu0 is supposed to be offlineable on Linux. There's no "online" file in /sys/devices/system/cpu/cpu0. That's why former lstopo doesn't like CPU0 being hotplugged out. We are actually making that case work for another non-standard corner case. But offlining "cpu0" this is considered "normal", somebody must add that missing "online" sysfs attribute for "cpu0" (change
https://elixir.bootlin.com/linux/latest/source/drivers/base/cpu.c#L375).
By the way, did anybody actually see an error with lstopo when there's no "type" attribute for L3? I can't reproduce any issue, we just skip that specific cache entirely, but everything else appears. If you guys want to make that "no_cache" cache appear, I'll make it a Unified cache unless you tell me what to show :)
Brice
Hi Brice,
Quoted 15 lines · Brice Goglin
Le 12/09/2018 à 11:49, Sudeep Holla a écrit :
Quoted 11 lines
Quoted 7 lines
Yes. Without this change, we hit the lscpu error in the commit message, and get zero output about the system. We don't even get information about the caches which are architecturally specified or how many cpus are present. With this change, we get what we expect out of lscpu (and also lstopo) including the cache(s) which are not architecturally specified.
lscpu and lstopo are so broken. They just assume everything on CPU0. If you hotplug them out, you start seeing issues. So reading and file that doesn't exist and then bail out on other essential info though they are present, hmmm ...
Can you elaborate?
Quoted 7 lines
I am not sure cpu0 is supposed to be offlineable on Linux. There's no "online" file in
/sys/devices/system/cpu/cpu0. That's why former lstopo doesn't like CPU0 being hotplugged out. We are actually making that case work for another non-standard corner case. But offlining "cpu0" this is considered "normal", somebody must add that missing "online" sysfs attribute for "cpu0" (change
https://elixir.bootlin.com/linux/latest/source/drivers/base/cpu.c#L375).
On x86 you can't normally offline CPU0, its something to do with certain interrupts always being routed to CPU0, (oh, and hibernate).
You should be able to enable this behaviour with 'cpu0_hotplug' on the kernel command line.
(Kconfig's CONFIG_BOOTPARAM_HOTPLUG_CPU0 and CONFIG_DEBUG_HOTPLUG_CPU0 are also worth a look)
On arm64 at least, cpu0 is just like the others, and can be offlined.
Thanks,
James
Quoted 5 lines
By the way, did anybody actually see an error with lstopo when there's no "type" attribute for L3? I can't reproduce any issue, we just skip that specific cache entirely, but everything else appears. If you guys want to make that "
no_cache" cache appear, I'll make it a Unified cache unless you tell me what to show :)
Quoted 31 lines · James Morse
Hi Brice,
Quoted 15 lines · Brice Goglin
Le 12/09/2018 à 11:49, Sudeep Holla a écrit :
Quoted 11 lines
Quoted 7 lines
Yes. Without this change, we hit the lscpu error in the commit message, and get zero output about the system. We don't even get information about the caches which are architecturally specified or how many cpus are present. With this change, we get what we expect out of lscpu (and also lstopo) including the cache(s) which are not architecturally specified.
lscpu and lstopo are so broken. They just assume everything on CPU0. If you hotplug them out, you start seeing issues. So reading and file that doesn't exist and then bail out on other essential info though they are present, hmmm ...
Can you elaborate?
Quoted 7 lines
I am not sure cpu0 is supposed to be offlineable on Linux. There's no "online" file in
/sys/devices/system/cpu/cpu0. That's why former lstopo doesn't like CPU0 being hotplugged out. We are actually making that case work for another non-standard corner case. But offlining "cpu0" this is considered "normal", somebody must add that missing "online" sysfs attribute for "cpu0" (change
https://elixir.bootlin.com/linux/latest/source/drivers/base/cpu.c#L375).On x86 you can't normally offline CPU0, its something to do with certain interrupts always being routed to CPU0, (oh, and hibernate).
You should be able to enable this behaviour with 'cpu0_hotplug' on the kernel command line.
Quoted 5 lines
(Kconfig's
CONFIG_BOOTPARAM_HOTPLUG_CPU0andCONFIG_DEBUG_HOTPLUG_CPU0are also worth a look)On arm64 at least, cpu0 is just like the others, and can be offlined.
Thanks James, for providing all the details.
To add to the issues I spotted with lscpu/lstopo around topology, it ignores the updates to topology sibling masks when CPUs are hotplugged in and out.
We have following in lscpu:
add_summary_n(tb, _("Core(s) per socket:"),
cores_per_socket ?: desc->ncores / desc->nsockets);Now when cores_per_socket = 1, (i.e when we don't have procfs entry), if ncores = (ncores_max - few_cpus_hotplugged_out), core(s) per socket will get computed as less than the actual number.
IMO lscpu should be used only when all CPUs are online and it should have a warning when all cores are not online.
Quoted 5 lines
Quoted 5 lines
By the way, did anybody actually see an error with lstopo when there's no "type" attribute for L3? I can't reproduce any issue, we just skip that specific cache entirely, but everything else appears. If you guys want to make that "
no_cache" cache appear, I'll make it a Unified cache unless you tell me what to show :)
IIUC, Jeffrey Hugo did see error as per his initial message:
"
This fixes the following lscpu issue where only the cache type sysfs file is missing which results in no output providing a poor user experience in the above system configuration.
lscpu: cannot open /sys/devices/system/cpu/cpu0/cache/index3/type: No such file or directory
"
--
Regards,
Sudeep
Le 13/09/2018 à 11:35, Sudeep Holla a écrit :
Quoted 56 lines · James Morse
Quoted 29 lines
Hi Brice,
Quoted 14 lines · Brice Goglin
Le 12/09/2018 à 11:49, Sudeep Holla a écrit :
Quoted 11 lines
Quoted 7 lines
Yes. Without this change, we hit the lscpu error in the commit message, and get zero output about the system. We don't even get information about the caches which are architecturally specified or how many cpus are present. With this change, we get what we expect out of lscpu (and also lstopo) including the cache(s) which are not architecturally specified.
lscpu and lstopo are so broken. They just assume everything on CPU0. If you hotplug them out, you start seeing issues. So reading and file that doesn't exist and then bail out on other essential info though they are present, hmmm ...
Can you elaborate?
Quoted 7 lines
I am not sure cpu0 is supposed to be offlineable on Linux. There's no "online" file in
/sys/devices/system/cpu/cpu0. That's why former lstopo doesn't like CPU0 being hotplugged out. We are actually making that case work for another non-standard corner case. But offlining "cpu0" this is considered "normal", somebody must add that missing "online" sysfs attribute for "cpu0" (change
https://elixir.bootlin.com/linux/latest/source/drivers/base/cpu.c#L375).On x86 you can't normally offline CPU0, its something to do with certain interrupts always being routed to CPU0, (oh, and hibernate).
You should be able to enable this behaviour with 'cpu0_hotplug' on the kernel command line.Quoted 5 lines
(Kconfig's
CONFIG_BOOTPARAM_HOTPLUG_CPU0andCONFIG_DEBUG_HOTPLUG_CPU0are also worth a look)On arm64 at least, cpu0 is just like the others, and can be offlined.
Thanks James, for providing all the details.
To add to the issues I spotted with lscpu/lstopo around topology, it ignores the updates to topology sibling masks when CPUs are hotplugged in and out.
We have following in lscpu:
add_summary_n(tb, _("Core(s) per socket:"), cores_per_socket ?: desc->ncores / desc->nsockets);Now when
cores_per_socket= 1, (i.e when we don't have procfs entry), if ncores = (ncores_max-few_cpus_hotplugged_out), core(s) per socket will get computed as less than the actual number.IMO lscpu should be used only when all CPUs are online and it should have a warning when all cores are not online.
Quoted 5 lines
Quoted 5 lines
By the way, did anybody actually see an error with lstopo when there's no "type" attribute for L3? I can't reproduce any issue, we just skip that specific cache entirely, but everything else appears. If you guys want to make that "
no_cache" cache appear, I'll make it a Unified cache unless you tell me what to show :)
Quoted 9 lines
IIUC, Jeffrey Hugo did see error as per his initial message:
"
This fixes the following lscpu issue where only the cache type sysfs file is missing which results in no output providing a poor user experience in the above system configuration.
lscpu: cannot open/sys/devices/system/cpu/cpu0/cache/index3/type: No such file or directory
"
I don't know about lscpu (it's a different project), but lstopo shouldn't have any such problem.
If you see an issue with lstopo, I'd be interesting in getting the tarball generated by hwloc-gather-topology (it dumps useful files from procfs and sysfs so that we may debug offline).
Brice
Quoted 67 lines · Brice Goglin
Le 13/09/2018 à 11:35, Sudeep Holla a écrit :
Quoted 56 lines · James Morse
Quoted 29 lines
Hi Brice,
Quoted 14 lines · Brice Goglin
Le 12/09/2018 à 11:49, Sudeep Holla a écrit :
Quoted 11 lines
Quoted 7 lines
Yes. Without this change, we hit the lscpu error in the commit message, and get zero output about the system. We don't even get information about the caches which are architecturally specified or how many cpus are present. With this change, we get what we expect out of lscpu (and also lstopo) including the cache(s) which are not architecturally specified.
lscpu and lstopo are so broken. They just assume everything on CPU0. If you hotplug them out, you start seeing issues. So reading and file that doesn't exist and then bail out on other essential info though they are present, hmmm ...
Can you elaborate?
Quoted 7 lines
I am not sure cpu0 is supposed to be offlineable on Linux. There's no "online" file in
/sys/devices/system/cpu/cpu0. That's why former lstopo doesn't like CPU0 being hotplugged out. We are actually making that case work for another non-standard corner case. But offlining "cpu0" this is considered "normal", somebody must add that missing "online" sysfs attribute for "cpu0" (change
https://elixir.bootlin.com/linux/latest/source/drivers/base/cpu.c#L375).On x86 you can't normally offline CPU0, its something to do with certain interrupts always being routed to CPU0, (oh, and hibernate).
You should be able to enable this behaviour with 'cpu0_hotplug' on the kernel command line.Quoted 5 lines
(Kconfig's
CONFIG_BOOTPARAM_HOTPLUG_CPU0andCONFIG_DEBUG_HOTPLUG_CPU0are also worth a look)On arm64 at least, cpu0 is just like the others, and can be offlined.
Thanks James, for providing all the details.
To add to the issues I spotted with lscpu/lstopo around topology, it ignores the updates to topology sibling masks when CPUs are hotplugged in and out.
We have following in lscpu:
add_summary_n(tb, _("Core(s) per socket:"), cores_per_socket ?: desc->ncores / desc->nsockets);Now when
cores_per_socket= 1, (i.e when we don't have procfs entry), if ncores = (ncores_max-few_cpus_hotplugged_out), core(s) per socket will get computed as less than the actual number.IMO lscpu should be used only when all CPUs are online and it should have a warning when all cores are not online.
Quoted 5 lines
Quoted 5 lines
By the way, did anybody actually see an error with lstopo when there's no "type" attribute for L3? I can't reproduce any issue, we just skip that specific cache entirely, but everything else appears. If you guys want to make that "
no_cache" cache appear, I'll make it a Unified cache unless you tell me what to show :)Quoted 9 lines
IIUC, Jeffrey Hugo did see error as per his initial message:
"
This fixes the following lscpu issue where only the cache type sysfs file is missing which results in no output providing a poor user experience in the above system configuration.
lscpu: cannot open/sys/devices/system/cpu/cpu0/cache/index3/type: No such file or directory
"
Quoted 6 lines
I don't know about lscpu (it's a different project), but lstopo shouldn't have any such problem.
If you see an issue with lstopo, I'd be interesting in getting the tarball generated by hwloc-gather-topology (it dumps useful files from procfs and sysfs so that we may debug offline).
No error was reported with lstopo, but we don't see the cache as expected. Fixing the type results in the expected lstopo output. This seems consistent with your expectations.
Signature · 4 lines
Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 4 lines · Brice Goglin
Le 13/09/2018 à 11:35, Sudeep Holla a écrit :
Quoted 1 line
Quoted 1 line · Brice Goglin
[...]
Quoted 15 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
By the way, did anybody actually see an error with lstopo when there's no "type" attribute for L3? I can't reproduce any issue, we just skip that specific cache entirely, but everything else appears. If you guys want to make that "
no_cache" cache appear, I'll make it a Unified cache unless you tell me what to show :)Quoted 9 lines
IIUC, Jeffrey Hugo did see error as per his initial message:
"
This fixes the following lscpu issue where only the cache type sysfs file is missing which results in no output providing a poor user experience in the above system configuration.
lscpu: cannot open/sys/devices/system/cpu/cpu0/cache/index3/type: No such file or directory
"
Quoted 3 lines
I don't know about lscpu (it's a different project), but lstopo shouldn't have any such problem.
Ah OK, I have only looked at lscpu source. Sorry for that, I assumed they are based on same/similar code base. Thanks for letting me know they are not.
Quoted 3 lines
If you see an issue with lstopo, I'd be interesting in getting the tarball generated by hwloc-gather-topology (it dumps useful files from procfs and sysfs so that we may debug offline).
Thanks for the information.
Signature · 2 lines
Regards, Sudeep
Quoted 4 lines · Jeremy Linton
Hi Jeffrey,
(+Sudeep)
Thanks for looping me in.
Quoted 12 lines
If you look at the next line of code following this comment its going to update the cache type for fully populated PPTT nodes. Although with the suggested change its only going to activate if someone completely fills out the node and fails to set the valid flag on the cache type.
What I suspect is happening in the reported case is that the nodes in the PPTT table are missing fields we consider to be important. Since that data isn't being filled out anywhere else, so we leave the cache type alone too. This has the effect of hiding sysfs nodes with incomplete information.
Quoted 5 lines
Also, the lack of the DATA/INST fields is based on the assumption that the only nodes which need their type field updated are outside of the CPU core itself so they are pretty much guaranteed to be UNIFIED. Are you hitting this case?
Completely agree with you.
Signature · 2 lines
Regards, Sudeep