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:   Tue, 23 Aug 2022 09:06:52 +0100
From:   Ionela Voinescu <ionela.voinescu@arm.com>
To:     Yicong Yang <yangyicong@huawei.com>
Cc:     sudeep.holla@arm.com, vincent.guittot@linaro.org,
        linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org,
        rafael@kernel.org, 21cnbao@gmail.com, jonathan.cameron@huawei.com,
        linuxarm@huawei.com, prime.zeng@huawei.com,
        yangyicong@hisilicon.com
Subject: Re: [PATCH] arch_topology: Make cluster topology span at least SMT
 CPUs
Message-ID: <YwSKnGii2zdd7Fig@arm.com>
References: <20220823073044.58697-1-yangyicong@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220823073044.58697-1-yangyicong@huawei.com>
Precedence: bulk

Hi Yicong,

On Tuesday 23 Aug 2022 at 15:30:44 (+0800), Yicong Yang wrote:
> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> Currently cpu_clustergroup_mask() will return CPU mask if cluster span
> more or the same CPUs as cpu_coregroup_mask(). This will result topology
> borken on non-Cluster SMT machines.

Might be worth adding here:.. "when building with CONFIG_SCHED_CLUSTER=y"

> 

I thought I had covered this case, but I think I had considered LLC
spanning more than one core (tested on TX2 as well).

So you'd only hit this if LLC and cluster level span the same cores (a
single core in this case, for non-cluster), in the presence of SMT.

Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>

Many thanks,
Ionela.

> Test with:
> qemu-system-aarch64 -enable-kvm -machine virt \
>  -net none \
>  -cpu host \
>  -bios ./QEMU_EFI.fd \
>  -m 2G \
>  -smp 48,sockets=2,cores=12,threads=2 \
>  -kernel $Image \
>  -initrd $Rootfs \
>  -nographic
>  -append "rdinit=init console=ttyAMA0 sched_verbose loglevel=8"
> 
> We'll get below error:
> [    3.084568] BUG: arch topology borken
> [    3.084570]      the SMT domain not a subset of the CLS domain
> 
> Since cluster is a level higher than SMT, fix this by making cluster
> spans at least SMT CPUs.
> 
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: Ionela Voinescu <ionela.voinescu@arm.com>
> Fixes: bfcc4397435d ("arch_topology: Limit span of cpu_clustergroup_mask()")
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/base/arch_topology.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 0424b59b695e..0056a1273275 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -724,7 +724,7 @@ const struct cpumask *cpu_clustergroup_mask(int cpu)
>  	 */
>  	if (cpumask_subset(cpu_coregroup_mask(cpu),
>  			   &cpu_topology[cpu].cluster_sibling))
> -		return get_cpu_mask(cpu);
> +		return cpu_smt_mask(cpu);
>  
>  	return &cpu_topology[cpu].cluster_sibling;
>  }
> -- 
> 2.24.0
>