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@xxxxxxxxxx> --- 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list