Re: [Qemu-devel] [RFC 4/4] numa: check threads of the same core are on the same node

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4988 bytes --]

On Tue, Feb 12, 2019 at 10:48:27PM +0100, Laurent Vivier wrote:
> A core cannot be split between two nodes.
> To check if a thread of the same core has already been assigned to a node,
> this patch reverses the numa topology checking order and exits if the
> topology is not valid.

I'm not entirely sure if this makes sense to enforce generically.

It's certainly true for PAPR - we have no way to represent threads
with different NUMA nodes to the guest.

It probably makes sense for everything - the whole point of threading
is to take better advantage of latencies accessing memory, so it seems
implausible that the threads would have different paths to memory.

But... there are some pretty weird setups out there, so I'm not sure
it's a good idea to enforce a restriction generically that's not
actually inherent in the structure of the problem.

> 
> Update test/numa-test accordingly.
> 
> Fixes: 722387e78daf ("spapr: get numa node mapping from possible_cpus instead of numa_get_node_for_cpu()")
> Cc: imammedo@redhat.com
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  hw/core/machine.c | 27 ++++++++++++++++++++++++---
>  tests/numa-test.c |  4 ++--
>  2 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index a2c29692b55e..c0a556b0dce7 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -602,6 +602,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>      bool match = false;
>      int i;
> +    const CpuInstanceProperties *previous_props = NULL;
>  
>      if (!mc->possible_cpu_arch_ids) {
>          error_setg(errp, "mapping of CPUs to NUMA node is not supported");
> @@ -634,18 +635,38 @@ void machine_set_cpu_numa_node(MachineState *machine,
>          }
>  
>          /* skip slots with explicit mismatch */
> -        if (props->has_thread_id && props->thread_id != slot->props.thread_id) {
> +        if (props->has_socket_id && props->socket_id != slot->props.socket_id) {
>                  continue;
>          }
>  
> -        if (props->has_core_id && props->core_id != slot->props.core_id) {
> +        if (props->has_core_id) {
> +            if (props->core_id != slot->props.core_id) {
>                  continue;
> +            }
> +            if (slot->props.has_node_id) {
> +                /* we have a node where our core is already assigned */
> +                previous_props = &slot->props;
> +            }
>          }
>  
> -        if (props->has_socket_id && props->socket_id != slot->props.socket_id) {
> +        if (props->has_thread_id && props->thread_id != slot->props.thread_id) {
>                  continue;
>          }
>  
> +        /* check current thread matches node of the thread of the same core */
> +        if (previous_props && previous_props->has_node_id &&
> +            previous_props->node_id != props->node_id) {
> +            char *cpu_str = cpu_props_to_string(props);
> +            char *node_str = cpu_props_to_string(previous_props);
> +            error_setg(errp,  "Invalid node-id=%"PRIu64" of [%s]: core-id "
> +                              "[%s] is already assigned to node-id %"PRIu64,
> +                              props->node_id, cpu_str,
> +                              node_str, previous_props->node_id);
> +            g_free(cpu_str);
> +            g_free(node_str);
> +            return;
> +        }
> +
>          /* reject assignment if slot is already assigned, for compatibility
>           * of legacy cpu_index mapping with SPAPR core based mapping do not
>           * error out if cpu thread and matched core have the same node-id */
> diff --git a/tests/numa-test.c b/tests/numa-test.c
> index 5280573fc992..a7c3c5b4dee8 100644
> --- a/tests/numa-test.c
> +++ b/tests/numa-test.c
> @@ -112,7 +112,7 @@ static void pc_numa_cpu(const void *data)
>          "-numa cpu,node-id=1,socket-id=0 "
>          "-numa cpu,node-id=0,socket-id=1,core-id=0 "
>          "-numa cpu,node-id=0,socket-id=1,core-id=1,thread-id=0 "
> -        "-numa cpu,node-id=1,socket-id=1,core-id=1,thread-id=1");
> +        "-numa cpu,node-id=0,socket-id=1,core-id=1,thread-id=1");
>      qtest_start(cli);
>      cpus = get_cpus(&resp);
>      g_assert(cpus);
> @@ -141,7 +141,7 @@ static void pc_numa_cpu(const void *data)
>          } else if (socket == 1 && core == 1 && thread == 0) {
>              g_assert_cmpint(node, ==, 0);
>          } else if (socket == 1 && core == 1 && thread == 1) {
> -            g_assert_cmpint(node, ==, 1);
> +            g_assert_cmpint(node, ==, 0);
>          } else {
>              g_assert(false);
>          }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]