[v2] cacheinfo: Fix LLC is not exported through sysfs

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

Posted by Yicong Yang 1 year ago

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>
---
Change since v1:
- Get rid of the goto label, per Pierre
Link: https://lore.kernel.org/all/20230323122528.16691-1-yangyicong@huawei.com/

 drivers/base/cacheinfo.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index f6573c335f4c..f3903d002819 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -474,12 +474,18 @@ int detect_cache_attributes(unsigned int cpu)
 
 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
-- 
2.24.0

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

Posted by Sudeep Holla 1 year ago

On Tue, Mar 28, 2023 at 07:49:15PM +0800, Yicong Yang wrote:
> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> After entering 6.3-rc1 the LLC cacheinfo is not exported on our ACPI

I would have preferred the commit causing the issue as you know and have
it in the fixes tag, but info is there so I don't think it is worth another
version churn. Sorry my bad for missing it earlier.

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

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

Hi Greg,

Can you pick this up in your next round of fixes for v6.3 please ?

--
Regards,
Sudeep

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

Posted by Greg KH 1 year ago

On Tue, Mar 28, 2023 at 01:47:21PM +0100, Sudeep Holla wrote:
> On Tue, Mar 28, 2023 at 07:49:15PM +0800, Yicong Yang wrote:
> > From: Yicong Yang <yangyicong@hisilicon.com>
> > 
> > After entering 6.3-rc1 the LLC cacheinfo is not exported on our ACPI
> 
> I would have preferred the commit causing the issue as you know and have
> it in the fixes tag, but info is there so I don't think it is worth another
> version churn. Sorry my bad for missing it earlier.
> 
> > 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.
> >
> 
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> 
> Hi Greg,
> 
> Can you pick this up in your next round of fixes for v6.3 please ?

Will do, thanks!

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

Posted by Pierre Gondois 1 year ago

Hello Yicong,
Thanks for the new version,

Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>

On 3/28/23 13:49, 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>
> ---
> Change since v1:
> - Get rid of the goto label, per Pierre
> Link: https://lore.kernel.org/all/20230323122528.16691-1-yangyicong@huawei.com/
> 
>   drivers/base/cacheinfo.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index f6573c335f4c..f3903d002819 100644
> --- a/drivers/base/cacheinfo.c
> +++ b/drivers/base/cacheinfo.c
> @@ -474,12 +474,18 @@ int detect_cache_attributes(unsigned int cpu)
>   
>   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