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(a)redhat.com&gt; --- 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 Thu, Oct 01, 2015 at 06:35:44PM -0400, John Ferlan wrote: The sentence seems unfinished, and I'm not sure whether I got it correctly =) 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. 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. Oh, right, I'll fix that :)

On 10/01/2015 08:10 AM, Martin Kletzander wrote: 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) 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

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(a)redhat.com&gt; --- 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 Thu, Oct 01, 2015 at 07:50:09PM -0400, John Ferlan wrote: I edited that few times and messed that up during those edits probably. I did s/, neither/ or/ so it reads more cleanly. 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. That's exactly aligned with the change I'm doing here. You don't need memory-backend-file just because hugepage backing is requested. 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. 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. yes, I didn't know how to split any more of this out into separate commits, sorry. I'll se if they have anything to say, thanks for the review!