diff mbox series

[v8,2/5] qtest/numa-test: Specify CPU topology in aarch64_numa_cpu()

Message ID 20220425032802.365061-3-gshan@redhat.com (mailing list archive)
State New, archived
Headers show
Series hw/arm/virt: Fix CPU's default NUMA node ID | expand

Commit Message

Gavin Shan April 25, 2022, 3:27 a.m. UTC 
The CPU topology isn't enabled on arm/virt machine yet, but we're
going to do it in next patch. After the CPU topology is enabled by
next patch, "thrad-id=1" becomes invalid because the CPU core is
preferred on arm/virt machine. It means these two CPUs have 0/1
as their core IDs, but their thread IDs are all 0. It will trigger
test failure as the following message indicates:

  [14/21 qemu:qtest+qtest-aarch64 / qtest-aarch64/numa-test  ERROR
  1.48s   killed by signal 6 SIGABRT
  >>> G_TEST_DBUS_DAEMON=/home/gavin/sandbox/qemu.main/tests/dbus-vmstate-daemon.sh \
      QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon         \
      QTEST_QEMU_BINARY=./qemu-system-aarch64                                       \
      QTEST_QEMU_IMG=./qemu-img MALLOC_PERTURB_=83                                  \
      /home/gavin/sandbox/qemu.main/build/tests/qtest/numa-test --tap -k
  ――――――――――――――――――――――――――――――――――――――――――――――
  stderr:
  qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found

This fixes the issue by providing comprehensive SMP configurations
in aarch64_numa_cpu(). The SMP configurations aren't used before
the CPU topology is enabled in next patch.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
---
 tests/qtest/numa-test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Igor Mammedov May 2, 2022, 8:52 a.m. UTC | #1 
On Mon, 25 Apr 2022 11:27:59 +0800
Gavin Shan <gshan@redhat.com> wrote:

> The CPU topology isn't enabled on arm/virt machine yet, but we're
> going to do it in next patch. After the CPU topology is enabled by
> next patch, "thrad-id=1" becomes invalid because the CPU core is
                 ^^^ typo

> preferred on arm/virt machine. It means these two CPUs have 0/1
> as their core IDs, but their thread IDs are all 0. It will trigger
> test failure as the following message indicates:
> 
>   [14/21 qemu:qtest+qtest-aarch64 / qtest-aarch64/numa-test  ERROR
>   1.48s   killed by signal 6 SIGABRT
>   >>> G_TEST_DBUS_DAEMON=/home/gavin/sandbox/qemu.main/tests/dbus-vmstate-daemon.sh \  
>       QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon         \
>       QTEST_QEMU_BINARY=./qemu-system-aarch64                                       \
>       QTEST_QEMU_IMG=./qemu-img MALLOC_PERTURB_=83                                  \
>       /home/gavin/sandbox/qemu.main/build/tests/qtest/numa-test --tap -k
>   ――――――――――――――――――――――――――――――――――――――――――――――
>   stderr:
>   qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found
> 
> This fixes the issue by providing comprehensive SMP configurations
> in aarch64_numa_cpu(). The SMP configurations aren't used before
> the CPU topology is enabled in next patch.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  tests/qtest/numa-test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 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 "
>          "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 "
>          "-numa cpu,node-id=1,thread-id=0 "
                                ^^^^
make it sensible as well, i.e. socket/cluster/cores-ids ...

>          "-numa cpu,node-id=0,thread-id=1");
Gavin Shan May 2, 2022, 10:07 a.m. UTC | #2 
Hi Igor,

On 5/2/22 4:52 PM, Igor Mammedov wrote:
> On Mon, 25 Apr 2022 11:27:59 +0800
> Gavin Shan <gshan@redhat.com> wrote:
> 
>> The CPU topology isn't enabled on arm/virt machine yet, but we're
>> going to do it in next patch. After the CPU topology is enabled by
>> next patch, "thrad-id=1" becomes invalid because the CPU core is
>                   ^^^ typo
> 

hmm, my bad. Lets fix it in next revision.

>> preferred on arm/virt machine. It means these two CPUs have 0/1
>> as their core IDs, but their thread IDs are all 0. It will trigger
>> test failure as the following message indicates:
>>
>>    [14/21 qemu:qtest+qtest-aarch64 / qtest-aarch64/numa-test  ERROR
>>    1.48s   killed by signal 6 SIGABRT
>>    >>> G_TEST_DBUS_DAEMON=/home/gavin/sandbox/qemu.main/tests/dbus-vmstate-daemon.sh \
>>        QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon         \
>>        QTEST_QEMU_BINARY=./qemu-system-aarch64                                       \
>>        QTEST_QEMU_IMG=./qemu-img MALLOC_PERTURB_=83                                  \
>>        /home/gavin/sandbox/qemu.main/build/tests/qtest/numa-test --tap -k
>>    ――――――――――――――――――――――――――――――――――――――――――――――
>>    stderr:
>>    qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found
>>
>> This fixes the issue by providing comprehensive SMP configurations
>> in aarch64_numa_cpu(). The SMP configurations aren't used before
>> the CPU topology is enabled in next patch.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   tests/qtest/numa-test.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> 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 "
>>           "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 "
>>           "-numa cpu,node-id=1,thread-id=0 "
>                                  ^^^^
> make it sensible as well, i.e. socket/cluster/cores-ids ...
> 

Could you help if the following command lines are what you want? I don't
think we can do it. Without PATCH[v8 3/5] applied, {socket,cluster,core}-id
are invalid from arm/virt machine side and we will run into errors.

      -machine                                                           \
      smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2  \
      -numa node,nodeid=0,memdev=ram -numa node,nodeid=1                 \
      -numa cpu,node-id=1,socket-id=0,cluster-id=0,core-id=0,thread-id=0 \
      -numa cpu,node-id=0,socket-id=0,cluster-id=0,core-id=0,thread-id=1
     
      # make -j 10 check
        :
      >>> QTEST_QEMU_IMG=./qemu-img MALLOC_PERTURB_=237                        \
         QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon \
         QTEST_QEMU_BINARY=./qemu-system-aarch64                               \
         G_TEST_DBUS_DAEMON=/home/gavin/sandbox/qemu.main/tests/dbus-vmstate-daemon.sh \
         /home/gavin/sandbox/qemu.main/build/tests/qtest/numa-test --tap -k
     ―――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――
     stderr:
     qemu-system-aarch64: -numa cpu,node-id=1,socket-id=0,cluster-id=0,core-id=0,thread-id=0: core-id is not supported
     Broken pipe

     (The error is reported from hw/core/machine.c::machine_set_cpu_numa_node())

By the way, could you also help to check if the following patches look
good to you? I hope to make next revision eligible for merge :)

     [PATCH v8 1/5] qapi/machine.json: Add cluster-id
     [PATCH v8 3/5] hw/arm/virt: Consider SMP configuration in CPU topology

>>           "-numa cpu,node-id=0,thread-id=1");
> 

