Messages in this thread Patch in this message | | | Subject | Re: [PATCH] cacheinfo: Fix LLC is not exported through sysfs | From | Yicong Yang <> | Date | Mon, 27 Mar 2023 14:57:07 +0800 |
| |
Hi Pierre and Sudeep,
On 2023/3/24 19:35, Sudeep Holla wrote: > On Thu, Mar 23, 2023 at 06:58:53PM +0100, Pierre Gondois wrote: >> 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. >>> > > IIUC this is for the case where arch register doesn't report the system level > cache. I wonder if it makes sense to fix the arch callback to deal with that > instead of here. I am fine either way, just checking as ideally it is > something populate_cache_leaves() is messing up. >
yes it's right, the LLC information is not provided by the CPU register and can only be retrieved from PPTT on my machine. Maybe fix the issue first, I don't know how to make arch callback handle this since arch_topology is also used other than arm64 which I'm not familiar with.
> [...] > >>> @@ -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.) >> > > Agreed, I prefer that as well. >
ok, will modify as below with a little refactor to get rid of the 'populate_leaves:' label as suggested.
Thanks, Yicong
diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c index f6573c335f4c..e34e6b77e81a 100644 --- a/drivers/base/cacheinfo.c +++ b/drivers/base/cacheinfo.c @@ -462,24 +462,28 @@ int detect_cache_attributes(unsigned int cpu) * as it will happen only once (the cacheinfo memory is never freed). * Just populate the cacheinfo. */ - if (per_cpu_cacheinfo(cpu)) - goto populate_leaves; - - if (init_cache_level(cpu) || !cache_leaves(cpu)) - return -ENOENT; + if (!per_cpu_cacheinfo(cpu)) { + if (init_cache_level(cpu) || !cache_leaves(cpu)) + return -ENOENT;
- ret = allocate_cache_info(cpu); - if (ret) - return ret; + ret = allocate_cache_info(cpu); + if (ret) + return ret; + }
-populate_leaves: /* - * populate_cache_leaves() may completely setup the cache leaves and - * shared_cpu_map or it may leave it partially setup. + * If LLC is valid the cache leaves were already populated so just go to + * update the cpu map. */ - ret = populate_cache_leaves(cpu); - if (ret) - goto free_ci; + if (!last_level_cache_is_valid(cpu)) { + /* + * populate_cache_leaves() may completely setup the cache leaves and + * shared_cpu_map or it may leave it partially setup. + */ + ret = populate_cache_leaves(cpu); + if (ret) + goto free_ci; + }
/* * For systems using DT for cache hierarchy, fw_token
| |