Messages in this thread | | | Subject | Re: [PATCH] cacheinfo: Fix LLC is not exported through sysfs | From | Yicong Yang <> | Date | Tue, 28 Mar 2023 19:33:24 +0800 |
| |
On 2023/3/28 18:48, 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. >> >>> [...] >>> >>>>> @@ -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 > > The above looks OK but I would prefer to keep it simple as a fix by just not > adding update_cpu_map label, but I don't have strong opinion about that. So > I am fine either way. >
ok got it. I'll respin a v2 patch only involves the fix.
Thanks.
| |