[libvirt] [PATCH 5/5] qemu: Use memory-backing-file only when needed
Martin Kletzander
mkletzan at redhat.com
Fri Oct 2 11:01:24 UTC 2015
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 <mkletzan at 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 \
>>
-------------- 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/b024e0e8/attachment-0001.sig>
More information about the libvir-list
mailing list