Thanks,
Gavin
Igor Mammedov May 3, 2022, 8:54 a.m. UTC | #3 
On Mon, 2 May 2022 18:07:00 +0800
Gavin Shan <gshan@redhat.com> wrote:

> Hi Igor,
> 
> On 5/2/22 4:52 PM, Igor Mammedov wrote:
> > On Mon, 25 Apr 2022 11:27:59 +0800
> > Gavin Shan <gshan@redhat.com> wrote:
> >   
> >> The CPU topology isn't enabled on arm/virt machine yet, but we're
> >> going to do it in next patch. After the CPU topology is enabled by
> >> next patch, "thrad-id=1" becomes invalid because the CPU core is  
> >                   ^^^ typo
> >   
> 
> hmm, my bad. Lets fix it in next revision.
> 
> >> preferred on arm/virt machine. It means these two CPUs have 0/1
> >> as their core IDs, but their thread IDs are all 0. It will trigger
> >> test failure as the following message indicates:
> >>
> >>    [14/21 qemu:qtest+qtest-aarch64 / qtest-aarch64/numa-test  ERROR
> >>    1.48s   killed by signal 6 SIGABRT  
> >>    >>> G_TEST_DBUS_DAEMON=/home/gavin/sandbox/qemu.main/tests/dbus-vmstate-daemon.sh \  
> >>        QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon         \
> >>        QTEST_QEMU_BINARY=./qemu-system-aarch64                                       \
> >>        QTEST_QEMU_IMG=./qemu-img MALLOC_PERTURB_=83                                  \
> >>        /home/gavin/sandbox/qemu.main/build/tests/qtest/numa-test --tap -k
> >>    ――――――――――――――――――――――――――――――――――――――――――――――
> >>    stderr:
> >>    qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found
> >>
> >> This fixes the issue by providing comprehensive SMP configurations
> >> in aarch64_numa_cpu(). The SMP configurations aren't used before
> >> the CPU topology is enabled in next patch.
> >>
> >> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
> >> ---
> >>   tests/qtest/numa-test.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> 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 "
> >>           "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 "
> >>           "-numa cpu,node-id=1,thread-id=0 "  
> >                                  ^^^^
> > make it sensible as well, i.e. socket/cluster/cores-ids ...
> >   
> 
> Could you help if the following command lines are what you want? I don't
> think we can do it. Without PATCH[v8 3/5] applied, {socket,cluster,core}-id
> are invalid from arm/virt machine side and we will run into errors.
you are right, you can only fix -numa after 3/5

btw: 
splitting threads between several numa nodes here probably is unreal
configuration. Should be fixed in follow up patches.

>       -machine                                                           \
>       smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2  \
>       -numa node,nodeid=0,memdev=ram -numa node,nodeid=1                 \
>       -numa cpu,node-id=1,socket-id=0,cluster-id=0,core-id=0,thread-id=0 \
>       -numa cpu,node-id=0,socket-id=0,cluster-id=0,core-id=0,thread-id=1
>      
>       # make -j 10 check
>         :
>       >>> QTEST_QEMU_IMG=./qemu-img MALLOC_PERTURB_=237                        \  
>          QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon \
>          QTEST_QEMU_BINARY=./qemu-system-aarch64                               \
>          G_TEST_DBUS_DAEMON=/home/gavin/sandbox/qemu.main/tests/dbus-vmstate-daemon.sh \
>          /home/gavin/sandbox/qemu.main/build/tests/qtest/numa-test --tap -k
>      ―――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――
>      stderr:
>      qemu-system-aarch64: -numa cpu,node-id=1,socket-id=0,cluster-id=0,core-id=0,thread-id=0: core-id is not supported
>      Broken pipe
> 
>      (The error is reported from hw/core/machine.c::machine_set_cpu_numa_node())
> 
> By the way, could you also help to check if the following patches look
> good to you? I hope to make next revision eligible for merge :)
> 
>      [PATCH v8 1/5] qapi/machine.json: Add cluster-id
>      [PATCH v8 3/5] hw/arm/virt: Consider SMP configuration in CPU topology
> 
> >>           "-numa cpu,node-id=0,thread-id=1");  
> >   
> 
> Thanks,
> Gavin
>
Gavin Shan May 3, 2022, 1:47 p.m. UTC | #4 
Hi Igor,

