[PATCH 0/2] numa: Parse initiator= attribute before cpus= attribute

[PATCH 0/2] numa: Parse initiator= attribute before cpus= attribute

[Michal Privoznik]

See 2/2 for explanation. The first patch is just cosmetics.

Michal Privoznik (2):
  numa: Report expected initiator
  numa: Parse initiator= attribute before cpus= attribute

 hw/core/machine.c |  3 ++-
 hw/core/numa.c    | 45 +++++++++++++++++++++++----------------------
 2 files changed, 25 insertions(+), 23 deletions(-)

-- 
2.31.1

[PATCH 1/2] numa: Report expected initiator

[Michal Privoznik]

When setting up NUMA with HMAT enabled there's a check performed
in machine_set_cpu_numa_node() that reports an error when a NUMA
node has a CPU but the node's initiator is not itself. The error
message reported contains only the expected value and not the
actual value (which is different because an error is being
reported). Report both values in the error message.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 hw/core/machine.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 57c18f909a..6f59fb0b7f 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -728,7 +728,8 @@ void machine_set_cpu_numa_node(MachineState *machine,
             if ((numa_info[props->node_id].initiator < MAX_NODES) &&
                 (props->node_id != numa_info[props->node_id].initiator)) {
                 error_setg(errp, "The initiator of CPU NUMA node %" PRId64
-                        " should be itself", props->node_id);
+                           " should be itself (got %" PRIu16 ")",
+                           props->node_id, numa_info[props->node_id].initiator);
                 return;
             }
             numa_info[props->node_id].has_cpu = true;
-- 
2.31.1

[PATCH 2/2] numa: Parse initiator= attribute before cpus= attribute

[Michal Privoznik]

When parsing cpus= attribute of -numa object couple of checks
is performed, such as correct initiator setting (see the if()
statement at the end of for() loop in
machine_set_cpu_numa_node()).

However, with the current code cpus= attribute is parsed before
initiator= attribute and thus the check may fail even though it
is not obvious why. But since parsing the initiator= attribute
does not depend on the cpus= attribute we can swap the order of
the two.

It's fairly easy to reproduce with the following command line
(snippet of an actual cmd line):

  -smp 4,sockets=4,cores=1,threads=1 \
  -object '{"qom-type":"memory-backend-ram","id":"ram-node0","size":2147483648}' \
  -numa node,nodeid=0,cpus=0-1,initiator=0,memdev=ram-node0 \
  -object '{"qom-type":"memory-backend-ram","id":"ram-node1","size":2147483648}' \
  -numa node,nodeid=1,cpus=2-3,initiator=1,memdev=ram-node1 \
  -numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-latency,latency=5 \
  -numa hmat-lb,initiator=0,target=0,hierarchy=first-level,data-type=access-latency,latency=10 \
  -numa hmat-lb,initiator=1,target=1,hierarchy=memory,data-type=access-latency,latency=5 \
  -numa hmat-lb,initiator=1,target=1,hierarchy=first-level,data-type=access-latency,latency=10 \
  -numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-bandwidth,bandwidth=204800K \
  -numa hmat-lb,initiator=0,target=0,hierarchy=first-level,data-type=access-bandwidth,bandwidth=208896K \
  -numa hmat-lb,initiator=1,target=1,hierarchy=memory,data-type=access-bandwidth,bandwidth=204800K \
  -numa hmat-lb,initiator=1,target=1,hierarchy=first-level,data-type=access-bandwidth,bandwidth=208896K \
  -numa hmat-cache,node-id=0,size=10K,level=1,associativity=direct,policy=write-back,line=8 \
  -numa hmat-cache,node-id=1,size=10K,level=1,associativity=direct,policy=write-back,line=8 \

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 hw/core/numa.c | 45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/hw/core/numa.c b/hw/core/numa.c
index 1058d3697b..510d096a88 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -88,6 +88,29 @@ static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
         return;
     }
 
