From: Wim ten Have <wim.ten.have@oracle.com>

This patch generates a NUMA distance-aware libxl description from the
information extracted from a NUMA distance-aware libvirt XML file.

By default, if no NUMA node distance information is supplied in the
libvirt XML file, this patch uses the distances 10 for local and 20
for remote nodes/sockets."

Signed-off-by: Wim ten Have <wim.ten.have@oracle.com>
---
Changes on v1:
- Fix function calling (coding) standards.
- Use virReportError(...) under all failing circumstances.
- Reduce redundant (format->parse) code sorting bitmaps.
- Avoid non GNU shorthand notations, difficult code practice.
Changes on v2:
- Have autonomous defaults applied from virDomainNumaGetNodeDistance.
- Automatically make Xen driver simulate vnuma pnodes on non NUMA h/w.
- Compute 'memory unit=' making it the sum of all node memory.
Changes on v4:
- Escape virDomainNumaGetNodeCount() if effective.
---
 src/libxl/libxl_conf.c   | 121 +++++++++++++++++++++++++++++++++++++++++++++++
 src/libxl/libxl_driver.c |   3 +-
 2 files changed, 123 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 34233a955..adac6c19c 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -374,6 +374,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
                           def->features[VIR_DOMAIN_FEATURE_APIC] ==
                           VIR_TRISTATE_SWITCH_ON);
         libxl_defbool_set(&b_info->u.hvm.acpi,
+                          virDomainNumaGetNodeCount(def->numa) > 0 ||
                           def->features[VIR_DOMAIN_FEATURE_ACPI] ==
                           VIR_TRISTATE_SWITCH_ON);
 
@@ -618,6 +619,121 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
     return 0;
 }
 
