Re: [PATCH] cacheinfo: Fix LLC is not exported through sysfs

[Yicong Yang <yangyicong@huawei.com>]

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.