[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 2/4] hw/arm/virt: Consider SMP configuration in CPU topolo
From: |
Igor Mammedov |
Subject: |
Re: [PATCH v6 2/4] hw/arm/virt: Consider SMP configuration in CPU topology |
Date: |
Wed, 20 Apr 2022 16:50:50 +0200 |
On Wed, 20 Apr 2022 22:24:46 +0800
Gavin Shan <gshan@redhat.com> wrote:
> Hi Igor,
>
> On 4/20/22 7:50 PM, Igor Mammedov wrote:
> > On Wed, 20 Apr 2022 18:31:02 +0800
> > Gavin Shan <gshan@redhat.com> wrote:
> >> On 4/20/22 4:32 PM, Igor Mammedov wrote:
> >>> On Mon, 18 Apr 2022 10:09:18 +0800
> >>> Gavin Shan <gshan@redhat.com> wrote:
> >>>
> >>>> Currently, the SMP configuration isn't considered when the CPU
> >>>> topology is populated. In this case, it's impossible to provide
> >>>> the default CPU-to-NUMA mapping or association based on the socket
> >>>> ID of the given CPU.
> >>>>
> >>>> This takes account of SMP configuration when the CPU topology
> >>>> is populated. The die ID for the given CPU isn't assigned since
> >>>> it's not supported on arm/virt machine. Besides, the used SMP
> >>>> configuration in qtest/numa-test/aarch64_numa_cpu() is corrcted
> >>>> to avoid testing failure
> >>>>
> >>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
> >>>> ---
> >>>> hw/arm/virt.c | 15 ++++++++++++++-
> >>>> tests/qtest/numa-test.c | 3 ++-
> >>>> 2 files changed, 16 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >>>> index d2e5ecd234..5443ecae92 100644
> >>>> --- a/hw/arm/virt.c
> >>>> +++ b/hw/arm/virt.c
> >>>> @@ -2505,6 +2505,7 @@ static const CPUArchIdList
> >>>> *virt_possible_cpu_arch_ids(MachineState *ms)
> >>>> int n;
> >>>> unsigned int max_cpus = ms->smp.max_cpus;
> >>>> VirtMachineState *vms = VIRT_MACHINE(ms);
> >>>> + MachineClass *mc = MACHINE_GET_CLASS(vms);
> >>>>
> >>>> if (ms->possible_cpus) {
> >>>> assert(ms->possible_cpus->len == max_cpus);
> >>>> @@ -2518,8 +2519,20 @@ static const CPUArchIdList
> >>>> *virt_possible_cpu_arch_ids(MachineState *ms)
> >>>> ms->possible_cpus->cpus[n].type = ms->cpu_type;
> >>>> ms->possible_cpus->cpus[n].arch_id =
> >>>> virt_cpu_mp_affinity(vms, n);
> >>>> +
> >>>> + assert(!mc->smp_props.dies_supported);
> >>>> + ms->possible_cpus->cpus[n].props.has_socket_id = true;
> >>>> + ms->possible_cpus->cpus[n].props.socket_id =
> >>>> + (n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads));
> >>>> + ms->possible_cpus->cpus[n].props.has_cluster_id = true;
> >>>> + ms->possible_cpus->cpus[n].props.cluster_id =
> >>>> + (n / (ms->smp.cores * ms->smp.threads)) % ms->smp.clusters;
> >>>> + ms->possible_cpus->cpus[n].props.has_core_id = true;
> >>>> + ms->possible_cpus->cpus[n].props.core_id =
> >>>> + (n / ms->smp.threads) % ms->smp.cores;
> >>>> ms->possible_cpus->cpus[n].props.has_thread_id = true;
> >>>> - ms->possible_cpus->cpus[n].props.thread_id = n;
> >>>> + ms->possible_cpus->cpus[n].props.thread_id =
> >>>> + n % ms->smp.threads;
> >>>> }
> >>>> return ms->possible_cpus;
> >>>> }
> >>>> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
> >>>> index 90bf68a5b3..aeda8c774c 100644
> >>>> --- a/tests/qtest/numa-test.c
> >>>> +++ b/tests/qtest/numa-test.c
> >>>> @@ -223,7 +223,8 @@ static void aarch64_numa_cpu(const void *data)
> >>>> QTestState *qts;
> >>>> g_autofree char *cli = NULL;
> >>>>
> >>>> - cli = make_cli(data, "-machine smp.cpus=2 "
> >>>> + cli = make_cli(data, "-machine "
> >>>> +
> >>>> "smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2 "
> >>>
> >>> Is cluster-less config possible?
> >>> (looks like it used to work before and it doesn't after this series)
> >>>
> >>
> >> Nope, it's impossible. This specific test case uses arm/virt machine
> >> where cluster is always supported.mc->smp_props.clusters_supported
> >> has been set to true in hw/arm/virt.c::virt_machine_class_init().
> >>
> >> Exactly, the changes to virt_possible_cpu_arch_ids() included in this
> >> patch breaks
> >> the test. It's why the fix to qtest/numa-test has been squashed to this
> >> patch, to
> >> make it 'bit bisect' friendly as Yanan suggested.
> >
> > so what was error that broke the test?
> > (probably should be mentioned in commit message)
> >
> > (also is it possible to split out the test patch into
> > a separate one and put it before this one)
> >
>
> With amend to the command lines, the following one is used and below error
> is raised from the test. The error is mentioned in the commit log in
> PATCH[v7 2/4].
>
> -machine smp.cpus=2 \
> -numa node,nodeid=0,memdev=ram -numa node,nodeid=1 \
> -numa cpu,node-id=1,thread-id=0 \
> -numa cpu,node-id=0,thread-id=1
>
> qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found
> (reported from hw/core/machine.c::machine_set_cpu_numa_node())
>
> After the changes to virt_possible_cpu_arch_ids() is applied, "thread-id=1"
> isn't valid any more. The CPU topology becomes like below. Note that
> mc->smp_props.prefer_sockets is true on arm/virt machine.
>
> index socket cluster core thread
> --------------------------------------------
> 0 0 0 0 0
> 1 1 0 0 0
>
> With the amended command lines, the topology changes again so
> that "thread-id=1" is valid:
>
> index socket cluster core thread
> --------------------------------------------
> 0 0 0 0 0
> 1 0 0 0 1
>
> It should be ok to split the test/qtest/aarch64_numa_cpu() changes into
> a separate patch and put it before this one. In that case, the specified
> smp.{socket, cluster, core, threads} isn't used by arm/virt machine yet,
> and 'thread-id=2' should be still valid. Lets do this if I need post v8.
> Otherwise, I guess it's also fine to squash the test/qtest/aarch64_numa_cpu()
> changes into PATCH[2/4], as we're doing.
if you need to respin v7. do it as separate patch with proper commit message
and maybe add an extra test that exercises fully specified topo.
>
> >
> >>
> >>
> >>>> "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 "
> >>>> "-numa cpu,node-id=1,thread-id=0 "
> >>>> "-numa cpu,node-id=0,thread-id=1");
>
> Thanks,
> Gavin
>
[PATCH v6 3/4] hw/arm/virt: Fix CPU's default NUMA node ID, Gavin Shan, 2022/04/17
[PATCH v6 4/4] hw/acpi/aml-build: Use existing CPU topology to build PPTT table, Gavin Shan, 2022/04/17