+#ifdef LIBXL_HAVE_VNUMA
+static int
+libxlMakeVnumaList(virDomainDefPtr def,
+                   libxl_ctx *ctx,
+                   libxl_domain_config *d_config)
+{
+    int ret = -1;
+    size_t i, j;
+    size_t nr_nodes;
+    size_t num_vnuma;
+    bool simulate = false;
+    virBitmapPtr bitmap = NULL;
+    virDomainNumaPtr numa = def->numa;
+    libxl_domain_build_info *b_info = &d_config->b_info;
+    libxl_physinfo physinfo;
+    libxl_vnode_info *vnuma_nodes = NULL;
+
+    if (!numa)
+        return 0;
+
+    num_vnuma = virDomainNumaGetNodeCount(numa);
+    if (!num_vnuma)
+        return 0;
+
+    libxl_physinfo_init(&physinfo);
+    if (libxl_get_physinfo(ctx, &physinfo) < 0) {
+        libxl_physinfo_dispose(&physinfo);
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("libxl_get_physinfo_info failed"));
+        return -1;
+    }
+    nr_nodes = physinfo.nr_nodes;
+    libxl_physinfo_dispose(&physinfo);
+
+    if (num_vnuma > nr_nodes) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("Number of configured numa cells %zu exceeds the physical available nodes %zu, guest simulates numa"),
+                       num_vnuma, nr_nodes);
+        simulate = true;
+    }
+
+    /*
+     * allocate the vnuma_nodes for assignment under b_info.
+     */
+    if (VIR_ALLOC_N(vnuma_nodes, num_vnuma) < 0)
+        return -1;
+
+    /*
+     * parse the vnuma vnodes data.
+     */
+    for (i = 0; i < num_vnuma; i++) {
+        int cpu;
+        libxl_bitmap vcpu_bitmap;
+        libxl_vnode_info *p = &vnuma_nodes[i];
+
+        libxl_vnode_info_init(p);
+
+        /* pnode */
+        p->pnode = simulate ? 0 : i;
+
+        /* memory size */
+        p->memkb = virDomainNumaGetNodeMemorySize(numa, i);
+
+        /* vcpus */
+        bitmap = virDomainNumaGetNodeCpumask(numa, i);
+        if (bitmap == NULL) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("vnuma sibling %zu missing vcpus set"), i);
+            goto cleanup;
+        }
+
+        if ((cpu = virBitmapNextSetBit(bitmap, -1)) < 0)
+            goto cleanup;
+
+        libxl_bitmap_init(&vcpu_bitmap);
+        if (libxl_cpu_bitmap_alloc(ctx, &vcpu_bitmap, b_info->max_vcpus)) {
+            virReportOOMError();
+            goto cleanup;
+        }
+
+        do {
+            libxl_bitmap_set(&vcpu_bitmap, cpu);
+        } while ((cpu = virBitmapNextSetBit(bitmap, cpu)) >= 0);
+
+        libxl_bitmap_copy_alloc(ctx, &p->vcpus, &vcpu_bitmap);
+        libxl_bitmap_dispose(&vcpu_bitmap);
+
+        /* vdistances */
+        if (VIR_ALLOC_N(p->distances, num_vnuma) < 0)
+            goto cleanup;
+        p->num_distances = num_vnuma;
+
+        for (j = 0; j < num_vnuma; j++)
+            p->distances[j] = virDomainNumaGetNodeDistance(numa, i, j);
+    }
+
+    b_info->vnuma_nodes = vnuma_nodes;
+    b_info->num_vnuma_nodes = num_vnuma;
+
+    ret = 0;
+
+ cleanup:
+    if (ret) {
+        for (i = 0; i < num_vnuma; i++) {
+            libxl_vnode_info *p = &vnuma_nodes[i];
+
+            VIR_FREE(p->distances);
+        }
+        VIR_FREE(vnuma_nodes);
+    }
+
+    return ret;
+}
+#endif
+
 static int
 libxlDiskSetDiscard(libxl_device_disk *x_disk, int discard)
 {
@@ -2209,6 +2325,11 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
     if (libxlMakeDomBuildInfo(def, ctx, caps, d_config) < 0)
         return -1;
 
+#ifdef LIBXL_HAVE_VNUMA
+    if (libxlMakeVnumaList(def, ctx, d_config) < 0)
+        return -1;
+#endif
+
     if (libxlMakeDiskList(def, d_config) < 0)
         return -1;
 
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 8483d6ecf..656f4a82d 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -2788,7 +2788,8 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag
     virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
 
     if (flags & VIR_DOMAIN_DEFINE_VALIDATE)
-        parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA;
+        parse_flags |= (VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA |
+                        VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);
 
     if (!(def = virDomainDefParseString(xml, cfg->caps, driver->xmlopt,
                                         NULL, parse_flags)))
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 10/12/2017 01:31 PM, Wim Ten Have wrote:
> From: Wim ten Have <wim.ten.have@oracle.com>
> 
> This patch generates a NUMA distance-aware libxl description from the
> information extracted from a NUMA distance-aware libvirt XML file.
> 
> By default, if no NUMA node distance information is supplied in the
> libvirt XML file, this patch uses the distances 10 for local and 20
> for remote nodes/sockets."

Spurious " at the end of above sentence.

> Signed-off-by: Wim ten Have <wim.ten.have@oracle.com>
> ---
> Changes on v1:
> - Fix function calling (coding) standards.
> - Use virReportError(...) under all failing circumstances.
> - Reduce redundant (format->parse) code sorting bitmaps.
> - Avoid non GNU shorthand notations, difficult code practice.
> Changes on v2:
> - Have autonomous defaults applied from virDomainNumaGetNodeDistance.
> - Automatically make Xen driver simulate vnuma pnodes on non NUMA h/w.
> - Compute 'memory unit=' making it the sum of all node memory.
> Changes on v4:
> - Escape virDomainNumaGetNodeCount() if effective.
> ---
>   src/libxl/libxl_conf.c   | 121 +++++++++++++++++++++++++++++++++++++++++++++++
>   src/libxl/libxl_driver.c |   3 +-
>   2 files changed, 123 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 34233a955..adac6c19c 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -374,6 +374,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>                             def->features[VIR_DOMAIN_FEATURE_APIC] ==
>                             VIR_TRISTATE_SWITCH_ON);
>           libxl_defbool_set(&b_info->u.hvm.acpi,
> +                          virDomainNumaGetNodeCount(def->numa) > 0 || >                             def->features[VIR_DOMAIN_FEATURE_ACPI] ==
>                             VIR_TRISTATE_SWITCH_ON);

This doesn't seem right. At a minimum we should ensure 
def->features[VIR_DOMAIN_FEATURE_ACPI] is set to ON when a vnuma configuration 
is defined. I think libxlDomainDefPostParse is a better place to do that.