On 5/3/22 4:54 PM, Igor Mammedov wrote:
> On Mon, 2 May 2022 18:07:00 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>> On 5/2/22 4:52 PM, Igor Mammedov wrote:
>>> On Mon, 25 Apr 2022 11:27:59 +0800
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>    
>>>> The CPU topology isn't enabled on arm/virt machine yet, but we're
>>>> going to do it in next patch. After the CPU topology is enabled by
>>>> next patch, "thrad-id=1" becomes invalid because the CPU core is
>>>                    ^^^ typo
>>>    
>>
>> hmm, my bad. Lets fix it in next revision.
>>
>>>> preferred on arm/virt machine. It means these two CPUs have 0/1
>>>> as their core IDs, but their thread IDs are all 0. It will trigger
>>>> test failure as the following message indicates:
>>>>
>>>>     [14/21 qemu:qtest+qtest-aarch64 / qtest-aarch64/numa-test  ERROR
>>>>     1.48s   killed by signal 6 SIGABRT
>>>>     >>> G_TEST_DBUS_DAEMON=/home/gavin/sandbox/qemu.main/tests/dbus-vmstate-daemon.sh \
>>>>         QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon         \
>>>>         QTEST_QEMU_BINARY=./qemu-system-aarch64                                       \
>>>>         QTEST_QEMU_IMG=./qemu-img MALLOC_PERTURB_=83                                  \
>>>>         /home/gavin/sandbox/qemu.main/build/tests/qtest/numa-test --tap -k
>>>>     ――――――――――――――――――――――――――――――――――――――――――――――
>>>>     stderr:
>>>>     qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found
>>>>
>>>> This fixes the issue by providing comprehensive SMP configurations
>>>> in aarch64_numa_cpu(). The SMP configurations aren't used before
>>>> the CPU topology is enabled in next patch.
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>>>> ---
>>>>    tests/qtest/numa-test.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> 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 "
>>>>            "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 "
>>>>            "-numa cpu,node-id=1,thread-id=0 "
>>>                                   ^^^^
>>> make it sensible as well, i.e. socket/cluster/cores-ids ...
>>>    
>>
>> Could you help if the following command lines are what you want? I don't
>> think we can do it. Without PATCH[v8 3/5] applied, {socket,cluster,core}-id
>> are invalid from arm/virt machine side and we will run into errors.
> you are right, you can only fix -numa after 3/5
> 
> btw:
> splitting threads between several numa nodes here probably is unreal
> configuration. Should be fixed in follow up patches.
> 

Lets fix it in PATCH[v9 4/6] with the following command lines. Besides,
socket/cluster/core/thread IDs should be checked when the NUMA IDs are
verified. It helps to check if the CPU topology is properly populated
at least.

v9 should be sent shortly after doing some tests. Please take a look.

     -machine                                                           \
     smp.cpus=2,smp.sockets=2,smp.clusters=1,smp.cores=1,smp.threads=1  \
     -numa node,nodeid=0,memdev=ram -numa node,nodeid=1                 \
     -numa cpu,node-id=0,socket-id=1,cluster-id=0,core-id=0,thread-id=0 \
     -numa cpu,node-id=1,socket-id=0,cluster-id=0,core-id=0,thread-id=0

[...]

Thanks,
Gavin
diff mbox series

Patch

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 "
         "-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");