[libvirt] [PATCH 2/2] Add support for preallocated memory - xml2argv
Safka, JaroslavX
jaroslavx.safka at intel.com
Thu Jun 23 13:47:05 UTC 2016
If source is file then -object memory-backend-file,id=mem,size=1024M,mem-path=/var/lib/libvirt/qemu -numa node,memdev=mem should be added to the qemu commandline
If allocation is immediate then -mem-prealloc should be added to the qemu commanline.
If access is shared then the share=on parameter should be added to the memory-backend-file
e.g.-object memory-backend-file,id=mem,size=1024M,mem-path=/var/lib/libvirt/qemu,share=on
-----Original Message-----
From: Martin Kletzander [mailto:mkletzan at redhat.com]
Sent: Thursday, June 23, 2016 3:42 PM
To: Safka, JaroslavX <jaroslavx.safka at intel.com>
Cc: libvir-list at redhat.com; Mooney, Sean K <sean.k.mooney at intel.com>; Ptacek, MichalX <michalx.ptacek at intel.com>
Subject: Re: [libvirt] [PATCH 2/2] Add support for preallocated memory - xml2argv
On Thu, Jun 23, 2016 at 01:25:29PM +0100, Jaroslav Safka wrote:
>Add conversion from xml to argv for subelements source,access and
>allocation of <memoryBacking>
>
>This change introduces support for preallocated shared file descriptor
>based memory backing.
>It allows vhost-user to be used without hugepages.
>
How does this show up in the guest?
>Configured by these elements:
><memoryBacking>
> <source type="file|anonymous"/>
> <access Mode="shared|private"/>
> <allocation mode="immediate|ondemand"/> </memoryBacking>
>---
> src/qemu/qemu_command.c | 56 ++++++++++++++++++++++
> src/qemu/qemu_command.h | 4 ++
> .../qemuxml2argv-memorybacking-set.args | 6 ++-
> tests/qemuxml2argvtest.c | 3 ++
> 4 files changed, 68 insertions(+), 1 deletion(-)
>
>diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index
>6944129..321e71f 100644
>--- a/src/qemu/qemu_command.c
>+++ b/src/qemu/qemu_command.c
>@@ -9328,6 +9328,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
> if (qemuBuildNVRAMCommandLine(cmd, def, qemuCaps) < 0)
> goto error;
>
>+ if (qemuBuildMemoryBackendCommandLine(cmd, def, qemuCaps) < 0)
>+ goto error;
>+
So this is not accounted for in any of the memory sizes, right?
Shouldn't this be reflected in some of those virDomainDefGetMemory*() functions? It probably depends on the answer to my previous question.
> if (snapshot)
> virCommandAddArgList(cmd, "-loadvm", snapshot->def->name,
> NULL);
>
>@@ -9592,3 +9595,56 @@ qemuBuildChrDeviceStr(char **deviceStr,
>
> return ret;
> }
>+
>+static char *
>+qemuBuildMemoryBackendFileStr(const virDomainDef *def) {
>+ virBuffer buf = VIR_BUFFER_INITIALIZER;
>+ const char template[] =
>+"memory-backend-file,id=mem,size=1024M,mem-path=/var/lib/libvirt/qemu"
>+;
>+
Wow, this seems highly configurable. How come none of these options needs to be changed? That doesn't seem right.
>+ if (VIR_DOMAIN_MEMORY_ACCESS_SHARED == def->mem.access) {
As you might've noticed in the code, we don't do yoda conditions.
>+ // add ",share=on" to -object memory-backend-file
>+ virBufferAsprintf(&buf, "%s,share=on", template);
>+ } else {
>+ virBufferAsprintf(&buf, "%s", template);
>+ }
>+
The virAsprintf() function should shorten this function a bit.
>+
>+ if (virBufferCheckError(&buf) < 0)
>+ goto error;
>+
>+ return virBufferContentAndReset(&buf);
>+
>+ error:
>+ virBufferFreeAndReset(&buf);
>+ return NULL;
>+}
>+
>+
>+int
>+qemuBuildMemoryBackendCommandLine(virCommandPtr cmd,
>+ const virDomainDef *def,
>+ virQEMUCapsPtr qemuCaps
>+__attribute__((unused))) {
>+ char *optstr = NULL;
>+
>+ if (VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE == def->mem.allocation) {
>+ // add '-mem-prealloc'
>+ virCommandAddArg(cmd, "-mem-prealloc");
>+ }
>+
>+ if (VIR_DOMAIN_MEMORY_SOURCE_FILE == def->mem.source) {
>+ optstr = qemuBuildMemoryBackendFileStr(def);
>+ if (optstr) {
>+ virCommandAddArg(cmd, "-object");
>+ virCommandAddArg(cmd, optstr);
>+ VIR_FREE(optstr);
>+ }
>+
>+ // add '-object memory-backend-file,id=mem,size=1024M,mem-path=/var/lib/libvirt/qemu'
>+ // add '-numa node,memdev=mem'
>+ virCommandAddArgList(cmd, "-numa", "node,memdev=mem", NULL);
This looks like it duplicates some of the code that is there already.
Couldn't this be handled more cleanly? Could there be only part of that memory shared and not all of it?
>+ }
>+
>+ return 0;
>+}
>diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index
>9ff4edb..f95d0ea 100644
>--- a/src/qemu/qemu_command.h
>+++ b/src/qemu/qemu_command.h
>@@ -175,5 +175,9 @@ bool qemuCheckCCWS390AddressSupport(const virDomainDef *def,
> virDomainDeviceInfo info,
> virQEMUCapsPtr qemuCaps,
> const char *devicename);
>+int
>+qemuBuildMemoryBackendCommandLine(virCommandPtr cmd,
>+ const virDomainDef *def,
>+ virQEMUCapsPtr qemuCaps);
>
Not aligned.
> #endif /* __QEMU_COMMAND_H__*/
>diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.args
>b/tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.args
>index bb702dc..626fb21 100644
>--- a/tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.args
>+++ b/tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.args
>@@ -19,4 +19,8 @@ QEMU_AUDIO_DRV=none \ -usb \ -drive
>file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
>-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
>--device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
>+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \
>+-mem-prealloc \ -object
>+memory-backend-file,id=mem,size=1024M,mem-path=/var/lib/libvirt/qemu,\
So the only difference to setting this up with hugepages is that the mem-path is not set to hugetlbfs mountpoint?
>+share=on \
>+-numa node,memdev=mem
And it automatically implies numa node? If the numa node is there automatically (which could be), why not just using the:
<cpu>
<numa>
<cell ... memAccess='shared'/>?
More info on:
https://libvirt.org/formatdomain.html#elementsCPU
Hope that helps, have a nice day,
Martin
>diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index
>a4b8bf4..401b455 100644
>--- a/tests/qemuxml2argvtest.c
>+++ b/tests/qemuxml2argvtest.c
>@@ -2024,6 +2024,9 @@ mymain(void)
>
> DO_TEST("acpi-table", NONE);
>
>+ DO_TEST("memorybacking-unset", NONE);
>+ DO_TEST("memorybacking-set", NONE);
>+
> qemuTestDriverFree(&driver);
>
> return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
>--
>2.1.0
>
>--------------------------------------------------------------
>Intel Research and Development Ireland Limited Registered in Ireland
>Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
>Registered Number: 308263
>
>
>This e-mail and any attachments may contain confidential material for
>the sole use of the intended recipient(s). Any review or distribution
>by others is strictly prohibited. If you are not the intended
>recipient, please contact the sender and delete all copies.
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.
More information about the libvir-list
mailing list