>   
> @@ -618,6 +619,121 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>       return 0;
>   }
>   
> +#ifdef LIBXL_HAVE_VNUMA
> +static int
> +libxlMakeVnumaList(virDomainDefPtr def,
> +                   libxl_ctx *ctx,
> +                   libxl_domain_config *d_config)
> +{
> +    int ret = -1;
> +    size_t i, j;
> +    size_t nr_nodes;
> +    size_t num_vnuma;
> +    bool simulate = false;
> +    virBitmapPtr bitmap = NULL;
> +    virDomainNumaPtr numa = def->numa;
> +    libxl_domain_build_info *b_info = &d_config->b_info;
> +    libxl_physinfo physinfo;
> +    libxl_vnode_info *vnuma_nodes = NULL;
> +
> +    if (!numa)
> +        return 0;
> +
> +    num_vnuma = virDomainNumaGetNodeCount(numa);
> +    if (!num_vnuma)
> +        return 0;
> +
> +    libxl_physinfo_init(&physinfo);
> +    if (libxl_get_physinfo(ctx, &physinfo) < 0) {
> +        libxl_physinfo_dispose(&physinfo);
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("libxl_get_physinfo_info failed"));
> +        return -1;
> +    }
> +    nr_nodes = physinfo.nr_nodes;
> +    libxl_physinfo_dispose(&physinfo);
> +
> +    if (num_vnuma > nr_nodes) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("Number of configured numa cells %zu exceeds the physical available nodes %zu, guest simulates numa"),
> +                       num_vnuma, nr_nodes);

It seems this should be VIR_WARN instead of virReportError, afterall an error is 
not returned.

> +        simulate = true;
> +    }
> +
> +    /*
> +     * allocate the vnuma_nodes for assignment under b_info.
> +     */
> +    if (VIR_ALLOC_N(vnuma_nodes, num_vnuma) < 0)
> +        return -1;
> +
> +    /*
> +     * parse the vnuma vnodes data.
> +     */
> +    for (i = 0; i < num_vnuma; i++) {
> +        int cpu;
> +        libxl_bitmap vcpu_bitmap;
> +        libxl_vnode_info *p = &vnuma_nodes[i];
> +
> +        libxl_vnode_info_init(p);
> +
> +        /* pnode */
> +        p->pnode = simulate ? 0 : i;

So any time the number of vnuma nodes exceeds the number of physical nodes, all 
vnuma nodes are confined to physical node 0? Does xl behave this way too?

> +
> +        /* memory size */
> +        p->memkb = virDomainNumaGetNodeMemorySize(numa, i);
> +
> +        /* vcpus */
> +        bitmap = virDomainNumaGetNodeCpumask(numa, i);
> +        if (bitmap == NULL) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("vnuma sibling %zu missing vcpus set"), i);
> +            goto cleanup;
> +        }
> +
> +        if ((cpu = virBitmapNextSetBit(bitmap, -1)) < 0)
> +            goto cleanup;
> +
> +        libxl_bitmap_init(&vcpu_bitmap);
> +        if (libxl_cpu_bitmap_alloc(ctx, &vcpu_bitmap, b_info->max_vcpus)) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +
> +        do {
> +            libxl_bitmap_set(&vcpu_bitmap, cpu);
> +        } while ((cpu = virBitmapNextSetBit(bitmap, cpu)) >= 0);
> +
> +        libxl_bitmap_copy_alloc(ctx, &p->vcpus, &vcpu_bitmap);
> +        libxl_bitmap_dispose(&vcpu_bitmap);
> +
> +        /* vdistances */
> +        if (VIR_ALLOC_N(p->distances, num_vnuma) < 0)
> +            goto cleanup;
> +        p->num_distances = num_vnuma;
> +
> +        for (j = 0; j < num_vnuma; j++)
> +            p->distances[j] = virDomainNumaGetNodeDistance(numa, i, j);
> +    }
> +
> +    b_info->vnuma_nodes = vnuma_nodes;
> +    b_info->num_vnuma_nodes = num_vnuma;
> +
> +    ret = 0;
> +
> + cleanup:
> +    if (ret) {
> +        for (i = 0; i < num_vnuma; i++) {
> +            libxl_vnode_info *p = &vnuma_nodes[i];
> +
> +            VIR_FREE(p->distances);
> +        }
> +        VIR_FREE(vnuma_nodes);
> +    }
> +
> +    return ret;
> +}
> +#endif
> +
>   static int
>   libxlDiskSetDiscard(libxl_device_disk *x_disk, int discard)
>   {
> @@ -2209,6 +2325,11 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>       if (libxlMakeDomBuildInfo(def, ctx, caps, d_config) < 0)
>           return -1;
>   
> +#ifdef LIBXL_HAVE_VNUMA
> +    if (libxlMakeVnumaList(def, ctx, d_config) < 0)
> +        return -1;
> +#endif
> +
>       if (libxlMakeDiskList(def, d_config) < 0)
>           return -1;
>   
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 8483d6ecf..656f4a82d 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -2788,7 +2788,8 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag
>       virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
>   
>       if (flags & VIR_DOMAIN_DEFINE_VALIDATE)
> -        parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA;
> +        parse_flags |= (VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA |
> +                        VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);

Why is this change needed? In its current form, the patch doesn't touch any code 
in the domain def post parse callback.

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, 26 Oct 2017 17:40:37 -0600
Jim Fehlig <jfehlig@suse.com> wrote:

> On 10/12/2017 01:31 PM, Wim Ten Have wrote:
> > From: Wim ten Have <wim.ten.have@oracle.com>
> > 
> > This patch generates a NUMA distance-aware libxl description from the
> > information extracted from a NUMA distance-aware libvirt XML file.
	...
> > +        /* pnode */
> > +        p->pnode = simulate ? 0 : i;  
> 
> So any time the number of vnuma nodes exceeds the number of physical nodes, all 
> vnuma nodes are confined to physical node 0?

  Yes.
>                                              Does xl behave this way too?

  Yes.

> > +
> > +        /* memory size */
> > +        p->memkb = virDomainNumaGetNodeMemorySize(numa, i);
> > +
> > +        /* vcpus */
> > +        bitmap = virDomainNumaGetNodeCpumask(numa, i);
> > +        if (bitmap == NULL) {
> > +            virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                           _("vnuma sibling %zu missing vcpus set"), i);
> > +            goto cleanup;
> > +        }
> > +
> > +        if ((cpu = virBitmapNextSetBit(bitmap, -1)) < 0)
> > +            goto cleanup;
> > +
> > +        libxl_bitmap_init(&vcpu_bitmap);
> > +        if (libxl_cpu_bitmap_alloc(ctx, &vcpu_bitmap, b_info->max_vcpus)) {
> > +            virReportOOMError();
> > +            goto cleanup;
> > +        }
> > +
> > +        do {
> > +            libxl_bitmap_set(&vcpu_bitmap, cpu);
> > +        } while ((cpu = virBitmapNextSetBit(bitmap, cpu)) >= 0);
> > +
> > +        libxl_bitmap_copy_alloc(ctx, &p->vcpus, &vcpu_bitmap);
> > +        libxl_bitmap_dispose(&vcpu_bitmap);
> > +
> > +        /* vdistances */
> > +        if (VIR_ALLOC_N(p->distances, num_vnuma) < 0)
> > +            goto cleanup;
> > +        p->num_distances = num_vnuma;
> > +
> > +        for (j = 0; j < num_vnuma; j++)
> > +            p->distances[j] = virDomainNumaGetNodeDistance(numa, i, j);
> > +    }
> > +
> > +    b_info->vnuma_nodes = vnuma_nodes;
> > +    b_info->num_vnuma_nodes = num_vnuma;
> > +
> > +    ret = 0;
> > +
> > + cleanup:
> > +    if (ret) {
> > +        for (i = 0; i < num_vnuma; i++) {
> > +            libxl_vnode_info *p = &vnuma_nodes[i];
> > +
> > +            VIR_FREE(p->distances);
> > +        }
> > +        VIR_FREE(vnuma_nodes);
> > +    }
> > +
> > +    return ret;
> > +}
> > +#endif
> > +
> >   static int
> >   libxlDiskSetDiscard(libxl_device_disk *x_disk, int discard)
> >   {
> > @@ -2209,6 +2325,11 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
> >       if (libxlMakeDomBuildInfo(def, ctx, caps, d_config) < 0)
> >           return -1;
> >   
> > +#ifdef LIBXL_HAVE_VNUMA
> > +    if (libxlMakeVnumaList(def, ctx, d_config) < 0)
> > +        return -1;
> > +#endif
> > +
> >       if (libxlMakeDiskList(def, d_config) < 0)
> >           return -1;
> >   
> > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> > index 8483d6ecf..656f4a82d 100644
> > --- a/src/libxl/libxl_driver.c
> > +++ b/src/libxl/libxl_driver.c
> > @@ -2788,7 +2788,8 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag
> >       virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
> >   
> >       if (flags & VIR_DOMAIN_DEFINE_VALIDATE)
> > -        parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA;
> > +        parse_flags |= (VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA |
> > +                        VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);  
> 
> Why is this change needed? In its current form, the patch doesn't touch any code 
> in the domain def post parse callback.

  I remove this.  I added this to forcibly recompute memory size in case total_memory
  held a value not matching the numa cells setting. (!= 0)
  See virDomainDefPostParseMemory()
  
    /* Attempt to infer the initial memory size from the sum NUMA memory sizes
     * in case ABI updates are allowed or the <memory> element wasn't specified */
    if (def->mem.total_memory == 0 ||
        parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE ||
        parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE_MIGRATION)
        numaMemory = virDomainNumaGetMemorySize(def->numa);

  Will address all other brought comments under v6.

Regards,
- Wim.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list