Re: [PATCH 2/2] Add support for preallocated memory - xml2argv

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Safka, JaroslavX
> Sent: Thursday, June 23, 2016 2:47 PM
> To: Martin Kletzander <mkletzan@xxxxxxxxxx>
> Cc: libvir-list@xxxxxxxxxx; Mooney, Sean K <sean.k.mooney@xxxxxxxxx>;
> Ptacek, MichalX <michalx.ptacek@xxxxxxxxx>
> Subject: RE:  [PATCH 2/2] Add support for preallocated memory - xml2argv
> 
> 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

[Mooney, Sean K]  in the above example the size should  not be hardcoded but instead match the amount of
Memory requested for the vm.  The patch should also be defined by libvirt either as a complie time constant or
As a user specified path not a hardcoded string.

What those parameters actually mean is as follows.

-object memory-backing-file,id=mem,size=xM,path=xyz 
# create a memory-backing-file object called mem of size x megabytes with an open file descriptor to xyz.
# Note that this does not actually create a file just a file descriptor so we will not be write to disk.

-numa node,memdev=mem
# this instruct qemu to use the memory-backing-file descriptor as the memory backing for the guest.
 
-object memory-backend-file,id=mem,size=1024M,mem-
> path=/var/lib/libvirt/qemu -numa node,memdev=mem
> 
> 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@xxxxxxxxxx]
> Sent: Thursday, June 23, 2016 3:42 PM
> To: Safka, JaroslavX <jaroslavx.safka@xxxxxxxxx>
> Cc: libvir-list@xxxxxxxxxx; Mooney, Sean K <sean.k.mooney@xxxxxxxxx>;
> Ptacek, MichalX <michalx.ptacek@xxxxxxxxx>
> Subject: Re:  [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@xxxxxxxxxx
> >https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]