+    /*
+     * If not set the initiator, set it to MAX_NODES. And if
+     * HMAT is enabled and this node has no cpus, QEMU will raise error.
+     */
+    numa_info[nodenr].initiator = MAX_NODES;
+    if (node->has_initiator) {
+        if (!ms->numa_state->hmat_enabled) {
+            error_setg(errp, "ACPI Heterogeneous Memory Attribute Table "
+                       "(HMAT) is disabled, enable it with -machine hmat=on "
+                       "before using any of hmat specific options");
+            return;
+        }
+
+        if (node->initiator >= MAX_NODES) {
+            error_report("The initiator id %" PRIu16 " expects an integer "
+                         "between 0 and %d", node->initiator,
+                         MAX_NODES - 1);
+            return;
+        }
+
+        numa_info[nodenr].initiator = node->initiator;
+    }
+
     for (cpus = node->cpus; cpus; cpus = cpus->next) {
         CpuInstanceProperties props;
         if (cpus->value >= max_cpus) {
@@ -142,28 +165,6 @@ static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
         numa_info[nodenr].node_memdev = MEMORY_BACKEND(o);
     }
 
-    /*
-     * If not set the initiator, set it to MAX_NODES. And if
-     * HMAT is enabled and this node has no cpus, QEMU will raise error.
-     */
-    numa_info[nodenr].initiator = MAX_NODES;
-    if (node->has_initiator) {
-        if (!ms->numa_state->hmat_enabled) {
-            error_setg(errp, "ACPI Heterogeneous Memory Attribute Table "
-                       "(HMAT) is disabled, enable it with -machine hmat=on "
-                       "before using any of hmat specific options");
-            return;
-        }
-
-        if (node->initiator >= MAX_NODES) {
-            error_report("The initiator id %" PRIu16 " expects an integer "
-                         "between 0 and %d", node->initiator,
-                         MAX_NODES - 1);
-            return;
-        }
-
-        numa_info[nodenr].initiator = node->initiator;
-    }
     numa_info[nodenr].present = true;
     max_numa_nodeid = MAX(max_numa_nodeid, nodenr + 1);
     ms->numa_state->num_nodes++;
-- 
2.31.1

Re: [PATCH 0/2] numa: Parse initiator= attribute before cpus= attribute

[Connor Kuehl]

On Wed Jul 7, 2021 at 6:40 AM PDT, Michal Privoznik wrote:
> See 2/2 for explanation. The first patch is just cosmetics.
>
> Michal Privoznik (2):
> numa: Report expected initiator
> numa: Parse initiator= attribute before cpus= attribute
>
> hw/core/machine.c | 3 ++-
> hw/core/numa.c | 45 +++++++++++++++++++++++----------------------
> 2 files changed, 25 insertions(+), 23 deletions(-)
>
> --
> 2.31.1

For the series:

Reviewed-by: Connor Kuehl <ckuehl@redhat.com>

Re: [PATCH 2/2] numa: Parse initiator= attribute before cpus= attribute

[Igor Mammedov]

On Wed,  7 Jul 2021 15:40:30 +0200
Michal Privoznik <mprivozn@redhat.com> wrote:

> When parsing cpus= attribute of -numa object couple of checks
> is performed, such as correct initiator setting (see the if()
> statement at the end of for() loop in
> machine_set_cpu_numa_node()).
> 
> However, with the current code cpus= attribute is parsed before
> initiator= attribute and thus the check may fail even though it
> is not obvious why. But since parsing the initiator= attribute
> does not depend on the cpus= attribute we can swap the order of
> the two.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

FYI:
I'm planning to deprecate '-numa node,cpus=' in favor of '-numa cpu'.


> It's fairly easy to reproduce with the following command line
> (snippet of an actual cmd line):
> 
>   -smp 4,sockets=4,cores=1,threads=1 \
>   -object '{"qom-type":"memory-backend-ram","id":"ram-node0","size":2147483648}' \
>   -numa node,nodeid=0,cpus=0-1,initiator=0,memdev=ram-node0 \
>   -object '{"qom-type":"memory-backend-ram","id":"ram-node1","size":2147483648}' \
>   -numa node,nodeid=1,cpus=2-3,initiator=1,memdev=ram-node1 \
>   -numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-latency,latency=5 \
>   -numa hmat-lb,initiator=0,target=0,hierarchy=first-level,data-type=access-latency,latency=10 \
>   -numa hmat-lb,initiator=1,target=1,hierarchy=memory,data-type=access-latency,latency=5 \
>   -numa hmat-lb,initiator=1,target=1,hierarchy=first-level,data-type=access-latency,latency=10 \
>   -numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-bandwidth,bandwidth=204800K \
>   -numa hmat-lb,initiator=0,target=0,hierarchy=first-level,data-type=access-bandwidth,bandwidth=208896K \
>   -numa hmat-lb,initiator=1,target=1,hierarchy=memory,data-type=access-bandwidth,bandwidth=204800K \
>   -numa hmat-lb,initiator=1,target=1,hierarchy=first-level,data-type=access-bandwidth,bandwidth=208896K \
>   -numa hmat-cache,node-id=0,size=10K,level=1,associativity=direct,policy=write-back,line=8 \
>   -numa hmat-cache,node-id=1,size=10K,level=1,associativity=direct,policy=write-back,line=8 \
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  hw/core/numa.c | 45 +++++++++++++++++++++++----------------------
>  1 file changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/core/numa.c b/hw/core/numa.c
> index 1058d3697b..510d096a88 100644
> --- a/hw/core/numa.c
> +++ b/hw/core/numa.c
> @@ -88,6 +88,29 @@ static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
>          return;
>      }
>  
> +    /*
> +     * If not set the initiator, set it to MAX_NODES. And if
> +     * HMAT is enabled and this node has no cpus, QEMU will raise error.
> +     */
> +    numa_info[nodenr].initiator = MAX_NODES;
> +    if (node->has_initiator) {
> +        if (!ms->numa_state->hmat_enabled) {
> +            error_setg(errp, "ACPI Heterogeneous Memory Attribute Table "
> +                       "(HMAT) is disabled, enable it with -machine hmat=on "
> +                       "before using any of hmat specific options");
> +            return;
> +        }
> +
> +        if (node->initiator >= MAX_NODES) {
> +            error_report("The initiator id %" PRIu16 " expects an integer "
> +                         "between 0 and %d", node->initiator,
> +                         MAX_NODES - 1);
> +            return;
> +        }
> +
> +        numa_info[nodenr].initiator = node->initiator;
> +    }
> +
>      for (cpus = node->cpus; cpus; cpus = cpus->next) {
>          CpuInstanceProperties props;
>          if (cpus->value >= max_cpus) {
> @@ -142,28 +165,6 @@ static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
>          numa_info[nodenr].node_memdev = MEMORY_BACKEND(o);
>      }
>  
> -    /*
> -     * If not set the initiator, set it to MAX_NODES. And if
> -     * HMAT is enabled and this node has no cpus, QEMU will raise error.
> -     */
> -    numa_info[nodenr].initiator = MAX_NODES;
> -    if (node->has_initiator) {
> -        if (!ms->numa_state->hmat_enabled) {
> -            error_setg(errp, "ACPI Heterogeneous Memory Attribute Table "
> -                       "(HMAT) is disabled, enable it with -machine hmat=on "
> -                       "before using any of hmat specific options");
> -            return;
> -        }
> -
> -        if (node->initiator >= MAX_NODES) {
> -            error_report("The initiator id %" PRIu16 " expects an integer "
> -                         "between 0 and %d", node->initiator,
> -                         MAX_NODES - 1);
> -            return;
> -        }
> -
> -        numa_info[nodenr].initiator = node->initiator;
> -    }
>      numa_info[nodenr].present = true;
>      max_numa_nodeid = MAX(max_numa_nodeid, nodenr + 1);
>      ms->numa_state->num_nodes++;

Re: [PATCH 1/2] numa: Report expected initiator

[Igor Mammedov]

On Wed,  7 Jul 2021 15:40:29 +0200
Michal Privoznik <mprivozn@redhat.com> wrote:

> When setting up NUMA with HMAT enabled there's a check performed
> in machine_set_cpu_numa_node() that reports an error when a NUMA
> node has a CPU but the node's initiator is not itself. The error
> message reported contains only the expected value and not the
> actual value (which is different because an error is being
> reported). Report both values in the error message.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/core/machine.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 57c18f909a..6f59fb0b7f 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -728,7 +728,8 @@ void machine_set_cpu_numa_node(MachineState *machine,
>              if ((numa_info[props->node_id].initiator < MAX_NODES) &&
>                  (props->node_id != numa_info[props->node_id].initiator)) {
>                  error_setg(errp, "The initiator of CPU NUMA node %" PRId64
> -                        " should be itself", props->node_id);
> +                           " should be itself (got %" PRIu16 ")",
> +                           props->node_id, numa_info[props->node_id].initiator);
>                  return;
>              }
>              numa_info[props->node_id].has_cpu = true;

Re: [PATCH 2/2] numa: Parse initiator= attribute before cpus= attribute

[Michal Prívozník]

On 7/9/21 11:26 AM, Igor Mammedov wrote:
> On Wed,  7 Jul 2021 15:40:30 +0200
> Michal Privoznik <mprivozn@redhat.com> wrote:
> 
>> When parsing cpus= attribute of -numa object couple of checks
>> is performed, such as correct initiator setting (see the if()
>> statement at the end of for() loop in
>> machine_set_cpu_numa_node()).
>>
>> However, with the current code cpus= attribute is parsed before
>> initiator= attribute and thus the check may fail even though it
>> is not obvious why. But since parsing the initiator= attribute
>> does not depend on the cpus= attribute we can swap the order of
>> the two.
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
> FYI:
> I'm planning to deprecate '-numa node,cpus=' in favor of '-numa cpu'.

Yes, I'm aware of that and I remember I posted some patches for libvirt
that you reviewed. I need to return to them rather sooner than later.
Meanwhile, this patch helps.

Thanks,
Michal

Re: [PATCH 1/2] numa: Report expected initiator

[Pankaj Gupta]

> When setting up NUMA with HMAT enabled there's a check performed
> in machine_set_cpu_numa_node() that reports an error when a NUMA
> node has a CPU but the node's initiator is not itself. The error
> message reported contains only the expected value and not the
> actual value (which is different because an error is being
> reported). Report both values in the error message.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  hw/core/machine.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 57c18f909a..6f59fb0b7f 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -728,7 +728,8 @@ void machine_set_cpu_numa_node(MachineState *machine,
>              if ((numa_info[props->node_id].initiator < MAX_NODES) &&
>                  (props->node_id != numa_info[props->node_id].initiator)) {
>                  error_setg(errp, "The initiator of CPU NUMA node %" PRId64
> -                        " should be itself", props->node_id);
> +                           " should be itself (got %" PRIu16 ")",
> +                           props->node_id, numa_info[props->node_id].initiator);
>                  return;
>              }
>              numa_info[props->node_id].has_cpu = true;

Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>

Re: [PATCH 0/2] numa: Parse initiator= attribute before cpus= attribute

[Eduardo Habkost]

On Wed, Jul 07, 2021 at 03:40:28PM +0200, Michal Privoznik wrote:
> See 2/2 for explanation. The first patch is just cosmetics.
> 
> Michal Privoznik (2):
>   numa: Report expected initiator
>   numa: Parse initiator= attribute before cpus= attribute

Queued, thanks!

-- 
Eduardo