* [PATCH] arch_topology: Make cluster topology span at least SMT CPUs
@ 2022-08-23 7:30 Yicong Yang
2022-08-23 8:06 ` Ionela Voinescu
2022-08-23 9:22 ` Sudeep Holla
0 siblings, 2 replies; 6+ messages in thread
From: Yicong Yang @ 2022-08-23 7:30 UTC (permalink / raw)
To: sudeep.holla, vincent.guittot, ionela.voinescu, linux-kernel
Cc: gregkh, rafael, 21cnbao, jonathan.cameron, linuxarm, prime.zeng,
yangyicong
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] arch_topology: Make cluster topology span at least SMT CPUs
2022-08-23 7:30 [PATCH] arch_topology: Make cluster topology span at least SMT CPUs Yicong Yang
@ 2022-08-23 8:06 ` Ionela Voinescu
2022-08-23 13:05 ` Yicong Yang
2022-08-23 9:22 ` Sudeep Holla
1 sibling, 1 reply; 6+ messages in thread
From: Ionela Voinescu @ 2022-08-23 8:06 UTC (permalink / raw)
To: Yicong Yang
Cc: sudeep.holla, vincent.guittot, linux-kernel, gregkh, rafael,
21cnbao, jonathan.cameron, linuxarm, prime.zeng, yangyicong
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arch_topology: Make cluster topology span at least SMT CPUs
2022-08-23 7:30 [PATCH] arch_topology: Make cluster topology span at least SMT CPUs Yicong Yang
2022-08-23 8:06 ` Ionela Voinescu
@ 2022-08-23 9:22 ` Sudeep Holla
1 sibling, 0 replies; 6+ messages in thread
From: Sudeep Holla @ 2022-08-23 9:22 UTC (permalink / raw)
To: Yicong Yang
Cc: vincent.guittot, ionela.voinescu, linux-kernel, gregkh, rafael,
21cnbao, jonathan.cameron, linuxarm, prime.zeng, yangyicong
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arch_topology: Make cluster topology span at least SMT CPUs
2022-08-23 8:06 ` Ionela Voinescu
@ 2022-08-23 13:05 ` Yicong Yang
2022-08-23 17:53 ` Ionela Voinescu
0 siblings, 1 reply; 6+ messages in thread
From: Yicong Yang @ 2022-08-23 13:05 UTC (permalink / raw)
To: Ionela Voinescu
Cc: yangyicong, sudeep.holla, vincent.guittot, linux-kernel, gregkh,
rafael, 21cnbao, jonathan.cameron, linuxarm, prime.zeng
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
>>
>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arch_topology: Make cluster topology span at least SMT CPUs
2022-08-23 13:05 ` Yicong Yang
@ 2022-08-23 17:53 ` Ionela Voinescu
2022-08-24 1:31 ` Yicong Yang
0 siblings, 1 reply; 6+ messages in thread
From: Ionela Voinescu @ 2022-08-23 17:53 UTC (permalink / raw)
To: Yicong Yang
Cc: yangyicong, sudeep.holla, vincent.guittot, linux-kernel, gregkh,
rafael, 21cnbao, jonathan.cameron, linuxarm, prime.zeng
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
> >>
> >
> > .
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arch_topology: Make cluster topology span at least SMT CPUs
2022-08-23 17:53 ` Ionela Voinescu
@ 2022-08-24 1:31 ` Yicong Yang
0 siblings, 0 replies; 6+ messages in thread
From: Yicong Yang @ 2022-08-24 1:31 UTC (permalink / raw)
To: Ionela Voinescu
Cc: yangyicong, sudeep.holla, vincent.guittot, linux-kernel, gregkh,
rafael, 21cnbao, jonathan.cameron, linuxarm, prime.zeng
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
>>>>
>>>
>>> .
>>>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-08-24 1:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-23 7:30 [PATCH] arch_topology: Make cluster topology span at least SMT CPUs Yicong Yang
2022-08-23 8:06 ` Ionela Voinescu
2022-08-23 13:05 ` Yicong Yang
2022-08-23 17:53 ` Ionela Voinescu
2022-08-24 1:31 ` Yicong Yang
2022-08-23 9:22 ` Sudeep Holla
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox