1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/2] Don't choke on valid NUMA nodesets on daemon restart
  4. View patch

[libvirt] [PATCH 2/2] qemu: Figure out nodeset bitmap size correctly

Andrea Bolognani posted 2 patches 7 years, 1 month ago
There is a newer version of this series
[libvirt] [PATCH 2/2] qemu: Figure out nodeset bitmap size correctly
Posted by Andrea Bolognani 7 years, 1 month ago 
The current private XML parsing code relies on the assumption
that NUMA node IDs start from 0 and are densely allocated,
neither of which is necessarily the case.

Change it so that the bitmap size is dynamically calculated by
looking at NUMA node IDs instead, which ensures all nodes will
be able to fit and thus the bitmap will be parsed successfully.

Update one of the test cases so that it would fail with the
previous approach, but passes with the new one.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1490158

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/qemu/qemu_domain.c                    | 11 ++++++++++-
 tests/qemustatusxml2xmldata/modern-in.xml |  2 +-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 100304fd05..b126c69490 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2248,6 +2248,8 @@ qemuDomainObjPrivateXMLParseAutomaticPlacement(xmlXPathContextPtr ctxt,
     virCapsPtr caps = NULL;
     char *nodeset;
     char *cpuset;
+    int nodesetSize = 0;
+    int i;
     int ret = -1;
 
     nodeset = virXPathString("string(./numad/@nodeset)", ctxt);
@@ -2259,8 +2261,15 @@ qemuDomainObjPrivateXMLParseAutomaticPlacement(xmlXPathContextPtr ctxt,
     if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
         goto cleanup;
 
+    /* Figure out how big the nodeset bitmap needs to be.
+     * This is necessary because NUMA node IDs are not guaranteed to
+     * start from 0 or be densely allocated */
+    for (i = 0; i < caps->host.nnumaCell; i++) {
+        nodesetSize = MAX(nodesetSize, caps->host.numaCell[i]->num + 1);
+    }
+
     if (nodeset &&
-        virBitmapParse(nodeset, &priv->autoNodeset, caps->host.nnumaCell_max) < 0)
+        virBitmapParse(nodeset, &priv->autoNodeset, nodesetSize) < 0)
         goto cleanup;
 
     if (cpuset) {
diff --git a/tests/qemustatusxml2xmldata/modern-in.xml b/tests/qemustatusxml2xmldata/modern-in.xml
index 2e166e6e67..c1e57618b6 100644
--- a/tests/qemustatusxml2xmldata/modern-in.xml
+++ b/tests/qemustatusxml2xmldata/modern-in.xml
@@ -252,7 +252,7 @@
     <device alias='usb'/>
     <device alias='ide0-0-0'/>
   </devices>
-  <numad nodeset='0' cpuset='0-7'/>
+  <numad nodeset='6' cpuset='0-7'/>
   <libDir path='/var/lib/libvirt/qemu/domain-1-upstream'/>
   <channelTargetDir path='/var/lib/libvirt/qemu/channel/target/domain-1-upstream'/>
   <chardevStdioLogd/>
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: Figure out nodeset bitmap size correctly
Posted by Peter Krempa 7 years ago 
On Thu, Apr 12, 2018 at 08:47:58 +0200, Andrea Bolognani wrote:
> The current private XML parsing code relies on the assumption
> that NUMA node IDs start from 0 and are densely allocated,
> neither of which is necessarily the case.
> 
> Change it so that the bitmap size is dynamically calculated by
> looking at NUMA node IDs instead, which ensures all nodes will
> be able to fit and thus the bitmap will be parsed successfully.
> 
> Update one of the test cases so that it would fail with the
> previous approach, but passes with the new one.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1490158

While the patch below will fix this case, I'd also like to see that the
parsing of the bitmaps is made non-fatal. If the nodesets are missing
some of the reported data will be wrong, but not having this is
certainly not a deal-breaker so that we should not reconnect to qemu.

> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/qemu/qemu_domain.c                    | 11 ++++++++++-
>  tests/qemustatusxml2xmldata/modern-in.xml |  2 +-
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 100304fd05..b126c69490 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2248,6 +2248,8 @@ qemuDomainObjPrivateXMLParseAutomaticPlacement(xmlXPathContextPtr ctxt,
>      virCapsPtr caps = NULL;
>      char *nodeset;
>      char *cpuset;
> +    int nodesetSize = 0;
> +    int i;
>      int ret = -1;
>  
>      nodeset = virXPathString("string(./numad/@nodeset)", ctxt);
> @@ -2259,8 +2261,15 @@ qemuDomainObjPrivateXMLParseAutomaticPlacement(xmlXPathContextPtr ctxt,
>      if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
>          goto cleanup;
>  
> +    /* Figure out how big the nodeset bitmap needs to be.
> +     * This is necessary because NUMA node IDs are not guaranteed to
> +     * start from 0 or be densely allocated */
> +    for (i = 0; i < caps->host.nnumaCell; i++) {
> +        nodesetSize = MAX(nodesetSize, caps->host.numaCell[i]->num + 1);
> +    }

Syntax-check is moaning about this.

> +
>      if (nodeset &&
> -        virBitmapParse(nodeset, &priv->autoNodeset, caps->host.nnumaCell_max) < 0)
> +        virBitmapParse(nodeset, &priv->autoNodeset, nodesetSize) < 0)
>          goto cleanup;
>  
>      if (cpuset) {

ACK if you actually run syntax-check.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: Figure out nodeset bitmap size correctly
Posted by Andrea Bolognani 7 years ago 
On Thu, 2018-04-19 at 14:11 +0200, Peter Krempa wrote:
> On Thu, Apr 12, 2018 at 08:47:58 +0200, Andrea Bolognani wrote:
> > The current private XML parsing code relies on the assumption
> > that NUMA node IDs start from 0 and are densely allocated,
> > neither of which is necessarily the case.
> > 
> > Change it so that the bitmap size is dynamically calculated by
> > looking at NUMA node IDs instead, which ensures all nodes will
> > be able to fit and thus the bitmap will be parsed successfully.
> > 
> > Update one of the test cases so that it would fail with the
> > previous approach, but passes with the new one.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1490158
> 
> While the patch below will fix this case, I'd also like to see that the
> parsing of the bitmaps is made non-fatal. If the nodesets are missing
> some of the reported data will be wrong, but not having this is
> certainly not a deal-breaker so that we should not reconnect to qemu.

Mh, that's trickier than I initially though, because
virBitmapParseSeparator() calls virReportError() itself on parse
failure, and changing doesn't sound feasible.

> ACK if you actually run syntax-check.

Shame on me for not doing so the first time around :(

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: Figure out nodeset bitmap size correctly
Posted by Peter Krempa 7 years ago 
On Thu, Apr 19, 2018 at 15:45:52 +0200, Andrea Bolognani wrote:
> On Thu, 2018-04-19 at 14:11 +0200, Peter Krempa wrote:
> > On Thu, Apr 12, 2018 at 08:47:58 +0200, Andrea Bolognani wrote:
> > > The current private XML parsing code relies on the assumption
> > > that NUMA node IDs start from 0 and are densely allocated,
> > > neither of which is necessarily the case.
> > > 
> > > Change it so that the bitmap size is dynamically calculated by
> > > looking at NUMA node IDs instead, which ensures all nodes will
> > > be able to fit and thus the bitmap will be parsed successfully.
> > > 
> > > Update one of the test cases so that it would fail with the
> > > previous approach, but passes with the new one.
> > > 
> > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1490158
> > 
> > While the patch below will fix this case, I'd also like to see that the
> > parsing of the bitmaps is made non-fatal. If the nodesets are missing
> > some of the reported data will be wrong, but not having this is
> > certainly not a deal-breaker so that we should not reconnect to qemu.
> 
> Mh, that's trickier than I initially though, because
> virBitmapParseSeparator() calls virReportError() itself on parse
> failure, and changing doesn't sound feasible.

I think logging of the error is okay, we just probably should reset it.

At any rate this patch can be pushed as-is ... well after it passes
build checks ;)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: Figure out nodeset bitmap size correctly
Posted by Andrea Bolognani 7 years ago 
On Thu, 2018-04-19 at 16:15 +0200, Peter Krempa wrote:
> On Thu, Apr 19, 2018 at 15:45:52 +0200, Andrea Bolognani wrote:
> > Mh, that's trickier than I initially though, because
> > virBitmapParseSeparator() calls virReportError() itself on parse
> > failure, and changing doesn't sound feasible.
> 
> I think logging of the error is okay, we just probably should reset it.
> 
> At any rate this patch can be pushed as-is ... well after it passes
> build checks ;)

Are you referring to the syntax-check failure? I posted a v2 that
fixes that, but also changes 1/2, without which 2/2 can't work.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: Figure out nodeset bitmap size correctly
Posted by Peter Krempa 7 years ago 
On Thu, Apr 19, 2018 at 16:37:23 +0200, Andrea Bolognani wrote:
> On Thu, 2018-04-19 at 16:15 +0200, Peter Krempa wrote:
> > On Thu, Apr 19, 2018 at 15:45:52 +0200, Andrea Bolognani wrote:
> > > Mh, that's trickier than I initially though, because
> > > virBitmapParseSeparator() calls virReportError() itself on parse
> > > failure, and changing doesn't sound feasible.
> > 
> > I think logging of the error is okay, we just probably should reset it.
> > 
> > At any rate this patch can be pushed as-is ... well after it passes
> > build checks ;)
> 
> Are you referring to the syntax-check failure? I posted a v2 that
> fixes that, but also changes 1/2, without which 2/2 can't work.

No, I was referring to making that whole function pass even when the
numad hints can't be parsed.

I'll do a review of v2 in a bit.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.