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

[Sudeep Holla]

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@xxxxxxxxxxxxx>
> >>>
> >>> 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.

-- 
Regards,
Sudeep