Messages in this thread Patch in this message | | | Subject | Re: [PATCH] cacheinfo: Fix LLC is not exported through sysfs | From | Yicong Yang <> | Date | Tue, 28 Mar 2023 16:15:17 +0800 |
| |
On 2023/3/27 19:15, Sudeep Holla wrote: > On Mon, Mar 27, 2023 at 02:57:07PM +0800, Yicong Yang wrote: >> 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. >> > > I was thinking of something like below. > > -- > Regards, > Sudeep > > diff --git i/arch/arm64/kernel/cacheinfo.c w/arch/arm64/kernel/cacheinfo.c > index c307f69e9b55..4ef1033fe47e 100644 > --- i/arch/arm64/kernel/cacheinfo.c > +++ w/arch/arm64/kernel/cacheinfo.c > @@ -79,12 +79,16 @@ int init_cache_level(unsigned int cpu) > > int populate_cache_leaves(unsigned int cpu) > { > - unsigned int level, idx; > + unsigned int hw_lvl, level, idx; > enum cache_type type; > struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu); > struct cacheinfo *this_leaf = this_cpu_ci->info_list; > > - for (idx = 0, level = 1; level <= this_cpu_ci->num_levels && > + for (hw_lvl = 0; hw_lvl <= MAX_CACHE_LEVEL; hw_lvl++) > + if (CACHE_TYPE_NOCACHE == get_cache_type(hw_lvl + 1)) > + break; > +
We totally skip the system level caches and leaving their ->level initialized as 0, then we still cannot get the correct infomation by the PPTT side since it uses the ->level to find the cache info: drivers/acpi/pptt.c: cache_setup_acpi_cpu() [...] found_cache = acpi_find_cache_node(table, acpi_cpu_id, this_leaf->type, this_leaf->level, <---- we cannot find it with level 0 &cpu_node);
So I'd prefer the original fixes of mine or by the arch side (if no other archs suffer this issue) what about below for arm64 only:
diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c index c307f69e9b55..4801d0ff4ffb 100644 --- a/arch/arm64/kernel/cacheinfo.c +++ b/arch/arm64/kernel/cacheinfo.c @@ -86,6 +86,13 @@ int populate_cache_leaves(unsigned int cpu)
for (idx = 0, level = 1; level <= this_cpu_ci->num_levels && idx < this_cpu_ci->num_leaves; idx++, level++) { + /* + * This leaf has already been populated, do not reset it since + * this could be a system level cache. + */ + if (this_leaf->type != CACHE_TYPE_NOCACHE) + continue; + type = get_cache_type(level); if (type == CACHE_TYPE_SEPARATE) { ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level); > + for (idx = 0, level = 1; level <= hw_lvl && > idx < this_cpu_ci->num_leaves; idx++, level++) { > type = get_cache_type(level); > if (type == CACHE_TYPE_SEPARATE) { > > . >
| |