[PATCH 10/14] qemu: Add @nodemaskRet argument to qemuBuildMemoryBackendProps()

Michal Privoznik posted 14 patches 2 years, 9 months ago
There is a newer version of this series
[PATCH 10/14] qemu: Add @nodemaskRet argument to qemuBuildMemoryBackendProps()
Posted by Michal Privoznik 2 years, 9 months ago
While it's true that anybody who's interested in getting
.host-nodes attribute value can just use
virJSONValueObjectGetArray() (and that's exactly what
qemuBuildThreadContextProps() is doing, btw), it somebody is
interested in getting the actual virBitmap, they would have to
parse the JSON array.

Instead, introduce an argument to qemuBuildMemoryBackendProps()
which is set to corresponding value used when formatting the
attribute.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_command.c | 40 ++++++++++++++++++++++++----------------
 src/qemu/qemu_command.h |  4 +++-
 src/qemu/qemu_hotplug.c |  2 +-
 3 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7adcac418f..1b58171287 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3240,6 +3240,7 @@ qemuBuildMemoryGetPagesize(virQEMUDriverConfig *cfg,
  * @def: domain definition object
  * @mem: memory definition object
  * @force: forcibly use one of the backends
+ * @nodemaskRet: [out] bitmap used to format .host-nodes attribute
  *
  * Creates a configuration object that represents memory backend of given guest
  * NUMA node (domain @def and @mem). Use @priv->autoNodeset to fine tune the
@@ -3264,7 +3265,8 @@ qemuBuildMemoryBackendProps(virJSONValue **backendProps,
                             const virDomainDef *def,
                             const virDomainMemoryDef *mem,
                             bool force,
-                            bool systemMemory)
+                            bool systemMemory,
+                            virBitmap **nodemaskRet)
 {
     const char *backendType = "memory-backend-file";
     virDomainNumatuneMemMode mode;
@@ -3437,19 +3439,24 @@ qemuBuildMemoryBackendProps(virJSONValue **backendProps,
             return -1;
     }
 
-    /* Make sure the requested nodeset is sensible */
-    if (nodemask && !virNumaNodesetIsAvailable(nodemask))
-        return -1;
-
-    /* If mode is "restrictive", we should only use cgroups setting allowed memory
-     * nodes, and skip passing the host-nodes and policy parameters to QEMU command
-     * line which means we will use system default memory policy. */
-    if (nodemask && mode != VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE) {
-        if (virJSONValueObjectAdd(&props,
-                                  "m:host-nodes", nodemask,
-                                  "S:policy", qemuNumaPolicyTypeToString(mode),
-                                  NULL) < 0)
+    if (nodemask) {
+        /* Make sure the requested nodeset is sensible */
+        if (!virNumaNodesetIsAvailable(nodemask))
             return -1;
+
+        /* If mode is "restrictive", we should only use cgroups setting allowed memory
+         * nodes, and skip passing the host-nodes and policy parameters to QEMU command
+         * line which means we will use system default memory policy. */
+        if (mode != VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE) {
+            if (virJSONValueObjectAdd(&props,
+                                      "m:host-nodes", nodemask,
+                                      "S:policy", qemuNumaPolicyTypeToString(mode),
+                                      NULL) < 0)
+                return -1;
+
+            if (nodemaskRet)
+                *nodemaskRet = nodemask;
+        }
     }
 
     /* If none of the following is requested... */
@@ -3502,7 +3509,8 @@ qemuBuildMemoryCellBackendProps(virDomainDef *def,
     mem.targetNode = cell;
     mem.info.alias = alias;
 
-    return qemuBuildMemoryBackendProps(props, alias, cfg, priv, def, &mem, false, false);
+    return qemuBuildMemoryBackendProps(props, alias, cfg, priv,
+                                       def, &mem, false, false, NULL);
 }
 
 
@@ -3526,7 +3534,7 @@ qemuBuildMemoryDimmBackendStr(virCommand *cmd,
     alias = g_strdup_printf("mem%s", mem->info.alias);
 
     if (qemuBuildMemoryBackendProps(&props, alias, cfg,
-                                    priv, def, mem, true, false) < 0)
+                                    priv, def, mem, true, false, NULL) < 0)
         return -1;
 
     if (qemuBuildThreadContextProps(&tcProps, &props, priv) < 0)
@@ -7150,7 +7158,7 @@ qemuBuildMemCommandLineMemoryDefaultBackend(virCommand *cmd,
     mem.info.alias = (char *) defaultRAMid;
 
     if (qemuBuildMemoryBackendProps(&props, defaultRAMid, cfg,
-                                    priv, def, &mem, false, true) < 0)
+                                    priv, def, &mem, false, true, NULL) < 0)
         return -1;
 
     if (qemuBuildThreadContextProps(&tcProps, &props, priv) < 0)
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index c49096a057..9074822bc5 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -22,6 +22,7 @@
 #pragma once
 
 #include "domain_conf.h"
