Messages in this thread | | | Date | Thu, 23 Mar 2023 18:58:53 +0100 | Subject | Re: [PATCH] cacheinfo: Fix LLC is not exported through sysfs | From | Pierre Gondois <> |
| |
Hello Yicong,
FWIW, I think the patch is correct and I could reproduce the issue.
On 3/23/23 13:25, Yicong Yang wrote: > From: Yicong Yang <yangyicong@hisilicon.com> > > After entering 6.3-rc1 the LLC cacheinfo is not exported on our ACPI > based arm64 server. This is because the LLC cacheinfo is partly reset > when secondary CPUs boot up. On arm64 the primary cpu will allocate > and setup cacheinfo: > init_cpu_topology() > for_each_possible_cpu() > fetch_cache_info() // Allocate cacheinfo and init levels > detect_cache_attributes() > cache_shared_cpu_map_setup() > if (!last_level_cache_is_valid()) // not valid, setup LLC > cache_setup_properties() // setup LLC > > On secondary CPU boot up: > detect_cache_attributes() > populate_cache_leaves() > get_cache_type() // Get cache type from clidr_el1, > // for LLC type=CACHE_TYPE_NOCACHE > cache_shared_cpu_map_setup() > if (!last_level_cache_is_valid()) // Valid and won't go to this branch, > // leave LLC's type=CACHE_TYPE_NOCACHE > > The last_level_cache_is_valid() use cacheinfo->{attributes, fw_token} to > test it's valid or not, but populate_cache_leaves() will only reset > LLC's type, so we won't try to re-setup LLC's type and leave it > CACHE_TYPE_NOCACHE and won't export it through sysfs. > > This patch tries to fix this by not re-populating the cache leaves if > the LLC is valid. > > Fixes: 5944ce092b97 ("arch_topology: Build cacheinfo from primary CPU") > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > --- > drivers/base/cacheinfo.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c > index f6573c335f4c..d65f169d36dd 100644 > --- a/drivers/base/cacheinfo.c > +++ b/drivers/base/cacheinfo.c > @@ -473,6 +473,13 @@ int detect_cache_attributes(unsigned int cpu) > return ret; > > populate_leaves: > + /* > + * If LLC is valid the cache leaves were already populated so just go to > + * update the cpu map. > + */ > + if (last_level_cache_is_valid(cpu)) > + goto update_cpu_map; > + > /* > * populate_cache_leaves() may completely setup the cache leaves and > * shared_cpu_map or it may leave it partially setup. > @@ -481,6 +488,7 @@ int detect_cache_attributes(unsigned int cpu) > if (ret) > goto free_ci; > > +update_cpu_map:
Maybe just a suggestion about the code itself, it should be possible to replace the 'goto' by an 'if' condition. (Similarly, the 'populate_leaves:' label could have been avoided.)
> /* > * For systems using DT for cache hierarchy, fw_token > * and shared_cpu_map will be set up here only if they are
Thanks for the fix, Pierre
| |