[libvirt] [PATCH 3/5] qemu: Extract -mem-path building into its own function
Martin Kletzander
mkletzan at redhat.com
Fri Oct 2 09:10:16 UTC 2015
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 at 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
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151002/6bd04966/attachment-0001.sig>
More information about the libvir-list
mailing list