Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 13 May 2022 10:03:30 +0100
From:   Sudeep Holla <sudeep.holla@arm.com>
To:     Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc:     Qing Wang <wangqing@vivo.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Sudeep Holla <sudeep.holla@arm.com>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Morten Rasmussen <morten.rasmussen@arm.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH] arch_topology: Use llc_id instead of package_id
Message-ID: <20220513090330.z25fwthekn4rjkwq@bogus>
References: <20220513083400.343706-1-dietmar.eggemann@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220513083400.343706-1-dietmar.eggemann@arm.com>
Precedence: bulk

On Fri, May 13, 2022 at 10:34:00AM +0200, Dietmar Eggemann wrote:
> package_id should represent socket in DT. Free it for possible socket
> dts parsing and use the so far unused llc_id / llc_sibling cpumask to
> decode 1. level clusters used in pre-DynamIQ big/little systems.
> 
> cpu_coregroup_mask() will return llc_sibling isntead of core_mask in
> this case.
> 
> This will let the cluster_id / cluster_sibling cpumask be available for
> 2. level clusters (e.g. to map L2 sharing in Armv9 A510 complexes).
> 
> The corresponding sched domain CLS is similarly used in ACPI (servers)
> to map e.g. Kunpeng920 L3-tags or Ampere Altra's SCU bounderies.
> 
> Lighlty tested on qemu-system-aarch64 with 1x8 (cluster-x-core) and
> 2x4 cpu-map.
> 
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> ---
> 
> Related to: https://lkml.kernel.org/r/1652361692-13196-1-git-send-email-wangqing@vivo.com
> 
>  drivers/base/arch_topology.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index f73b836047cf..ac1488990cc8 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -490,7 +490,7 @@ static int __init get_cpu_for_node(struct device_node *node)
>  	return cpu;
>  }
>  
> -static int __init parse_core(struct device_node *core, int package_id,
> +static int __init parse_core(struct device_node *core, int llc_id,
>  			     int core_id)
>  {
>  	char name[20];
> @@ -506,7 +506,8 @@ static int __init parse_core(struct device_node *core, int package_id,
>  			leaf = false;
>  			cpu = get_cpu_for_node(t);
>  			if (cpu >= 0) {
> -				cpu_topology[cpu].package_id = package_id;
> +				cpu_topology[cpu].package_id = 0;
> +				cpu_topology[cpu].llc_id = llc_id;
>  				cpu_topology[cpu].core_id = core_id;
>  				cpu_topology[cpu].thread_id = i;
>  			} else if (cpu != -ENODEV) {
> @@ -527,7 +528,8 @@ static int __init parse_core(struct device_node *core, int package_id,
>  			return -EINVAL;
>  		}
>  
> -		cpu_topology[cpu].package_id = package_id;
> +		cpu_topology[cpu].package_id = 0;

While the above looks good and matches with what I am attempting to do
as well ...

> +		cpu_topology[cpu].llc_id = llc_id;

This looks wrong for simple reason that this is derived incorrectly from
the cpu-map while there is no guarantee that it matches the last level
cache ID on the system as we didn't parse the cache topology for this.
So I disagree with this change as it might conflict with the actual and
correct llc_id.

-- 
Regards,
Sudeep