10:21 a.m.
See detailed explanation in the commit message for patch 1/4
and the corresponding bug report.
Andrea Bolognani (4):
util: Introduce virBitmapUnion()
fixup? util: Optimize virBitmapUnion()
util: Introduce virNumaNodesetToCPUset()
qemu: Fix qemuProcessInitCpuAffinity()
src/libvirt_private.syms | 2 ++
src/qemu/qemu_process.c | 7 ++++-
src/util/virbitmap.c | 32 +++++++++++++++++++++++
src/util/virbitmap.h | 4 +++
src/util/virnuma.c | 55 ++++++++++++++++++++++++++++++++++++++++
src/util/virnuma.h | 2 ++
tests/virbitmaptest.c | 37 +++++++++++++++++++++++++++
7 files changed, 138 insertions(+), 1 deletion(-)
--
2.21.0
10:21 a.m.
New subject: [libvirt] [PATCH 1/4] util: Introduce virBitmapUnion()
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virbitmap.c | 26 ++++++++++++++++++++++++++
src/util/virbitmap.h | 4 ++++
tests/virbitmaptest.c | 37 +++++++++++++++++++++++++++++++++++++
4 files changed, 68 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 909975750c..678d0348a2 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1517,6 +1517,7 @@ virBitmapSubtract;
virBitmapToData;
virBitmapToDataBuf;
virBitmapToString;
+virBitmapUnion;
# util/virbuffer.h
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index 91df4b8601..1f0db563ab 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -1260,6 +1260,32 @@ virBitmapIntersect(virBitmapPtr a,
}
+/**
+ * virBitmapUnion:
+ * @a: bitmap, modified to contain result
+ * @b: other bitmap
+ *
+ * Performs union of two bitmaps: a = union(a, b)
+ *
+ * Returns 0 on success, <0 on failure.
+ */
+int
+virBitmapUnion(virBitmapPtr a,
+ virBitmapPtr b)
+{
+ size_t i;
+
+ for (i = 0; i < b->nbits; i++) {
+ if (virBitmapIsBitSet(b, i)) {
+ if (virBitmapSetBitExpand(a, i) < 0)
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+
/**
* virBitmapSubtract:
* @a: minuend/result
diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h
index 38dc06412a..49d9910fa7 100644
--- a/src/util/virbitmap.h
+++ b/src/util/virbitmap.h
@@ -149,6 +149,10 @@ bool virBitmapOverlaps(virBitmapPtr b1,
void virBitmapIntersect(virBitmapPtr a, virBitmapPtr b)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int virBitmapUnion(virBitmapPtr a,
+ virBitmapPtr b)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
void virBitmapSubtract(virBitmapPtr a, virBitmapPtr b)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c
index 2fbafc0a76..f9d3cfd995 100644
--- a/tests/virbitmaptest.c
+++ b/tests/virbitmaptest.c
@@ -740,6 +740,34 @@ test14(const void *opaque)
return ret;
}
+/* virBitmapUnion() */
+static int
+test15(const void *opaque)
+{
+ const struct testBinaryOpData *data = opaque;
+ VIR_AUTOPTR(virBitmap) amap = NULL;
+ VIR_AUTOPTR(virBitmap) bmap = NULL;
+ VIR_AUTOPTR(virBitmap) resmap = NULL;
+
+ if (!(amap = virBitmapParseUnlimited(data->a)) ||
+ !(bmap = virBitmapParseUnlimited(data->b)) ||
+ !(resmap = virBitmapParseUnlimited(data->res))) {
+ return -1;
+ }
+
+ if (virBitmapUnion(amap, bmap) < 0)
+ return -1;
+
+ if (!virBitmapEqual(amap, resmap)) {
+ fprintf(stderr,
+ "\n bitmap union failed: union('%s', '%s') !=
'%s'\n",
+ data->a, data->b, data->res);
+ return -1;
+ }
+
+ return 0;
+}
+
#define TESTBINARYOP(A, B, RES, FUNC) \
testBinaryOpData.a = A; \
@@ -798,6 +826,15 @@ mymain(void)
TESTBINARYOP("0-3", "0,^0", "0-3", test14);
TESTBINARYOP("0,2", "1,3", "0,2", test14);
+ virTestCounterReset("test15-");
+ TESTBINARYOP("0-1", "0-1", "0-1", test15);
+ TESTBINARYOP("0", "1", "0-1", test15);
+ TESTBINARYOP("0-1", "2-3", "0-3", test15);
+ TESTBINARYOP("0-3", "1-2", "0-3", test15);
+ TESTBINARYOP("0,^0", "12345", "12345", test15);
+ TESTBINARYOP("12345", "0,^0", "12345", test15);
+ TESTBINARYOP("0,^0", "0,^0", "0,^0", test15);
+
return ret;
}
--
2.21.0
6:57 a.m.
New subject: [libvirt] [PATCH 1/4] util: Introduce virBitmapUnion()
On Fri, May 31, 2019 at 05:21:59PM +0200, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virbitmap.c | 26 ++++++++++++++++++++++++++
src/util/virbitmap.h | 4 ++++
tests/virbitmaptest.c | 37 +++++++++++++++++++++++++++++++++++++
4 files changed, 68 insertions(+)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
10:22 a.m.
New subject: [libvirt] [PATCH 2/4] fixup? util: Optimize virBitmapUnion()
The original implementation is extremely straightforward but
not too efficient, because it uses the public API instead of
poking the innards directly. This second implementation does
the latter, and as a consequence can afford to make @b const,
which is nice even though most existing virBitmap APIs use
non-const pointers even for read-only bitmaps.
That said, I'm not entirely sure I got it quite right, so a
review from someone who's well versed in the virBitmap
internals (*cough* Peter *cough*) would be appreciated, and
if my implementation passes muster or can be amended through
review comments I'll gladly squash this commit into the
previous one.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/virbitmap.c | 18 ++++++++++++------
src/util/virbitmap.h | 2 +-
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index 1f0db563ab..f2127c053d 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -1271,17 +1271,23 @@ virBitmapIntersect(virBitmapPtr a,
*/
int
virBitmapUnion(virBitmapPtr a,
- virBitmapPtr b)
+ const virBitmap *b)
{
size_t i;
+ size_t max;
- for (i = 0; i < b->nbits; i++) {
- if (virBitmapIsBitSet(b, i)) {
- if (virBitmapSetBitExpand(a, i) < 0)
- return -1;
- }
+ if (a->nbits < b->nbits &&
+ virBitmapExpand(a, b->nbits) < 0) {
+ return -1;
}
+ max = a->map_len;
+ if (max > b->map_len)
+ max = b->map_len;
+
+ for (i = 0; i < max; i++)
+ a->map[i] |= b->map[i];
+
return 0;
}
diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h
index 49d9910fa7..8696214da8 100644
--- a/src/util/virbitmap.h
+++ b/src/util/virbitmap.h
@@ -150,7 +150,7 @@ void virBitmapIntersect(virBitmapPtr a, virBitmapPtr b)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int virBitmapUnion(virBitmapPtr a,
- virBitmapPtr b)
+ const virBitmap *b)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
void virBitmapSubtract(virBitmapPtr a, virBitmapPtr b)
--
2.21.0
7:10 a.m.
New subject: [libvirt] [PATCH 2/4] fixup? util: Optimize virBitmapUnion()
On Fri, May 31, 2019 at 05:22:00PM +0200, Andrea Bolognani wrote:
The original implementation is extremely straightforward but
not too efficient, because it uses the public API instead of
poking the innards directly. This second implementation does
the latter, and as a consequence can afford to make @b const,
which is nice even though most existing virBitmap APIs use
non-const pointers even for read-only bitmaps.
That said, I'm not entirely sure I got it quite right, so a
review from someone who's well versed in the virBitmap
internals (*cough* Peter *cough*) would be appreciated, and
if my implementation passes muster or can be amended through
review comments I'll gladly squash this commit into the
previous one.
Yes please. Also drop this paragraph. History is written by the winners
;)
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/virbitmap.c | 18 ++++++++++++------
src/util/virbitmap.h | 2 +-
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index 1f0db563ab..f2127c053d 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -1271,17 +1271,23 @@ virBitmapIntersect(virBitmapPtr a,
*/
int
virBitmapUnion(virBitmapPtr a,
- virBitmapPtr b)
+ const virBitmap *b)
{
size_t i;
+ size_t max;
- for (i = 0; i < b->nbits; i++) {
- if (virBitmapIsBitSet(b, i)) {
- if (virBitmapSetBitExpand(a, i) < 0)
- return -1;
- }
+ if (a->nbits < b->nbits &&
+ virBitmapExpand(a, b->nbits) < 0) {
After this, 'b' can hold b->nbits and 'a' can hold b->nbits+1.
if (b->nbits &&
a->nbits < b->nbits &&
virBitmapExpand(a, b->nbits -1) < 0) {
+ return -1;
}
+ max = a->map_len;
+ if (max > b->map_len)
+ max = b->map_len;
You expanded the 'a' bitmap just a few lines above.
So if you can safely go up to b->map_len without accessing
invalid memory in a or missing any set bits in b.
+
+ for (i = 0; i < max; i++)
+ a->map[i] |= b->map[i];
+
return 0;
}
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
9:31 a.m.
New subject: [libvirt] [PATCH 2/4] fixup? util: Optimize virBitmapUnion()
On Mon, 2019-06-03 at 14:10 +0200, Ján Tomko wrote:
On Fri, May 31, 2019 at 05:22:00PM +0200, Andrea Bolognani wrote:
> + if (a->nbits < b->nbits &&
> + virBitmapExpand(a, b->nbits) < 0) {
After this, 'b' can hold b->nbits and 'a' can hold b->nbits+1.
if (b->nbits &&
a->nbits < b->nbits &&
virBitmapExpand(a, b->nbits -1) < 0) {
Yeah, you're right, we need to account for the zero-indexing of bits.
I mean, it's not like the resulting bitmap would be incorrect either
way, but we might end up allocating more memory than it's actually
required.
The first check seems unnecessary, though: the only case in which the
argument to virBitmapExpand() would be incorrect is b->nbits == 0,
but we know that both a->nbits and b->nbits are >= 0 and we also just
verified that a->nbits < b->nbits, so b->nbits must be >= 1 and the
argument to virBitmapExpand() will always be correct. Or am I missing
something?
> + max = a->map_len;
> + if (max > b->map_len)
> + max = b->map_len;
You expanded the 'a' bitmap just a few lines above.
So if you can safely go up to b->map_len without accessing
invalid memory in a or missing any set bits in b.
I thought the above would be necessary to cover the case where a
is bigger than b, but I realize now that iterating over the smaller
b will deal with such a situation just fine. So nice catch, and
consider it fixed :)
--
Andrea Bolognani / Red Hat / Virtualization
9:43 a.m.
New subject: [libvirt] [PATCH 2/4] fixup? util: Optimize virBitmapUnion()
On Mon, Jun 03, 2019 at 04:31:51PM +0200, Andrea Bolognani wrote:
On Mon, 2019-06-03 at 14:10 +0200, Ján Tomko wrote:
> On Fri, May 31, 2019 at 05:22:00PM +0200, Andrea Bolognani wrote:
> > + if (a->nbits < b->nbits &&
> > + virBitmapExpand(a, b->nbits) < 0) {
>
> After this, 'b' can hold b->nbits and 'a' can hold b->nbits+1.
>
> if (b->nbits &&
> a->nbits < b->nbits &&
> virBitmapExpand(a, b->nbits -1) < 0) {
Yeah, you're right, we need to account for the zero-indexing of bits.
I mean, it's not like the resulting bitmap would be incorrect either
way, but we might end up allocating more memory than it's actually
required.
Actually I do think it would be incorrect. The bitmap size is as
important as its values - e.g. if the resulting bitmap had 4 set bits,
a size of 4 vs 5 would alter the result of virBitmapIsAllSet.
It would only be a marginal over-allocation if we were dealing with
map_len.
The first check seems unnecessary, though: the only case in which the
argument to virBitmapExpand() would be incorrect is b->nbits == 0,
but we know that both a->nbits and b->nbits are >= 0 and we also just
verified that a->nbits < b->nbits, so b->nbits must be >= 1 and the
argument to virBitmapExpand() will always be correct. Or am I missing
something?
Right, it will work unless someone changes nbits from size_t to double.
Jano
10:04 a.m.
New subject: [libvirt] [PATCH 2/4] fixup? util: Optimize virBitmapUnion()
On Mon, 2019-06-03 at 16:43 +0200, Ján Tomko wrote:
On Mon, Jun 03, 2019 at 04:31:51PM +0200, Andrea Bolognani wrote:
> On Mon, 2019-06-03 at 14:10 +0200, Ján Tomko wrote:
> > if (b->nbits &&
> > a->nbits < b->nbits &&
> > virBitmapExpand(a, b->nbits -1) < 0) {
>
> Yeah, you're right, we need to account for the zero-indexing of bits.
> I mean, it's not like the resulting bitmap would be incorrect either
> way, but we might end up allocating more memory than it's actually
> required.
Actually I do think it would be incorrect. The bitmap size is as
important as its values - e.g. if the resulting bitmap had 4 set bits,
a size of 4 vs 5 would alter the result of virBitmapIsAllSet.
Oh, right, that's something that I hadn't considered. This is pretty
much exactly why I didn't trust my implementation to be correct O:-)
> The first check seems unnecessary, though: the only case in
which the
> argument to virBitmapExpand() would be incorrect is b->nbits == 0,
> but we know that both a->nbits and b->nbits are >= 0 and we also just
> verified that a->nbits < b->nbits, so b->nbits must be >= 1 and the
> argument to virBitmapExpand() will always be correct. Or am I missing
> something?
Right, it will work unless someone changes nbits from size_t to double.
So ACK to drop the first check while adopting your version of that
hunk?
--
Andrea Bolognani / Red Hat / Virtualization
1:27 a.m.
New subject: [libvirt] [PATCH 2/4] fixup? util: Optimize virBitmapUnion()
On Mon, Jun 03, 2019 at 05:04:51PM +0200, Andrea Bolognani wrote:
On Mon, 2019-06-03 at 16:43 +0200, Ján Tomko wrote:
> On Mon, Jun 03, 2019 at 04:31:51PM +0200, Andrea Bolognani wrote:
> > On Mon, 2019-06-03 at 14:10 +0200, Ján Tomko wrote:
> > > if (b->nbits &&
> > > a->nbits < b->nbits &&
> > > virBitmapExpand(a, b->nbits -1) < 0) {
> >
> > Yeah, you're right, we need to account for the zero-indexing of bits.
> > I mean, it's not like the resulting bitmap would be incorrect either
> > way, but we might end up allocating more memory than it's actually
> > required.
>
> Actually I do think it would be incorrect. The bitmap size is as
> important as its values - e.g. if the resulting bitmap had 4 set bits,
> a size of 4 vs 5 would alter the result of virBitmapIsAllSet.
Oh, right, that's something that I hadn't considered. This is pretty
much exactly why I didn't trust my implementation to be correct O:-)
> > The first check seems unnecessary, though: the only case in which the
> > argument to virBitmapExpand() would be incorrect is b->nbits == 0,
> > but we know that both a->nbits and b->nbits are >= 0 and we also just
> > verified that a->nbits < b->nbits, so b->nbits must be >= 1 and
the
> > argument to virBitmapExpand() will always be correct. Or am I missing
> > something?
>
> Right, it will work unless someone changes nbits from size_t to double.
So ACK to drop the first check while adopting your version of that
hunk?
ACK indeed.
Or you can choose to apply my R-b tags, unlike someone ;)
Jano
10:22 a.m.
New subject: [libvirt] [PATCH 3/4] util: Introduce virNumaNodesetToCPUset()
This helper converts a set of NUMA node to the set of CPUs
they contain.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virnuma.c | 55 ++++++++++++++++++++++++++++++++++++++++
src/util/virnuma.h | 2 ++
3 files changed, 58 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 678d0348a2..3553545504 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2556,6 +2556,7 @@ virNumaGetPages;
virNumaIsAvailable;
virNumaNodeIsAvailable;
virNumaNodesetIsAvailable;
+virNumaNodesetToCPUset;
virNumaSetPagePoolSize;
virNumaSetupMemoryPolicy;
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index dd3fb7519e..9f9f149f0a 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -297,6 +297,49 @@ virNumaGetNodeCPUs(int node,
# undef MASK_CPU_ISSET
# undef n_bits
+/**
+ * virNumaNodesetToCPUset:
+ * @nodeset: bitmap containing a set of NUMA nodes
+ * @cpuset: return location for a bitmap containing a set of CPUs
+ *
+ * Convert a set of NUMA node to the set of CPUs they contain.
+ *
+ * Returns 0 on success, <0 on failure.
+ */
+int
+virNumaNodesetToCPUset(virBitmapPtr nodeset,
+ virBitmapPtr *cpuset)
+{
+ VIR_AUTOPTR(virBitmap) allNodesCPUs = NULL;
+ size_t nodesetSize;
+ size_t i;
+
+ *cpuset = NULL;
+
+ if (!nodeset)
+ return 0;
+
+ allNodesCPUs = virBitmapNewEmpty();
+ nodesetSize = virBitmapSize(nodeset);
+
+ for (i = 0; i < nodesetSize; i++) {
+ VIR_AUTOPTR(virBitmap) nodeCPUs = NULL;
+
+ if (!virBitmapIsBitSet(nodeset, i))
+ continue;
+
+ if (virNumaGetNodeCPUs(i, &nodeCPUs) < 0)
+ return -1;
+
+ if (virBitmapUnion(allNodesCPUs, nodeCPUs) < 0)
+ return -1;
+ }
+
+ VIR_STEAL_PTR(*cpuset, allNodesCPUs);
+
+ return 0;
+}
+
#else /* !WITH_NUMACTL */
int
@@ -351,6 +394,18 @@ virNumaGetNodeCPUs(int node ATTRIBUTE_UNUSED,
_("NUMA isn't available on this host"));
return -1;
}
+
+int
+virNumaNodesetToCPUset(virBitmapPtr nodeset,
+ virBitmapPtr *cpuset)
+{
+ *cpuset = NULL;
+
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("NUMA isn't available on this host"));
+ return -1;
+}
+
#endif /* !WITH_NUMACTL */
/**
diff --git a/src/util/virnuma.h b/src/util/virnuma.h
index cd7a3ec11f..8e95011dfa 100644
--- a/src/util/virnuma.h
+++ b/src/util/virnuma.h
@@ -48,6 +48,8 @@ int virNumaGetNodeMemory(int node,
unsigned int virNumaGetMaxCPUs(void);
int virNumaGetNodeCPUs(int node, virBitmapPtr *cpus) ATTRIBUTE_NOINLINE;
+int virNumaNodesetToCPUset(virBitmapPtr nodeset,
+ virBitmapPtr *cpuset);
int virNumaGetPageInfo(int node,
unsigned int page_size,
--
2.21.0
7:13 a.m.
New subject: [libvirt] [PATCH 3/4] util: Introduce virNumaNodesetToCPUset()
On Fri, May 31, 2019 at 05:22:01PM +0200, Andrea Bolognani wrote:
This helper converts a set of NUMA node to the set of CPUs
they contain.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virnuma.c | 55 ++++++++++++++++++++++++++++++++++++++++
src/util/virnuma.h | 2 ++
3 files changed, 58 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 678d0348a2..3553545504 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2556,6 +2556,7 @@ virNumaGetPages;
virNumaIsAvailable;
virNumaNodeIsAvailable;
virNumaNodesetIsAvailable;
+virNumaNodesetToCPUset;
virNumaSetPagePoolSize;
virNumaSetupMemoryPolicy;
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index dd3fb7519e..9f9f149f0a 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -297,6 +297,49 @@ virNumaGetNodeCPUs(int node,
# undef MASK_CPU_ISSET
# undef n_bits
+/**
+ * virNumaNodesetToCPUset:
+ * @nodeset: bitmap containing a set of NUMA nodes
+ * @cpuset: return location for a bitmap containing a set of CPUs
+ *
+ * Convert a set of NUMA node to the set of CPUs they contain.
+ *
+ * Returns 0 on success, <0 on failure.
+ */
+int
+virNumaNodesetToCPUset(virBitmapPtr nodeset,
+ virBitmapPtr *cpuset)
I'd prefer the output argument to be first - see VIR_STRDUP.
But I have no stats about which style is prevailing.
+{
+ VIR_AUTOPTR(virBitmap) allNodesCPUs = NULL;
+ size_t nodesetSize;
+ size_t i;
+
+ *cpuset = NULL;
+
+ if (!nodeset)
+ return 0;
+
+ allNodesCPUs = virBitmapNewEmpty();
+ nodesetSize = virBitmapSize(nodeset);
+
+ for (i = 0; i < nodesetSize; i++) {
+ VIR_AUTOPTR(virBitmap) nodeCPUs = NULL;
+
+ if (!virBitmapIsBitSet(nodeset, i))
+ continue;
+
+ if (virNumaGetNodeCPUs(i, &nodeCPUs) < 0)
+ return -1;
+
+ if (virBitmapUnion(allNodesCPUs, nodeCPUs) < 0)
+ return -1;
+ }
+
+ VIR_STEAL_PTR(*cpuset, allNodesCPUs);
+
+ return 0;
+}
+
#else /* !WITH_NUMACTL */
int
@@ -351,6 +394,18 @@ virNumaGetNodeCPUs(int node ATTRIBUTE_UNUSED,
_("NUMA isn't available on this host"));
return -1;
}
+
+int
+virNumaNodesetToCPUset(virBitmapPtr nodeset,
+ virBitmapPtr *cpuset)
+{
util/virnuma.c:399:37: error: unused parameter 'nodeset'
[-Werror,-Wunused-parameter]
virNumaNodesetToCPUset(virBitmapPtr nodeset,
^
1 error generated.
+ *cpuset = NULL;
+
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("NUMA isn't available on this host"));
+ return -1;
+}
+
#endif /* !WITH_NUMACTL */
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
9:45 a.m.
New subject: [libvirt] [PATCH 3/4] util: Introduce virNumaNodesetToCPUset()
On Mon, 2019-06-03 at 14:13 +0200, Ján Tomko wrote:
On Fri, May 31, 2019 at 05:22:01PM +0200, Andrea Bolognani wrote:
> +int
> +virNumaNodesetToCPUset(virBitmapPtr nodeset,
> + virBitmapPtr *cpuset)
I'd prefer the output argument to be first - see VIR_STRDUP.
But I have no stats about which style is prevailing.
The name of the function is virNumaNodesetToCPUset(), so having the
cpuset argument first and the nodeset argument second would be very
confusing :)
> +int
> +virNumaNodesetToCPUset(virBitmapPtr nodeset,
> + virBitmapPtr *cpuset)
> +{
util/virnuma.c:399:37: error: unused parameter 'nodeset'
[-Werror,-Wunused-parameter]
virNumaNodesetToCPUset(virBitmapPtr nodeset,
^
1 error generated.
Good catch, compiler! Fixed it locally.
--
Andrea Bolognani / Red Hat / Virtualization
10:22 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Fix qemuProcessInitCpuAffinity()
Commit f136b83139c6 made some changes in the way we set CPU
affinity for QEMU processes, and while doing so unfortunately
also introduced a logic bug in that it tried to use a NUMA node
map where a CPU map was expected.
Because of that, guests using <numatune> would suddenly fail to
start:
# virsh start guest
error: Failed to start domain guest
error: cannot set CPU affinity on process 40055: Invalid argument
This was particularly easy to trigger on POWER 8 machines,
where secondary threads always show up as offline in the host:
having
<numatune>
<memory mode='strict' placement='static' nodeset='1'/>
</numatune>
in the guest configuration, for example, would result in libvirt
trying to set the process affinity so that it would prefer
running on CPU 1, but since that's a secondary thread and thus
shows up as offline, the operation would fail, and so would
starting the guest.
Use the newly introduced virNumaNodesetToCPUset() to convert
the NUMA node map to a CPU map, which in the example above would
be 8-15 with CPU 8 showing up as online in the guest.
https://bugzilla.redhat.com/show_bug.cgi?id=1703661
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_process.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index fa5909e9be..295ad2e1ff 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2489,11 +2489,16 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm)
if (virDomainNumaGetNodeCount(vm->def->numa) <= 1 &&
virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 &&
mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
+ virBitmapPtr nodeset = NULL;
+
if (virDomainNumatuneMaybeGetNodeset(vm->def->numa,
priv->autoNodeset,
- &cpumapToSet,
+ &nodeset,
-1) < 0)
goto cleanup;
+
+ if (virNumaNodesetToCPUset(nodeset, &cpumapToSet) < 0)
+ goto cleanup;
} else if (vm->def->cputune.emulatorpin) {
cpumapToSet = vm->def->cputune.emulatorpin;
} else {
--
2.21.0
7:15 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Fix qemuProcessInitCpuAffinity()
On Fri, 2019-05-31 at 17:22 +0200, Andrea Bolognani wrote:
Commit f136b83139c6 made some changes in the way we set CPU
affinity for QEMU processes, and while doing so unfortunately
also introduced a logic bug in that it tried to use a NUMA node
map where a CPU map was expected.
Correction: the logic bug was present even in the original
implementation of the feature, introduced back in 2012 (!) with
commit 0f8e7ae33ace; setting the CPU affinity earlier in the guest
startup process, as we're doing since a few releases back, has
probably just made the mistake easier to spot.
Or maybe nobody was using it on POWER 8 and that's why we never
realized something was wrong :)
--
Andrea Bolognani / Red Hat / Virtualization
7:15 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Fix qemuProcessInitCpuAffinity()
On Fri, May 31, 2019 at 05:22:02PM +0200, Andrea Bolognani wrote:
Commit f136b83139c6 made some changes in the way we set CPU
affinity for QEMU processes, and while doing so unfortunately
also introduced a logic bug in that it tried to use a NUMA node
map where a CPU map was expected.
Because of that, guests using <numatune> would suddenly fail to
start:
# virsh start guest
error: Failed to start domain guest
error: cannot set CPU affinity on process 40055: Invalid argument
This was particularly easy to trigger on POWER 8 machines,
where secondary threads always show up as offline in the host:
having
<numatune>
<memory mode='strict' placement='static' nodeset='1'/>
</numatune>
in the guest configuration, for example, would result in libvirt
trying to set the process affinity so that it would prefer
running on CPU 1, but since that's a secondary thread and thus
shows up as offline, the operation would fail, and so would
starting the guest.
Use the newly introduced virNumaNodesetToCPUset() to convert
the NUMA node map to a CPU map, which in the example above would
be 8-15 with CPU 8 showing up as online in the guest.
https://bugzilla.redhat.com/show_bug.cgi?id=1703661
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_process.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
6:03 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Fix qemuProcessInitCpuAffinity()
On 5/31/19 11:22 AM, Andrea Bolognani wrote:
Commit f136b83139c6 made some changes in the way we set CPU
affinity for QEMU processes, and while doing so unfortunately
also introduced a logic bug in that it tried to use a NUMA node
map where a CPU map was expected.
Because of that, guests using <numatune> would suddenly fail to
start:
# virsh start guest
error: Failed to start domain guest
error: cannot set CPU affinity on process 40055: Invalid argument
This was particularly easy to trigger on POWER 8 machines,
where secondary threads always show up as offline in the host:
having
<numatune>
<memory mode='strict' placement='static' nodeset='1'/>
</numatune>
in the guest configuration, for example, would result in libvirt
trying to set the process affinity so that it would prefer
running on CPU 1, but since that's a secondary thread and thus
shows up as offline, the operation would fail, and so would
starting the guest.
Use the newly introduced virNumaNodesetToCPUset() to convert
the NUMA node map to a CPU map, which in the example above would
be 8-15 with CPU 8 showing up as online in the guest.
https://bugzilla.redhat.com/show_bug.cgi?id=1703661
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_process.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index fa5909e9be..295ad2e1ff 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2489,11 +2489,16 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm)
if (virDomainNumaGetNodeCount(vm->def->numa) <= 1 &&
virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0
&&
mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
+ virBitmapPtr nodeset = NULL;
+
if (virDomainNumatuneMaybeGetNodeset(vm->def->numa,
priv->autoNodeset,
- &cpumapToSet,
+ &nodeset,
-1) < 0)
goto cleanup;
+
+ if (virNumaNodesetToCPUset(nodeset, &cpumapToSet) < 0)
+ goto cleanup;
Coverity complained this morning because virNumaNodesetToCPUset will
allocate something into @cpumapToSet which isn't free'd when this code
jumps to cleanup.
John
} else if (vm->def->cputune.emulatorpin) {
cpumapToSet = vm->def->cputune.emulatorpin;
} else {
7:48 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Fix qemuProcessInitCpuAffinity()
On Tue, 2019-06-04 at 07:03 -0400, John Ferlan wrote:
On 5/31/19 11:22 AM, Andrea Bolognani wrote:
> @@ -2489,11 +2489,16 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm)
> if (virDomainNumaGetNodeCount(vm->def->numa) <= 1 &&
> virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0
&&
> mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
> + virBitmapPtr nodeset = NULL;
> +
> if (virDomainNumatuneMaybeGetNodeset(vm->def->numa,
> priv->autoNodeset,
> - &cpumapToSet,
> + &nodeset,
> -1) < 0)
> goto cleanup;
> +
> + if (virNumaNodesetToCPUset(nodeset, &cpumapToSet) < 0)
> + goto cleanup;
Coverity complained this morning because virNumaNodesetToCPUset will
allocate something into @cpumapToSet which isn't free'd when this code
jumps to cleanup.
Nice catch, Coverity!
I've just posted a fix (plus a bonus cleanup):
https://www.redhat.com/archives/libvir-list/2019-June/msg00076.html
--
Andrea Bolognani / Red Hat / Virtualization
1962
days inactive
1966
days old
16 comments
3 participants
participants (3)
-
Andrea Bolognani -
John Ferlan -
Ján Tomko