[libvirt] [PATCH 0/5] Fix migration from older libvirt with hugepages
Since libvirt is able to use memory-backend-{ram,file}, we were using it unconditionally, then we realized it might break migration from older libvirt, so we fixed the memory-backend-ram, but forgot about memory-backend-file. This series fixes the file backend as well and uses the memory-backend-file only when it is needed. Fortunately, there is no verison of libvirt that would know about such configuration and, at the same time, not use these new objects, so there is no old libvirt from which the migration would be broken from (due to this particular reason). Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1266856 Martin Kletzander (5): qemu: Move simplification variable to begining of the function qemu: Move memory size detection to the top of the function qemu: Extract -mem-path building into its own function qemu: Add -mem-path even with numa qemu: Use memory-backing-file only when needed src/qemu/qemu_command.c | 166 ++++++++++++--------- .../qemuxml2argv-hugepages-numa.args | 6 +- .../qemuxml2argv-hugepages-pages6.args | 2 +- 3 files changed, 100 insertions(+), 74 deletions(-) -- 2.6.0
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bb1835c2f5f1..f303a4b5bd1e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5052,6 +5052,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, virBitmapPtr nodemask = NULL; int ret = -1; virJSONValuePtr props = NULL; + bool nodeSpecified = virDomainNumatuneNodeSpecified(def->numa, guestNode); *backendProps = NULL; *backendType = NULL; @@ -5204,7 +5205,6 @@ qemuBuildMemoryBackendStr(unsigned long long size, } if (!hugepage && !pagesize) { - bool nodeSpecified = virDomainNumatuneNodeSpecified(def->numa, guestNode); if ((userNodeset || nodeSpecified || force) && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM)) { -- 2.6.0
NIT: $subj: "beginning" On 10/01/2015 08:10 AM, Martin Kletzander wrote:
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK John
To get rid of very long line and make it more readable. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f303a4b5bd1e..492313cbcba9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5249,13 +5249,15 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def, const char *backendType; int ret = -1; int rc; + unsigned long long memsize = virDomainNumaGetNodeMemorySize(def->numa, + cell); *backendStr = NULL; if (virAsprintf(&alias, "ram-node%zu", cell) < 0) goto cleanup; - if ((rc = qemuBuildMemoryBackendStr(virDomainNumaGetNodeMemorySize(def->numa, cell), + if ((rc = qemuBuildMemoryBackendStr(memsize, 0, cell, NULL, auto_nodeset, def, qemuCaps, cfg, -- 2.6.0
On 10/01/2015 08:10 AM, Martin Kletzander wrote:
To get rid of very long line and make it more readable.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
ACK John although you could probably adjust the call to have more args across 3 lines instead of the 5 or so mismatched line lengths now..
On Thu, Oct 01, 2015 at 06:17:33PM -0400, John Ferlan wrote:
On 10/01/2015 08:10 AM, Martin Kletzander wrote:
To get rid of very long line and make it more readable.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
ACK
John
although you could probably adjust the call to have more args across 3 lines instead of the 5 or so mismatched line lengths now..
OK, will do. This patch was just a leftover that I realized I don't need for this series to go in, but it was left in that branch and fixed a coding style issue, so I sent it and forgot to mention this (with a couple of other things) in the cover letter.
That function is called qemuBuildMemPathStr() and will be used in other places in the future. The change in the test suite is proper due to the fact that -mem-prealloc makes only sense with -mem-path (from qemu documentation -- html/qemu-doc.html). Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 120 ++++++++++++--------- .../qemuxml2argv-hugepages-pages6.args | 2 +- 2 files changed, 73 insertions(+), 49 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 492313cbcba9..17e5cfd71702 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7981,6 +7981,71 @@ qemuBuildSmpArgStr(const virDomainDef *def, } static int +qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg, + virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + virCommandPtr cmd) +{ + const long system_page_size = virGetSystemPageSizeKB(); + char *mem_path = NULL; + size_t i = 0; + + /* + * No-op if hugepages were not requested. + */ + if (!def->mem.nhugepages) + return 0; + + /* There is one special case: if user specified "huge" + * pages of regular system pages size. + * And there is nothing to do in this case. + */ + if (def->mem.hugepages[0].size == system_page_size) + return 0; + + if (!cfg->nhugetlbfs) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("hugetlbfs filesystem is not mounted " + "or disabled by administrator config")); + return -1; + } + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MEM_PATH)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("hugepage backing not supported by '%s'"), + def->emulator); + return -1; + } + + if (!def->mem.hugepages[0].size) { + if (!(mem_path = qemuGetDefaultHugepath(cfg->hugetlbfs, + cfg->nhugetlbfs))) + return -1; + } else { + for (i = 0; i < cfg->nhugetlbfs; i++) { + if (cfg->hugetlbfs[i].size == def->mem.hugepages[0].size) + break; + } + + if (i == cfg->nhugetlbfs) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find any usable hugetlbfs " + "mount for %llu KiB"), + def->mem.hugepages[0].size); + return -1; + } + + if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i]))) + return -1; + } + + virCommandAddArgList(cmd, "-mem-prealloc", "-mem-path", mem_path, NULL); + VIR_FREE(mem_path); + + return 0; +} + +static int qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, virDomainDefPtr def, virCommandPtr cmd, @@ -9354,54 +9419,13 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArgFormat(cmd, "%llu", virDomainDefGetMemoryInitial(def) / 1024); } - if (def->mem.nhugepages && !virDomainNumaGetNodeCount(def->numa)) { - const long system_page_size = virGetSystemPageSizeKB(); - char *mem_path = NULL; - - if (def->mem.hugepages[0].size == system_page_size) { - /* There is one special case: if user specified "huge" - * pages of regular system pages size. */ - } else { - if (!cfg->nhugetlbfs) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("hugetlbfs filesystem is not mounted " - "or disabled by administrator config")); - goto error; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MEM_PATH)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("hugepage backing not supported by '%s'"), - def->emulator); - goto error; - } - - if (def->mem.hugepages[0].size) { - for (j = 0; j < cfg->nhugetlbfs; j++) { - if (cfg->hugetlbfs[j].size == def->mem.hugepages[0].size) - break; - } - - if (j == cfg->nhugetlbfs) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to find any usable hugetlbfs mount for %llu KiB"), - def->mem.hugepages[0].size); - goto error; - } - - if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[j]))) - goto error; - } else { - if (!(mem_path = qemuGetDefaultHugepath(cfg->hugetlbfs, - cfg->nhugetlbfs))) - goto error; - } - } - - virCommandAddArg(cmd, "-mem-prealloc"); - if (mem_path) - virCommandAddArgList(cmd, "-mem-path", mem_path, NULL); - VIR_FREE(mem_path); - } + /* + * Add '-mem-path' parameter here only if there is no numa node + * specified. + */ + if (!virDomainNumaGetNodeCount(def->numa) && + qemuBuildMemPathStr(cfg, def, qemuCaps, cmd) < 0) + goto error; if (def->mem.locked && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_MLOCK)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args index 4eccb86e95ae..a3a4e5732622 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args @@ -1,4 +1,4 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -/usr/bin/qemu -S -M pc -m 1024 -mem-prealloc -smp 2 -nographic \ +/usr/bin/qemu -S -M pc -m 1024 -smp 2 -nographic \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -- 2.6.0
On 10/01/2015 08:10 AM, Martin Kletzander wrote:
That function is called qemuBuildMemPathStr() and will be used in other places in the future. The change in the test suite is proper due to the fact that -mem-prealloc makes only sense with -mem-path (from qemu documentation -- html/qemu-doc.html).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 120 ++++++++++++--------- .../qemuxml2argv-hugepages-pages6.args | 2 +- 2 files changed, 73 insertions(+), 49 deletions(-)
Not quite a straight refactor, but it seems OK - just a couple nit/comments below. Your call on how/if there's
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 492313cbcba9..17e5cfd71702 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7981,6 +7981,71 @@ qemuBuildSmpArgStr(const virDomainDef *def, }
static int +qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg, + virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + virCommandPtr cmd) +{ + const long system_page_size = virGetSystemPageSizeKB();
If cpu cycles are important... could save a few by fetching this after the No-op if hugepages not requested... or refactor a bit to pass it around since both qemuBuildNumaArgStr and this fetches the same value. Not required, but just noted... There was something long ago in my history where sysconf() calls were noted as "expensive" - not sure what triggered that repressed memory.
+ char *mem_path = NULL; + size_t i = 0; + + /* + * No-op if hugepages were not requested. + */ + if (!def->mem.nhugepages) + return 0; + + /* There is one special case: if user specified "huge" + * pages of regular system pages size. + * And there is nothing to do in this case. + */ + if (def->mem.hugepages[0].size == system_page_size) + return 0;
Just clarifying - in this case you wouldn't need to pass -mem-prealloc in this case.
+ + if (!cfg->nhugetlbfs) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("hugetlbfs filesystem is not mounted " + "or disabled by administrator config")); + return -1; + } + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MEM_PATH)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("hugepage backing not supported by '%s'"), + def->emulator); + return -1; + } + + if (!def->mem.hugepages[0].size) { + if (!(mem_path = qemuGetDefaultHugepath(cfg->hugetlbfs, + cfg->nhugetlbfs))) + return -1; + } else { + for (i = 0; i < cfg->nhugetlbfs; i++) { + if (cfg->hugetlbfs[i].size == def->mem.hugepages[0].size) + break; + } + + if (i == cfg->nhugetlbfs) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find any usable hugetlbfs " + "mount for %llu KiB"), + def->mem.hugepages[0].size); + return -1; + } + + if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i]))) + return -1; + } + + virCommandAddArgList(cmd, "-mem-prealloc", "-mem-path", mem_path, NULL); + VIR_FREE(mem_path); + + return 0; +} + +static int qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, virDomainDefPtr def, virCommandPtr cmd, @@ -9354,54 +9419,13 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArgFormat(cmd, "%llu", virDomainDefGetMemoryInitial(def) / 1024); }
- if (def->mem.nhugepages && !virDomainNumaGetNodeCount(def->numa)) { - const long system_page_size = virGetSystemPageSizeKB(); - char *mem_path = NULL; - - if (def->mem.hugepages[0].size == system_page_size) { - /* There is one special case: if user specified "huge" - * pages of regular system pages size. */ - } else { - if (!cfg->nhugetlbfs) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("hugetlbfs filesystem is not mounted " - "or disabled by administrator config")); - goto error; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MEM_PATH)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("hugepage backing not supported by '%s'"), - def->emulator); - goto error; - } - - if (def->mem.hugepages[0].size) { - for (j = 0; j < cfg->nhugetlbfs; j++) { - if (cfg->hugetlbfs[j].size == def->mem.hugepages[0].size) - break; - } - - if (j == cfg->nhugetlbfs) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to find any usable hugetlbfs mount for %llu KiB"), - def->mem.hugepages[0].size); - goto error; - } - - if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[j]))) - goto error; - } else { - if (!(mem_path = qemuGetDefaultHugepath(cfg->hugetlbfs, - cfg->nhugetlbfs))) - goto error; - } - } - - virCommandAddArg(cmd, "-mem-prealloc"); - if (mem_path) - virCommandAddArgList(cmd, "-mem-path", mem_path, NULL); - VIR_FREE(mem_path); - } + /* + * Add '-mem-path' parameter here only if there is no numa node + * specified.
Technically adding both -mem-prealloc and -mem-path ;-)
+ */ + if (!virDomainNumaGetNodeCount(def->numa) && + qemuBuildMemPathStr(cfg, def, qemuCaps, cmd) < 0) + goto error;
if (def->mem.locked && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_MLOCK)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args index 4eccb86e95ae..a3a4e5732622 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args @@ -1,4 +1,4 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -/usr/bin/qemu -S -M pc -m 1024 -mem-prealloc -smp 2 -nographic \ +/usr/bin/qemu -S -M pc -m 1024 -smp 2 -nographic \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none
On Thu, Oct 01, 2015 at 06:35:44PM -0400, John Ferlan wrote:
On 10/01/2015 08:10 AM, Martin Kletzander wrote:
That function is called qemuBuildMemPathStr() and will be used in other places in the future. The change in the test suite is proper due to the fact that -mem-prealloc makes only sense with -mem-path (from qemu documentation -- html/qemu-doc.html).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 120 ++++++++++++--------- .../qemuxml2argv-hugepages-pages6.args | 2 +- 2 files changed, 73 insertions(+), 49 deletions(-)
Not quite a straight refactor, but it seems OK - just a couple nit/comments below. Your call on how/if there's
The sentence seems unfinished, and I'm not sure whether I got it correctly =)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 492313cbcba9..17e5cfd71702 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7981,6 +7981,71 @@ qemuBuildSmpArgStr(const virDomainDef *def, }
static int +qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg, + virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + virCommandPtr cmd) +{ + const long system_page_size = virGetSystemPageSizeKB();
If cpu cycles are important... could save a few by fetching this after the No-op if hugepages not requested... or refactor a bit to pass it around since both qemuBuildNumaArgStr and this fetches the same value.
Not required, but just noted... There was something long ago in my history where sysconf() calls were noted as "expensive" - not sure what triggered that repressed memory.
Um... Yes and no. Firstly, this, compared to the number of things we do without optimizing the code is, I would say, is just a drop in an ocean. Plus when taking into account the number of times this gets called (once per starting a domain), I don't think it's _that_ expensive. Secondly, I believe the optimization done by compiler itself will handle this properly, without even trying to debug the binary with -O2 to confirm.
+ char *mem_path = NULL; + size_t i = 0; + + /* + * No-op if hugepages were not requested. + */ + if (!def->mem.nhugepages) + return 0; + + /* There is one special case: if user specified "huge" + * pages of regular system pages size. + * And there is nothing to do in this case. + */ + if (def->mem.hugepages[0].size == system_page_size) + return 0;
Just clarifying - in this case you wouldn't need to pass -mem-prealloc in this case.
Yes, that's the thing that gets fixed by this patch as mentioned in the commit message and that's also why the test needs fixing below.
+ + if (!cfg->nhugetlbfs) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("hugetlbfs filesystem is not mounted " + "or disabled by administrator config")); + return -1; + } + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MEM_PATH)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("hugepage backing not supported by '%s'"), + def->emulator); + return -1; + } + + if (!def->mem.hugepages[0].size) { + if (!(mem_path = qemuGetDefaultHugepath(cfg->hugetlbfs, + cfg->nhugetlbfs))) + return -1; + } else { + for (i = 0; i < cfg->nhugetlbfs; i++) { + if (cfg->hugetlbfs[i].size == def->mem.hugepages[0].size) + break; + } + + if (i == cfg->nhugetlbfs) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find any usable hugetlbfs " + "mount for %llu KiB"), + def->mem.hugepages[0].size); + return -1; + } + + if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i]))) + return -1; + } + + virCommandAddArgList(cmd, "-mem-prealloc", "-mem-path", mem_path, NULL); + VIR_FREE(mem_path); + + return 0; +} + +static int qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, virDomainDefPtr def, virCommandPtr cmd, @@ -9354,54 +9419,13 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArgFormat(cmd, "%llu", virDomainDefGetMemoryInitial(def) / 1024); }
- if (def->mem.nhugepages && !virDomainNumaGetNodeCount(def->numa)) { - const long system_page_size = virGetSystemPageSizeKB(); - char *mem_path = NULL; - - if (def->mem.hugepages[0].size == system_page_size) { - /* There is one special case: if user specified "huge" - * pages of regular system pages size. */ - } else { - if (!cfg->nhugetlbfs) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("hugetlbfs filesystem is not mounted " - "or disabled by administrator config")); - goto error; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MEM_PATH)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("hugepage backing not supported by '%s'"), - def->emulator); - goto error; - } - - if (def->mem.hugepages[0].size) { - for (j = 0; j < cfg->nhugetlbfs; j++) { - if (cfg->hugetlbfs[j].size == def->mem.hugepages[0].size) - break; - } - - if (j == cfg->nhugetlbfs) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to find any usable hugetlbfs mount for %llu KiB"), - def->mem.hugepages[0].size); - goto error; - } - - if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[j]))) - goto error; - } else { - if (!(mem_path = qemuGetDefaultHugepath(cfg->hugetlbfs, - cfg->nhugetlbfs))) - goto error; - } - } - - virCommandAddArg(cmd, "-mem-prealloc"); - if (mem_path) - virCommandAddArgList(cmd, "-mem-path", mem_path, NULL); - VIR_FREE(mem_path); - } + /* + * Add '-mem-path' parameter here only if there is no numa node + * specified.
Technically adding both -mem-prealloc and -mem-path ;-)
Oh, right, I'll fix that :)
+ */ + if (!virDomainNumaGetNodeCount(def->numa) && + qemuBuildMemPathStr(cfg, def, qemuCaps, cmd) < 0) + goto error;
if (def->mem.locked && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_MLOCK)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args index 4eccb86e95ae..a3a4e5732622 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args @@ -1,4 +1,4 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -/usr/bin/qemu -S -M pc -m 1024 -mem-prealloc -smp 2 -nographic \ +/usr/bin/qemu -S -M pc -m 1024 -smp 2 -nographic \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none
Of course this will be used only in case we don't need the memory-backend-file backend, so it should not fire until later. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 17e5cfd71702..321a5e931350 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8132,6 +8132,10 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, } } + if (!needBackend && + qemuBuildMemPathStr(cfg, def, qemuCaps, cmd) < 0) + goto cleanup; + for (i = 0; i < ncells; i++) { VIR_FREE(cpumask); if (!(cpumask = virBitmapFormat(virDomainNumaGetNodeCpumask(def->numa, i)))) -- 2.6.0
On 10/01/2015 08:10 AM, Martin Kletzander wrote:
Of course this will be used only in case we don't need the memory-backend-file backend, so it should not fire until later.
So if I'm reading things right... when building the numa string, if it's determine that "-object" and ",memdev=ram-node%zu" will be required, then it's also required to have "-mem-alloc -mem-path ..." (furrowing through the qemu docs was more difficult than I thought)
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 4 ++++ 1 file changed, 4 insertions(+)
Perhaps "outside" the domain of these changes; however, I note that the call to qemuBuildNumaArgStr is from qemuBuildCommandLine as follows: if (virDomainNumaGetNodeCount(def->numa)) { if (qemuBuildNumaArgStr(cfg, def, cmd, qemuCaps, nodeset) < 0) one of the first things done in qemuBuildNumaArgStr: size_t ncells = virDomainNumaGetNodeCount(def->numa); oh and of course the pesky const long system_page_size = virGetSystemPageSizeKB(); I mentioned previously BTW: Not noted by Coverity, but interestingly later in the code there's: if (ncells) { /* Fortunately, we allow only guest NUMA nodes to be continuous * starting from zero. */ pos = ncells - 1; } Which if I'm reading things correct - cannot happen. Just a note - you don't have to change/fix this, but well if you want to... ACK to what's here as it seems reasonable - perhaps a better reason in the commit message would help John
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 17e5cfd71702..321a5e931350 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8132,6 +8132,10 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, } }
+ if (!needBackend && + qemuBuildMemPathStr(cfg, def, qemuCaps, cmd) < 0) + goto cleanup; + for (i = 0; i < ncells; i++) { VIR_FREE(cpumask); if (!(cpumask = virBitmapFormat(virDomainNumaGetNodeCpumask(def->numa, i))))
On Thu, Oct 01, 2015 at 07:01:17PM -0400, John Ferlan wrote:
On 10/01/2015 08:10 AM, Martin Kletzander wrote:
Of course this will be used only in case we don't need the memory-backend-file backend, so it should not fire until later.
So if I'm reading things right... when building the numa string, if it's determine that "-object" and ",memdev=ram-node%zu" will be required, then it's also required to have "-mem-alloc -mem-path ..."
(furrowing through the qemu docs was more difficult than I thought)
This was harder for me to grasp as well, I'll try to explain. If you use -object memory-backend-file, that object itself has a path where to use hugepages from. If you instead use '-numa ...,mem=SIZE', then there is no way to say where from can this particular node take the hugepages, but you can use '-mem-prealloc -mem-path /path/to/hugetlbfs/mount' to specify it for the whole domain. So you need '-mem-path' if hugepages backing is requested and there is no need for -object memory-backend-file'. I hope that's understandable, let me know if I should try explaining it more clearly.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 4 ++++ 1 file changed, 4 insertions(+)
Perhaps "outside" the domain of these changes; however, I note that the call to qemuBuildNumaArgStr is from qemuBuildCommandLine as follows:
if (virDomainNumaGetNodeCount(def->numa)) { if (qemuBuildNumaArgStr(cfg, def, cmd, qemuCaps, nodeset) < 0)
one of the first things done in qemuBuildNumaArgStr:
size_t ncells = virDomainNumaGetNodeCount(def->numa);
oh and of course the pesky
const long system_page_size = virGetSystemPageSizeKB();
I mentioned previously
BTW: Not noted by Coverity, but interestingly later in the code there's:
if (ncells) { /* Fortunately, we allow only guest NUMA nodes to be continuous * starting from zero. */ pos = ncells - 1; }
Which if I'm reading things correct - cannot happen.
I haven't noticed this, but the way I'm reading it is that qemuBuildNumaArgStr() is called only if there are some numa nodes, so this will happen *every* time qemuBuildNumaArgStr().
Just a note - you don't have to change/fix this, but well if you want to...
ACK to what's here as it seems reasonable - perhaps a better reason in the commit message would help
I'll update the commit message, reading it after myself now I don't understand it either :)
John
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 17e5cfd71702..321a5e931350 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8132,6 +8132,10 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, } }
+ if (!needBackend && + qemuBuildMemPathStr(cfg, def, qemuCaps, cmd) < 0) + goto cleanup; + for (i = 0; i < ncells; i++) { VIR_FREE(cpumask); if (!(cpumask = virBitmapFormat(virDomainNumaGetNodeCpumask(def->numa, i))))
We are using memory-backing-file even when it's not needed, for example if user requests hugepages for mamory backing, but does not specify any pagesize, neither memory node pinning. This causes migrations to fail when migrating from older libvirt that did not do this. So similarly to commit 7832fac84741d65e851dbdbfaf474785cbfdcf3c which does it for memory-backend-ram, this commit makes is more generic and backend-agnostic, so the backend is not used if there is no specific pagesize of hugepages requested, no nodeset the memory node should be bound to, no memory access change required, and so on. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1266856 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 36 ++++++++++------------ .../qemuxml2argv-hugepages-numa.args | 6 ++-- 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 321a5e931350..7ff3e535a543 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5120,12 +5120,6 @@ qemuBuildMemoryBackendStr(unsigned long long size, } if (pagesize || hugepage) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("this qemu doesn't support hugepage memory backing")); - goto cleanup; - } - if (pagesize) { /* Now lets see, if the huge page we want to use is even mounted * and ready to use */ @@ -5204,29 +5198,31 @@ qemuBuildMemoryBackendStr(unsigned long long size, goto cleanup; } - if (!hugepage && !pagesize) { - - if ((userNodeset || nodeSpecified || force) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM)) { + /* If none of the following is requested... */ + if (!pagesize && !userNodeset && !memAccess && !nodeSpecified && !force) { + /* report back that using the new backend is not necessary + * to achieve the desired configuration */ + ret = 1; + } else { + /* otherwise check the required capability */ + if (STREQ(*backendType, "memory-backend-file") && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("this qemu doesn't support the " - "memory-backend-ram object")); + "memory-backend-file object")); goto cleanup; + } else if (STREQ(*backendType, "memory-backend-ram") && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support the " + "memory-backend-ram object")); } - /* report back that using the new backend is not necessary to achieve - * the desired configuration */ - if (!userNodeset && !nodeSpecified) { - *backendProps = props; - props = NULL; - ret = 1; - goto cleanup; - } + ret = 0; } *backendProps = props; props = NULL; - ret = 0; cleanup: virJSONValueFree(props); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args index 37511b109b6e..3496cf1a732d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args @@ -4,9 +4,9 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=spice \ -M pc-i440fx-2.3 \ -m size=1048576k,slots=16,maxmem=1099511627776k \ -smp 2 \ --object memory-backend-file,id=ram-node0,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=1073741824 \ --numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \ +-mem-prealloc \ +-mem-path /dev/hugepages2M/libvirt/qemu \ +-numa node,nodeid=0,cpus=0-1,mem=1024 \ -object memory-backend-file,id=memdimm0,prealloc=yes,\ mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=1-3,policy=bind \ -device pc-dimm,node=0,memdev=memdimm0,id=dimm0 \ -- 2.6.0
On 10/01/2015 08:10 AM, Martin Kletzander wrote:
We are using memory-backing-file even when it's not needed, for example if user requests hugepages for mamory backing, but does not specify any ^^^^^^ Different body part altogether ;-)
pagesize, neither memory node pinning. This causes migrations to fail
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Doesn't read well - as in I'm not quite sure what you meant... Neither and nor are usually paired together if that helps any
when migrating from older libvirt that did not do this. So similarly to commit 7832fac84741d65e851dbdbfaf474785cbfdcf3c which does it for memory-backend-ram, this commit makes is more generic and backend-agnostic, so the backend is not used if there is no specific pagesize of hugepages requested, no nodeset the memory node should be bound to, no memory access change required, and so on.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1266856
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 36 ++++++++++------------ .../qemuxml2argv-hugepages-numa.args | 6 ++-- 2 files changed, 19 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 321a5e931350..7ff3e535a543 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5120,12 +5120,6 @@ qemuBuildMemoryBackendStr(unsigned long long size, }
if (pagesize || hugepage) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("this qemu doesn't support hugepage memory backing")); - goto cleanup; - } -
Wasn't clear to me why this was removed. The code that follows within the if will create a memory-backend-file I see below the check is made in the else case but there's no hugepage the list of !x && !y && !z ... Perhaps I'm being thrown by the use of "pagesize" and "hugepage" conditions. The 'hugepage' only gets set if 'pagesize = 0'... If 'hugepage' is set, can pagesize be '0' (outside if pagesize == system_page_size) Sorry - just trying to think through the logic... I guess removing it is fine, but it's not obvious without digging in a bit...
if (pagesize) { /* Now lets see, if the huge page we want to use is even mounted * and ready to use */ @@ -5204,29 +5198,31 @@ qemuBuildMemoryBackendStr(unsigned long long size, goto cleanup; }
- if (!hugepage && !pagesize) { - - if ((userNodeset || nodeSpecified || force) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM)) { + /* If none of the following is requested... */ + if (!pagesize && !userNodeset && !memAccess && !nodeSpecified && !force) {
hmmm. force isn't documented - in the input args... I know different problem
+ /* report back that using the new backend is not necessary + * to achieve the desired configuration */ + ret = 1; + } else { + /* otherwise check the required capability */ + if (STREQ(*backendType, "memory-backend-file") && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("this qemu doesn't support the " - "memory-backend-ram object")); + "memory-backend-file object")); goto cleanup; + } else if (STREQ(*backendType, "memory-backend-ram") && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support the " + "memory-backend-ram object")); }
- /* report back that using the new backend is not necessary to achieve - * the desired configuration */ - if (!userNodeset && !nodeSpecified) { - *backendProps = props; - props = NULL; - ret = 1; - goto cleanup; - } + ret = 0;
As confusing as the diff is - it looks cleaner in it's final version. Hopefully Michal or Peter can also look at the final product - it looks good to me though ACK with some adjustments John
}
*backendProps = props; props = NULL; - ret = 0;
cleanup: virJSONValueFree(props); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args index 37511b109b6e..3496cf1a732d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args @@ -4,9 +4,9 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=spice \ -M pc-i440fx-2.3 \ -m size=1048576k,slots=16,maxmem=1099511627776k \ -smp 2 \ --object memory-backend-file,id=ram-node0,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=1073741824 \ --numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \ +-mem-prealloc \ +-mem-path /dev/hugepages2M/libvirt/qemu \ +-numa node,nodeid=0,cpus=0-1,mem=1024 \ -object memory-backend-file,id=memdimm0,prealloc=yes,\ mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=1-3,policy=bind \ -device pc-dimm,node=0,memdev=memdimm0,id=dimm0 \
On Thu, Oct 01, 2015 at 07:50:09PM -0400, John Ferlan wrote:
On 10/01/2015 08:10 AM, Martin Kletzander wrote:
We are using memory-backing-file even when it's not needed, for example if user requests hugepages for mamory backing, but does not specify any ^^^^^^ Different body part altogether ;-)
pagesize, neither memory node pinning. This causes migrations to fail
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Doesn't read well - as in I'm not quite sure what you meant... Neither and nor are usually paired together if that helps any
I edited that few times and messed that up during those edits probably. I did s/, neither/ or/ so it reads more cleanly.
when migrating from older libvirt that did not do this. So similarly to commit 7832fac84741d65e851dbdbfaf474785cbfdcf3c which does it for memory-backend-ram, this commit makes is more generic and backend-agnostic, so the backend is not used if there is no specific pagesize of hugepages requested, no nodeset the memory node should be bound to, no memory access change required, and so on.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1266856
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 36 ++++++++++------------ .../qemuxml2argv-hugepages-numa.args | 6 ++-- 2 files changed, 19 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 321a5e931350..7ff3e535a543 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5120,12 +5120,6 @@ qemuBuildMemoryBackendStr(unsigned long long size, }
if (pagesize || hugepage) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("this qemu doesn't support hugepage memory backing")); - goto cleanup; - } -
Wasn't clear to me why this was removed. The code that follows within the if will create a memory-backend-file
This code is a hairy ball of **** ***. This function creates the objects, but only after all of them are created we know whether the backend is needed or not (you cannot mix mem= and memdev= in -numa specification). So only after all that is done, the objects are either transformed into '-num' parameter or not. So with the change that I'm doing here it just removes the check simply because we don't know yet whether memory-backend-file object will be needed or not, and the decision is moved downwards.
I see below the check is made in the else case but there's no hugepage the list of !x && !y && !z ...
That's exactly aligned with the change I'm doing here. You don't need memory-backend-file just because hugepage backing is requested.
Perhaps I'm being thrown by the use of "pagesize" and "hugepage" conditions. The 'hugepage' only gets set if 'pagesize = 0'... If 'hugepage' is set, can pagesize be '0' (outside if pagesize == system_page_size)
Sorry - just trying to think through the logic...
No problem, that's the important part to understand what I'm doing here. And I understand this change is definitely not straight-forward, that's why I was trying to make the last commit as small and self-contained as possible.
I guess removing it is fine, but it's not obvious without digging in a bit...
if (pagesize) { /* Now lets see, if the huge page we want to use is even mounted * and ready to use */ @@ -5204,29 +5198,31 @@ qemuBuildMemoryBackendStr(unsigned long long size, goto cleanup; }
- if (!hugepage && !pagesize) { - - if ((userNodeset || nodeSpecified || force) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM)) { + /* If none of the following is requested... */ + if (!pagesize && !userNodeset && !memAccess && !nodeSpecified && !force) {
hmmm. force isn't documented - in the input args... I know different problem
Force means that we must use the memory-backend-{file-ram}. With force == true, this function is called to construct the object for memory device as opposed to numa node memory.
+ /* report back that using the new backend is not necessary + * to achieve the desired configuration */ + ret = 1; + } else { + /* otherwise check the required capability */ + if (STREQ(*backendType, "memory-backend-file") && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("this qemu doesn't support the " - "memory-backend-ram object")); + "memory-backend-file object")); goto cleanup; + } else if (STREQ(*backendType, "memory-backend-ram") && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support the " + "memory-backend-ram object")); }
- /* report back that using the new backend is not necessary to achieve - * the desired configuration */ - if (!userNodeset && !nodeSpecified) { - *backendProps = props; - props = NULL; - ret = 1; - goto cleanup; - } + ret = 0;
As confusing as the diff is - it looks cleaner in it's final version.
yes, I didn't know how to split any more of this out into separate commits, sorry.
Hopefully Michal or Peter can also look at the final product - it looks good to me though
ACK with some adjustments
I'll se if they have anything to say, thanks for the review!
John
}
*backendProps = props; props = NULL; - ret = 0;
cleanup: virJSONValueFree(props); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args index 37511b109b6e..3496cf1a732d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args @@ -4,9 +4,9 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=spice \ -M pc-i440fx-2.3 \ -m size=1048576k,slots=16,maxmem=1099511627776k \ -smp 2 \ --object memory-backend-file,id=ram-node0,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=1073741824 \ --numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \ +-mem-prealloc \ +-mem-path /dev/hugepages2M/libvirt/qemu \ +-numa node,nodeid=0,cpus=0-1,mem=1024 \ -object memory-backend-file,id=memdimm0,prealloc=yes,\ mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=1-3,policy=bind \ -device pc-dimm,node=0,memdev=memdimm0,id=dimm0 \
On 01.10.2015 14:10, Martin Kletzander wrote:
We are using memory-backing-file even when it's not needed, for example if user requests hugepages for mamory backing, but does not specify any pagesize, neither memory node pinning. This causes migrations to fail when migrating from older libvirt that did not do this. So similarly to commit 7832fac84741d65e851dbdbfaf474785cbfdcf3c which does it for memory-backend-ram, this commit makes is more generic and backend-agnostic, so the backend is not used if there is no specific pagesize of hugepages requested, no nodeset the memory node should be bound to, no memory access change required, and so on.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1266856
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 36 ++++++++++------------ .../qemuxml2argv-hugepages-numa.args | 6 ++-- 2 files changed, 19 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 321a5e931350..7ff3e535a543 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5120,12 +5120,6 @@ qemuBuildMemoryBackendStr(unsigned long long size, }
if (pagesize || hugepage) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("this qemu doesn't support hugepage memory backing")); - goto cleanup; - } - if (pagesize) { /* Now lets see, if the huge page we want to use is even mounted * and ready to use */ @@ -5204,29 +5198,31 @@ qemuBuildMemoryBackendStr(unsigned long long size, goto cleanup; }
- if (!hugepage && !pagesize) { - - if ((userNodeset || nodeSpecified || force) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM)) { + /* If none of the following is requested... */ + if (!pagesize && !userNodeset && !memAccess && !nodeSpecified && !force) { + /* report back that using the new backend is not necessary + * to achieve the desired configuration */ + ret = 1; + } else { + /* otherwise check the required capability */ + if (STREQ(*backendType, "memory-backend-file") && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("this qemu doesn't support the " - "memory-backend-ram object")); + "memory-backend-file object")); goto cleanup; + } else if (STREQ(*backendType, "memory-backend-ram") && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support the " + "memory-backend-ram object"));
goto cleanup; /* please */
}
- /* report back that using the new backend is not necessary to achieve - * the desired configuration */ - if (!userNodeset && !nodeSpecified) { - *backendProps = props; - props = NULL; - ret = 1; - goto cleanup; - } + ret = 0; }
*backendProps = props; props = NULL; - ret = 0;
cleanup: virJSONValueFree(props); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args index 37511b109b6e..3496cf1a732d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args @@ -4,9 +4,9 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=spice \ -M pc-i440fx-2.3 \ -m size=1048576k,slots=16,maxmem=1099511627776k \ -smp 2 \ --object memory-backend-file,id=ram-node0,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=1073741824 \ --numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \ +-mem-prealloc \ +-mem-path /dev/hugepages2M/libvirt/qemu \ +-numa node,nodeid=0,cpus=0-1,mem=1024 \ -object memory-backend-file,id=memdimm0,prealloc=yes,\ mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=1-3,policy=bind \ -device pc-dimm,node=0,memdev=memdimm0,id=dimm0 \
Otherwise looking good. ACK series. Michal
participants (3)
-
John Ferlan -
Martin Kletzander -
Michal Privoznik