+#include "virbitmap.h"
 #include "vircommand.h"
 #include "virenum.h"
 #include "qemu_block.h"
@@ -140,7 +141,8 @@ int qemuBuildMemoryBackendProps(virJSONValue **backendProps,
                                 const virDomainDef *def,
                                 const virDomainMemoryDef *mem,
                                 bool force,
-                                bool systemMemory);
+                                bool systemMemory,
+                                virBitmap **nodemaskRet);
 
 virJSONValue *
 qemuBuildMemoryDeviceProps(virQEMUDriverConfig *cfg,
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index da17525824..b9f6a031de 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2280,7 +2280,7 @@ qemuDomainAttachMemory(virQEMUDriver *driver,
         goto cleanup;
 
     if (qemuBuildMemoryBackendProps(&props, objalias, cfg,
-                                    priv, vm->def, mem, true, false) < 0)
+                                    priv, vm->def, mem, true, false, NULL) < 0)
         goto cleanup;
 
     if (qemuProcessBuildDestroyMemoryPaths(driver, vm, mem, true) < 0)
-- 
2.39.2
Re: [PATCH 10/14] qemu: Add @nodemaskRet argument to qemuBuildMemoryBackendProps()
Posted by Andrea Bolognani 2 years, 9 months ago
On Wed, Mar 08, 2023 at 12:14:37PM +0100, Michal Privoznik wrote:
> -    /* Make sure the requested nodeset is sensible */
> -    if (nodemask && !virNumaNodesetIsAvailable(nodemask))
> -        return -1;
> -
> -    /* If mode is "restrictive", we should only use cgroups setting allowed memory
> -     * nodes, and skip passing the host-nodes and policy parameters to QEMU command
> -     * line which means we will use system default memory policy. */
> -    if (nodemask && mode != VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE) {
> -        if (virJSONValueObjectAdd(&props,
> -                                  "m:host-nodes", nodemask,
> -                                  "S:policy", qemuNumaPolicyTypeToString(mode),
> -                                  NULL) < 0)
> +    if (nodemask) {
> +        /* Make sure the requested nodeset is sensible */
> +        if (!virNumaNodesetIsAvailable(nodemask))
>              return -1;
> +
> +        /* If mode is "restrictive", we should only use cgroups setting allowed memory
> +         * nodes, and skip passing the host-nodes and policy parameters to QEMU command
> +         * line which means we will use system default memory policy. */
> +        if (mode != VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE) {
> +            if (virJSONValueObjectAdd(&props,
> +                                      "m:host-nodes", nodemask,
> +                                      "S:policy", qemuNumaPolicyTypeToString(mode),
> +                                      NULL) < 0)
> +                return -1;
> +
> +            if (nodemaskRet)
> +                *nodemaskRet = nodemask;
> +        }
>      }

I don't mind the additional nesting, but adding it at the same time
as you're making functional changes makes it harder to focus on the
latter.

Please separate the functional and non-functional parts of this
change into two commits.

-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 10/14] qemu: Add @nodemaskRet argument to qemuBuildMemoryBackendProps()
Posted by Kristina Hanicova 2 years, 9 months ago
On Wed, Mar 8, 2023 at 12:16 PM Michal Privoznik <mprivozn@redhat.com>
wrote:

> While it's true that anybody who's interested in getting
> .host-nodes attribute value can just use
> virJSONValueObjectGetArray() (and that's exactly what
> qemuBuildThreadContextProps() is doing, btw), it somebody is
>

*if


> interested in getting the actual virBitmap, they would have to
> parse the JSON array.
>
> Instead, introduce an argument to qemuBuildMemoryBackendProps()
> which is set to corresponding value used when formatting the
> attribute.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_command.c | 40 ++++++++++++++++++++++++----------------
>  src/qemu/qemu_command.h |  4 +++-
>  src/qemu/qemu_hotplug.c |  2 +-
>  3 files changed, 28 insertions(+), 18 deletions(-)
>


Reviewed-by: Kristina Hanicova <khanicov@redhat.com>

Kristina