[libvirt] [PATCH 0/3] Fix memory ABI stability check issues
Note that this series applies only on top of the NUMA config unification series: http://www.redhat.com/archives/libvir-list/2015-February/msg00532.html Please see individual patches for explanation Peter Krempa (3): conf: ABI: Hugepage backing definition is not guest ABI conf: ABI: Memballoon setting is not guest ABI conf: numa: Check ABI stability of NUMA configuration src/conf/domain_conf.c | 31 ++----------------------------- src/conf/numa_conf.c | 37 +++++++++++++++++++++++++++++++++++++ src/conf/numa_conf.h | 3 +++ src/libvirt_private.syms | 1 + 4 files changed, 43 insertions(+), 29 deletions(-) -- 2.2.2
The backing of the vm's memory isn't influencing the guest ABI thus shouldn't be checked. --- src/conf/domain_conf.c | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fe73a5f..9a7060d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15969,30 +15969,6 @@ virDomainDefCheckABIStability(virDomainDefPtr src, dst->mem.cur_balloon, src->mem.cur_balloon); goto error; } - if (src->mem.nhugepages != dst->mem.nhugepages) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain huge pages count %zu does not match source %zu"), - dst->mem.nhugepages, src->mem.nhugepages); - goto error; - } - for (i = 0; i < src->mem.nhugepages; i++) { - virDomainHugePagePtr src_huge = &src->mem.hugepages[i]; - virDomainHugePagePtr dst_huge = &dst->mem.hugepages[i]; - - if (src_huge->size != dst_huge->size) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain huge page size %llu " - "does not match source %llu"), - dst_huge->size, src_huge->size); - goto error; - } - - if (!virBitmapEqual(src_huge->nodemask, dst_huge->nodemask)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Target huge page nodemask does not match source")); - goto error; - } - } if (src->vcpus != dst->vcpus) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, -- 2.2.2
On 02/16/2015 02:50 PM, Peter Krempa wrote:
The backing of the vm's memory isn't influencing the guest ABI thus shouldn't be checked. --- src/conf/domain_conf.c | 24 ------------------------ 1 file changed, 24 deletions(-)
Just to put it "officially there" - ACK... some thoughts below FWIW John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fe73a5f..9a7060d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15969,30 +15969,6 @@ virDomainDefCheckABIStability(virDomainDefPtr src, dst->mem.cur_balloon, src->mem.cur_balloon); goto error; } - if (src->mem.nhugepages != dst->mem.nhugepages) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain huge pages count %zu does not match source %zu"), - dst->mem.nhugepages, src->mem.nhugepages); - goto error; - }
Looks like this hunk was originally added by commit id 'f2fc1eee' when it was one huge chunk... I'm not overly familiar with how hugepages work, but agree that it doesn't seem to cause ABI issues.
- for (i = 0; i < src->mem.nhugepages; i++) { - virDomainHugePagePtr src_huge = &src->mem.hugepages[i]; - virDomainHugePagePtr dst_huge = &dst->mem.hugepages[i]; - - if (src_huge->size != dst_huge->size) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain huge page size %llu " - "does not match source %llu"), - dst_huge->size, src_huge->size); - goto error; - } - - if (!virBitmapEqual(src_huge->nodemask, dst_huge->nodemask)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Target huge page nodemask does not match source")); - goto error; - } - }
Looks like this hunk was added by commit id '136ad497' when the one chunk was split with the extra mask capability. Again, I agree it doesn't appear to cause ABI issues.
if (src->vcpus != dst->vcpus) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
The current balloon setting does not influence the ABI of the guest and the driver should take the change just fine. --- I personally don't think that this check is useful, but I'll happily drop the patch if anybody thinks otherwise. src/conf/domain_conf.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9a7060d..eee01b6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15963,12 +15963,6 @@ virDomainDefCheckABIStability(virDomainDefPtr src, dst->mem.max_balloon, src->mem.max_balloon); goto error; } - if (src->mem.cur_balloon != dst->mem.cur_balloon) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain current memory %lld does not match source %lld"), - dst->mem.cur_balloon, src->mem.cur_balloon); - goto error; - } if (src->vcpus != dst->vcpus) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, -- 2.2.2
On Mon, Feb 16, 2015 at 08:50:43PM +0100, Peter Krempa wrote:
The current balloon setting does not influence the ABI of the guest and the driver should take the change just fine. --- I personally don't think that this check is useful, but I'll happily drop the patch if anybody thinks otherwise.
src/conf/domain_conf.c | 6 ------ 1 file changed, 6 deletions(-)
Yes, you can change the balloon target on the fly, so this isn't something we need to keep as stable across migration. The question though is whether libvirt/QEMU will correctly handle the changed balloon setting during migration. ie does libvirt correctly issue a 'balloon' monitor command on the target host if it has changed in the target XML ? If it does, then I agree this check can go..
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9a7060d..eee01b6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15963,12 +15963,6 @@ virDomainDefCheckABIStability(virDomainDefPtr src, dst->mem.max_balloon, src->mem.max_balloon); goto error; } - if (src->mem.cur_balloon != dst->mem.cur_balloon) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain current memory %lld does not match source %lld"), - dst->mem.cur_balloon, src->mem.cur_balloon); - goto error; - }
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Mon, Feb 16, 2015 at 19:54:23 +0000, Daniel Berrange wrote:
On Mon, Feb 16, 2015 at 08:50:43PM +0100, Peter Krempa wrote:
The current balloon setting does not influence the ABI of the guest and the driver should take the change just fine. --- I personally don't think that this check is useful, but I'll happily drop the patch if anybody thinks otherwise.
src/conf/domain_conf.c | 6 ------ 1 file changed, 6 deletions(-)
Yes, you can change the balloon target on the fly, so this isn't something we need to keep as stable across migration. The question though is whether libvirt/QEMU will correctly handle the changed balloon setting during migration. ie does libvirt correctly issue a 'balloon' monitor command on the target host if it has changed in the target XML ? If it does, then I agree this check can go..
I think qemu should handle this just right as we pass it via the monitor, but I didn't actually try it. I'll hold off this one until I make sure that it actually is okay. Peter
Add helper to compare initial sizes of indivitual NUMA nodes and the map of belonging vCPUs. Other configuration is not ABI. --- src/conf/domain_conf.c | 3 +++ src/conf/numa_conf.c | 37 +++++++++++++++++++++++++++++++++++++ src/conf/numa_conf.h | 3 +++ src/libvirt_private.syms | 1 + 4 files changed, 44 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eee01b6..15d4fe0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15964,6 +15964,9 @@ virDomainDefCheckABIStability(virDomainDefPtr src, goto error; } + if (!virDomainNumaCheckABIStability(src->numa, dst->numa)) + goto error; + if (src->vcpus != dst->vcpus) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain vCPU count %d does not match source %d"), diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 0a6f902..70044d5 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -814,6 +814,43 @@ virDomainNumaNew(void) } +bool +virDomainNumaCheckABIStability(virDomainNumaPtr src, + virDomainNumaPtr tgt) +{ + size_t i; + + if (virDomainNumaGetNodeCount(src) != virDomainNumaGetNodeCount(tgt)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target NUMA node count '%zu' doesn't match " + "source '%zu'"), + virDomainNumaGetNodeCount(tgt), + virDomainNumaGetNodeCount(src)); + return false; + } + + for (i = 0; i < virDomainNumaGetNodeCount(src); i++) { + if (src->mem_nodes[i].mem != tgt->mem_nodes[i].mem) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Size of target NUMA node %zu (%llu) doesn't " + "match source (%llu)"), i, + tgt->mem_nodes[i].mem, src->mem_nodes[i].mem); + return false; + } + + if (!virBitmapEqual(src->mem_nodes[i].cpumask, + tgt->mem_nodes[i].cpumask)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Processor mask of target NUMA node %zu doesn't " + "match source"), i); + return false; + } + } + + return true; +} + + size_t virDomainNumaGetNodeCount(virDomainNumaPtr numa) { diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index 8b3c437..23265af 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -125,6 +125,9 @@ void virDomainNumaSetNodeMemorySize(virDomainNumaPtr numa, bool virDomainNumaEquals(virDomainNumaPtr n1, virDomainNumaPtr n2); +bool virDomainNumaCheckABIStability(virDomainNumaPtr src, + virDomainNumaPtr tgt); + bool virDomainNumatuneHasPlacementAuto(virDomainNumaPtr numatune); bool virDomainNumatuneHasPerNodeBinding(virDomainNumaPtr numatune); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8988b61..c02e089 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -627,6 +627,7 @@ virNodeDeviceObjUnlock; # conf/numa_conf.h +virDomainNumaCheckABIStability; virDomainNumaEquals; virDomainNumaFree; virDomainNumaGetNodeCount; -- 2.2.2
OK - I peeked after looking at 24/24 to see how difficult this series would be (and before soaking my brain cells in liquid refreshment)... The first two seemed OK and it doesn't appear there's anyone else disagreeing... For this one though... On 02/16/2015 02:50 PM, Peter Krempa wrote:
Add helper to compare initial sizes of indivitual NUMA nodes and the map of belonging vCPUs. Other configuration is not ABI. --- src/conf/domain_conf.c | 3 +++ src/conf/numa_conf.c | 37 +++++++++++++++++++++++++++++++++++++ src/conf/numa_conf.h | 3 +++ src/libvirt_private.syms | 1 + 4 files changed, 44 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eee01b6..15d4fe0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15964,6 +15964,9 @@ virDomainDefCheckABIStability(virDomainDefPtr src, goto error; }
+ if (!virDomainNumaCheckABIStability(src->numa, dst->numa)) + goto error; + if (src->vcpus != dst->vcpus) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain vCPU count %d does not match source %d"), diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 0a6f902..70044d5 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -814,6 +814,43 @@ virDomainNumaNew(void) }
+bool +virDomainNumaCheckABIStability(virDomainNumaPtr src, + virDomainNumaPtr tgt) +{ + size_t i; + + if (virDomainNumaGetNodeCount(src) != virDomainNumaGetNodeCount(tgt)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target NUMA node count '%zu' doesn't match " + "source '%zu'"), + virDomainNumaGetNodeCount(tgt), + virDomainNumaGetNodeCount(src)); + return false; + } + + for (i = 0; i < virDomainNumaGetNodeCount(src); i++) { + if (src->mem_nodes[i].mem != tgt->mem_nodes[i].mem) {
Use virDomainNumaGetNodeMemorySize() ?
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Size of target NUMA node %zu (%llu) doesn't " + "match source (%llu)"), i, + tgt->mem_nodes[i].mem, src->mem_nodes[i].mem); + return false; + } + + if (!virBitmapEqual(src->mem_nodes[i].cpumask, + tgt->mem_nodes[i].cpumask)) {
Use virDomainNumaGetNodeCpumask to access each cpumask bitmap ? This seems reasonable to me... ACK with the accessor changes... John
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Processor mask of target NUMA node %zu doesn't " + "match source"), i); + return false; + } + } + + return true; +} + + size_t virDomainNumaGetNodeCount(virDomainNumaPtr numa) { diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index 8b3c437..23265af 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -125,6 +125,9 @@ void virDomainNumaSetNodeMemorySize(virDomainNumaPtr numa, bool virDomainNumaEquals(virDomainNumaPtr n1, virDomainNumaPtr n2);
+bool virDomainNumaCheckABIStability(virDomainNumaPtr src, + virDomainNumaPtr tgt); + bool virDomainNumatuneHasPlacementAuto(virDomainNumaPtr numatune);
bool virDomainNumatuneHasPerNodeBinding(virDomainNumaPtr numatune); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8988b61..c02e089 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -627,6 +627,7 @@ virNodeDeviceObjUnlock;
# conf/numa_conf.h +virDomainNumaCheckABIStability; virDomainNumaEquals; virDomainNumaFree; virDomainNumaGetNodeCount;
On Thu, Feb 19, 2015 at 19:37:33 -0500, John Ferlan wrote:
OK - I peeked after looking at 24/24 to see how difficult this series would be (and before soaking my brain cells in liquid refreshment)...
The first two seemed OK and it doesn't appear there's anyone else disagreeing...
For this one though...
On 02/16/2015 02:50 PM, Peter Krempa wrote:
Add helper to compare initial sizes of indivitual NUMA nodes and the map of belonging vCPUs. Other configuration is not ABI. --- src/conf/domain_conf.c | 3 +++ src/conf/numa_conf.c | 37 +++++++++++++++++++++++++++++++++++++ src/conf/numa_conf.h | 3 +++ src/libvirt_private.syms | 1 + 4 files changed, 44 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eee01b6..15d4fe0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15964,6 +15964,9 @@ virDomainDefCheckABIStability(virDomainDefPtr src, goto error; }
+ if (!virDomainNumaCheckABIStability(src->numa, dst->numa)) + goto error; + if (src->vcpus != dst->vcpus) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain vCPU count %d does not match source %d"), diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 0a6f902..70044d5 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -814,6 +814,43 @@ virDomainNumaNew(void) }
+bool +virDomainNumaCheckABIStability(virDomainNumaPtr src, + virDomainNumaPtr tgt) +{ + size_t i; + + if (virDomainNumaGetNodeCount(src) != virDomainNumaGetNodeCount(tgt)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target NUMA node count '%zu' doesn't match " + "source '%zu'"), + virDomainNumaGetNodeCount(tgt), + virDomainNumaGetNodeCount(src)); + return false; + } + + for (i = 0; i < virDomainNumaGetNodeCount(src); i++) { + if (src->mem_nodes[i].mem != tgt->mem_nodes[i].mem) {
Use virDomainNumaGetNodeMemorySize() ?
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Size of target NUMA node %zu (%llu) doesn't " + "match source (%llu)"), i, + tgt->mem_nodes[i].mem, src->mem_nodes[i].mem); + return false; + } + + if (!virBitmapEqual(src->mem_nodes[i].cpumask, + tgt->mem_nodes[i].cpumask)) {
Use virDomainNumaGetNodeCpumask to access each cpumask bitmap ?
This seems reasonable to me...
ACK with the accessor changes...
I've changed to the accessor-based approach and pushed this one along with patch 1. Thanks. Peter
On Mon, Feb 16, 2015 at 20:50:41 +0100, Peter Krempa wrote:
Note that this series applies only on top of the NUMA config unification series: http://www.redhat.com/archives/libvir-list/2015-February/msg00532.html
Please see individual patches for explanation
Peter Krempa (3): conf: ABI: Hugepage backing definition is not guest ABI conf: ABI: Memballoon setting is not guest ABI conf: numa: Check ABI stability of NUMA configuration
src/conf/domain_conf.c | 31 ++----------------------------- src/conf/numa_conf.c | 37 +++++++++++++++++++++++++++++++++++++ src/conf/numa_conf.h | 3 +++ src/libvirt_private.syms | 1 + 4 files changed, 43 insertions(+), 29 deletions(-)
Rebased version of this series including the depending series is available at: git fetch git://pipo.sk/pipo/libvirt.git/ memory-refactors
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Peter Krempa