[PATCH] arch_topology: Make cluster topology span at least SMT CPUs

[PATCH] arch_topology: Make cluster topology span at least SMT CPUs

[Yicong Yang]

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.

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

Re: [PATCH] arch_topology: Make cluster topology span at least SMT CPUs

[Ionela Voinescu]

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
> 

Re: [PATCH] arch_topology: Make cluster topology span at least SMT CPUs

[Yicong Yang]

On 2022/8/23 16:06, Ionela Voinescu wrote:
> 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"
> 

will add this qualifier. thanks.

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

Not sure I understand it correctly but in the below case I think the LLC may span
the same core with socket?

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

Re: [PATCH] arch_topology: Make cluster topology span at least SMT CPUs

[Ionela Voinescu]

Hi,

On Tuesday 23 Aug 2022 at 21:05:47 (+0800), Yicong Yang wrote:
> On 2022/8/23 16:06, Ionela Voinescu wrote:
> > 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"
> > 
> 
> will add this qualifier. thanks.
> 
> >>
> > 
> > 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.
> > 
> 
> Not sure I understand it correctly but in the below case I think the LLC may span
> the same core with socket?

Ah, I've jumped to conclusions based on the "non-cluster" mention. Does
"non-cluster" just mean that there's no intermediary "cluster" grouping of
CPUs between core and LLC? How does the PPTT table look like, by the
way?

Thanks,
Ionela.

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

Re: [PATCH] arch_topology: Make cluster topology span at least SMT CPUs

[Yicong Yang]

On 2022/8/24 1:53, Ionela Voinescu wrote:
> Hi,
> 
> On Tuesday 23 Aug 2022 at 21:05:47 (+0800), Yicong Yang wrote:
>> On 2022/8/23 16:06, Ionela Voinescu wrote:
>>> 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"
>>>
>>
>> will add this qualifier. thanks.
>>
>>>>
>>>
>>> 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.
>>>
>>
>> Not sure I understand it correctly but in the below case I think the LLC may span
>> the same core with socket?
> 
> Ah, I've jumped to conclusions based on the "non-cluster" mention. Does
> "non-cluster" just mean that there's no intermediary "cluster" grouping of
> CPUs between core and LLC? How does the PPTT table look like, by the
> way?
> 

yes non-cluster means machine has no cluster topology. The PPTT is generated by the
qemu according to the cmdline so I didn't really check it. See the demo provided in
the commit.

'-smp 48,sockets=2,cores=12,threads=2' builds a mahine with 2 sockets, one socket has
12 cores and each core has 2 threads. If we want to have a cluster machine we build it
with '-smp 48,sockets=2,clusters=6,cores=2,threads=2', then we'll have 6 clusters in
one socket and 2 cores in each cluster, 2 threads for each core.

Thanks.

> Thanks,
> Ionela.
> 
>>
>>> 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
>>>>
>>>
>>> .
>>>
> .
> 

Re: [PATCH] arch_topology: Make cluster topology span at least SMT CPUs

[Sudeep Holla]

On Tue, Aug 23, 2022 at 03:30:44PM +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.
> 
> 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>

Reviewed-by: 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);

Sorry for this, for some reason I assumed the get_cpu_mask() will cover
threads too.

-- 
Regards,
Sudeep