On Thu, Oct 01, 2015 at 07:50:09PM -0400, John Ferlan wrote:
On 10/01/2015 08:10 AM, Martin Kletzander wrote: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^^^^^^ Different body part altogether ;-)pagesize, neither memory node pinning. This causes migrations to fail^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Doesn't read well - as in I'm not quite sure what you meant... Neither and nor are usually paired together if that helps any
I edited that few times and messed that up during those edits probably. I did s/, neither/ or/ so it reads more cleanly.
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 <mklet...@redhat.com> --- 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; - } -Wasn't clear to me why this was removed. The code that follows within the if will create a memory-backend-file
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.
I see below the check is made in the else case but there's no hugepage the list of !x && !y && !z ...
That's exactly aligned with the change I'm doing here. You don't need memory-backend-file just because hugepage backing is requested.
Perhaps I'm being thrown by the use of "pagesize" and "hugepage" conditions. The 'hugepage' only gets set if 'pagesize = 0'... If 'hugepage' is set, can pagesize be '0' (outside if pagesize == system_page_size) Sorry - just trying to think through the logic...
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.
I guess removing it is fine, but it's not obvious without digging in a bit...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) {hmmm. force isn't documented - in the input args... I know different problem
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.
+ /* 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;As confusing as the diff is - it looks cleaner in it's final version.
yes, I didn't know how to split any more of this out into separate commits, sorry.
Hopefully Michal or Peter can also look at the final product - it looks good to me though ACK with some adjustments
I'll se if they have anything to say, thanks for the review!
John} *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 \
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
