cacheinfo: Fix LLC is not exported through sysfs

  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.

This patch tries to fix this by not re-populating the cache leaves if
the LLC is valid.

Fixes: 5944ce092b97 ("arch_topology: Build cacheinfo from primary CPU")
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/base/cacheinfo.c | 8 ++++++++
 1 file changed, 8 insertions(+)
  

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.
> 
> This patch tries to fix this by not re-populating the cache leaves if
> the LLC is valid.
> 
> Fixes: 5944ce092b97 ("arch_topology: Build cacheinfo from primary CPU")
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>   drivers/base/cacheinfo.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index f6573c335f4c..d65f169d36dd 100644
> --- a/drivers/base/cacheinfo.c
> +++ b/drivers/base/cacheinfo.c
> @@ -473,6 +473,13 @@ int detect_cache_attributes(unsigned int cpu)
>   		return ret;
>   
>   populate_leaves:
> +	/*
> +	 * If LLC is valid the cache leaves were already populated so just go to
> +	 * update the cpu map.
> +	 */
> +	if (last_level_cache_is_valid(cpu))
> +		goto update_cpu_map;
> +
>   	/*
>   	 * populate_cache_leaves() may completely setup the cache leaves and
>   	 * shared_cpu_map or it may leave it partially setup.
> @@ -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.)


>   	/*
>   	 * For systems using DT for cache hierarchy, fw_token
>   	 * and shared_cpu_map will be set up here only if they are

Thanks for the fix,
Pierre
  

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.

[...]

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

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
  

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;
+
+       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) {
  

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) {
> 
> .
>
  

On Tue, Mar 28, 2023 at 04:15:17PM +0800, Yicong Yang wrote:
> 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:

Ah OK, in that case I fine with your approach, I assume few things wrong
when we reach the arch code.
  

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.
  

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.