On 04/26/2018 02:56 PM, Ján Tomko wrote:
> On Fri, Apr 20, 2018 at 11:09:31AM +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1480668
>>
>> QEMU has this new feature memory-backend-file.discard-data=yes
>> which is a nifty optimization. Basically, when qemu is quitting
>> or on memory hotplug it calls munmap() and close() on the file
>> that is backing the memory. However, this does not mean kernel
>> won't stop touching that part of memory. It still might. With
>> this feature enabled we tell kernel: "we don't need this memory
>> nor data stored in it". This makes kernel drop the memory
>> immediately without trying to sync memory with the mapped file.
>>
>> Unfortunately, this cannot be turned on by default because we
>> can't be sure when users really don't care about what happens to
>> data after qemu dies. So it has to be opt-in. As usual, there are
>> three places where one can configure memory attributes. This
>> patch adds the feature to all of them.
>>
>
> But only tests one of the three ways.
There are always some test cases missing. I don't think it should be a
show stopper. Do you? Because in that case a lot of patches without
proper tests are slipping in.
> It would be nice to split the conf
> and qemu changes and provide a XML->XML test.
Yay, more patches! Although I don't quite understand why. One patch
without the other doesn't make any sense. Anybody backporting this (=me)
would need to backport yet one more commit.
>
>> Signed-off-by: Michal Privoznik <mpriv...@redhat.com>
>> ---
>> docs/formatdomain.html.in | 34
>> ++++++++++++++++++++++--
>> docs/schemas/cputypes.rng | 5 ++++
>> docs/schemas/domaincommon.rng | 10 +++++++
>> src/conf/domain_conf.c | 39
>> ++++++++++++++++++++++++++--
>> src/conf/domain_conf.h | 3 +++
>> src/conf/numa_conf.c | 27 +++++++++++++++++++
>> src/conf/numa_conf.h | 3 +++
>> src/libvirt_private.syms | 1 +
>> src/qemu/qemu_command.c | 27 ++++++++++++++++---
>> tests/qemuxml2argvdata/hugepages-pages7.args | 3 ++-
>> tests/qemuxml2argvdata/hugepages-pages7.xml | 4 +--
>> tests/qemuxml2argvtest.c | 3 ++-
>> 12 files changed, 148 insertions(+), 11 deletions(-)
>>
>
>> @@ -26658,7 +26690,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>> }
>>
>
> A perfect candidate to be split out in a separate function...
>
>> if (def->mem.nhugepages || def->mem.nosharepages || def->mem.locked
>> - || def->mem.source || def->mem.access || def->mem.allocation)
>> + || def->mem.source || def->mem.access || def->mem.allocation
>> + || def->mem.discard)
>
> ... and converted to use virXMLFormatElement O:-)
Okay. I will separate it out. However, I'll leave the
virXMLFormatElement change for somebody else.
>
>> {
>> virBufferAddLit(buf, "<memoryBacking>\n");
>> virBufferAdjustIndent(buf, 2);
>> @@ -26677,6 +26710,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>> if (def->mem.allocation)
>> virBufferAsprintf(buf, "<allocation mode='%s'/>\n",
>>
>> virDomainMemoryAllocationTypeToString(def->mem.allocation));
>> + if (def->mem.discard)
>> + virBufferAddLit(buf, "<discard/>\n");
>>
>> virBufferAdjustIndent(buf, -2);
>> virBufferAddLit(buf, "</memoryBacking>\n");
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 3c7eccb8ca..52d29124f1 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -2107,6 +2107,7 @@ typedef enum {
>>
>> struct _virDomainMemoryDef {
>> virDomainMemoryAccess access;
>> + int discard; /* enum virTristateBool */
>
> You can use virTristateBool discard; directly
Can I? It's hard to track because every developer on the list have their
own preference. So it's hard to tell who is going to review my patches
and fit their taste.
>
>>
>> /* source */
>> virBitmapPtr sourceNodes;
>> @@ -2269,6 +2270,8 @@ struct _virDomainMemtune {
>> int source; /* enum virDomainMemorySource */
>> int access; /* enum virDomainMemoryAccess */
>> int allocation; /* enum virDomainMemoryAllocation */
>> +
>> + int discard; /* enum virTristateBool */
>
> Same here.
>
>> };
>>
>> typedef struct _virDomainPowerManagement virDomainPowerManagement;
>> diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
>> index 9307dd93d3..a1bbcfa945 100644
>> --- a/src/conf/numa_conf.c
>> +++ b/src/conf/numa_conf.c
>> @@ -77,6 +77,7 @@ struct _virDomainNuma {
>> virBitmapPtr nodeset; /* host memory nodes where this guest
>> node resides */
>> virDomainNumatuneMemMode mode; /* memory mode selection */
>> virDomainMemoryAccess memAccess; /* shared memory access
>> configuration */
>> + int discard; /* discard-data for memory-backend-file,
>> virTristateBool */
>>
>
> And here.
>
>> struct _virDomainNumaDistance {
>> unsigned int value; /* locality value for node i->j or
>> j->i */
>
So what was the showstopper for this patch? I wanted to get this in
upcoming release (and freeze is tomorrow). And having v5 for something
trivial like this sounds unbelievable.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
