Re: [libvirt] [PATCH v4 5/5] qemu: Introduce memoryBacking/discard

2018-04-26 Thread Michal Privoznik 
On 04/26/2018 09:34 PM, Peter Krempa wrote:
> On Thu, Apr 26, 2018 at 18:00:15 +0200, Michal Privoznik wrote:
>> On 04/26/2018 02:56 PM, Ján Tomko wrote:
>>> On Fri, Apr 20, 2018 at 11:09:31AM +0200, Michal Privoznik wrote:
> 
> [...]
> 
>> 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.
> 
> I don't think that this tone is warranted.

I'm genuinely asking, because it is not obvious to me what was the major
problem.

> Even if you would get an
> ACK/rb for this patch you would not make it into the release. The review
> was sent 24 seconds _after_ the release-candidate was tagged:

Shoot. 24 seconds.

> While I don't want to question your supernatural patch-fixing skill,
> fixing the typos and the mailing list delay would easily consume the 15
> minutes until the freeze was announced. And it could be even less than
> that as the tag was probably pushed prior to sending the anouncement.
> 

That's why I usually paste diff what needs to be squashed in and say
ACK. It also helps encouraging contributors IMO. Unless it is something
conceptual which renders patch needless.

Michal

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

Re: [libvirt] [PATCH v4 5/5] qemu: Introduce memoryBacking/discard

2018-04-26 Thread Peter Krempa 
On Thu, Apr 26, 2018 at 18:00:15 +0200, Michal Privoznik wrote:
> On 04/26/2018 02:56 PM, Ján Tomko wrote:
> > On Fri, Apr 20, 2018 at 11:09:31AM +0200, Michal Privoznik wrote:

[...]

> 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.

I don't think that this tone is warranted. Even if you would get an
ACK/rb for this patch you would not make it into the release. The review
was sent 24 seconds _after_ the release-candidate was tagged:

 Date: Thu, 26 Apr 2018 14:56:58 +0200
 Subject: Re: [libvirt] [PATCH v4 5/5] qemu: Introduce memoryBacking/discard

 tag v4.3.0-rc1
 TaggerDate: Thu Apr 26 14:56:34 2018 +0200

which was announced on the mailing list 15 minutes later.

 Date: Thu, 26 Apr 2018 15:12:05 +0200
 Subject: [libvirt] Entering freeze for libvirt-4.3.0

While I don't want to question your supernatural patch-fixing skill,
fixing the typos and the mailing list delay would easily consume the 15
minutes until the freeze was announced. And it could be even less than
that as the tag was probably pushed prior to sending the anouncement.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 5/5] qemu: Introduce memoryBacking/discard

2018-04-26 Thread Michal Privoznik 
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 
>> ---
>> 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, "\n");
>>     virBufferAdjustIndent(buf, 2);
>> @@ -26677,6 +26710,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>>     if (def->mem.allocation)
>>     virBufferAsprintf(buf, "\n",
>>    
>> virDomainMemoryAllocationTypeToString(def->mem.allocation));
>> +    if (def->mem.discard)
>> +    virBufferAddLit(buf, "\n");
>>
>>     virBufferAdjustIndent(buf, -2);
>>     virBufferAddLit(buf, "\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.
> 
>>  

Re: [libvirt] [PATCH v4 5/5] qemu: Introduce memoryBacking/discard

2018-04-26 Thread Ján Tomko 

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. It would be nice to split the conf
and qemu changes and provide a XML->XML test.


Signed-off-by: Michal Privoznik 
---
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:-)


{
virBufferAddLit(buf, "\n");
virBufferAdjustIndent(buf, 2);
@@ -26677,6 +26710,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
if (def->mem.allocation)
virBufferAsprintf(buf, "\n",
virDomainMemoryAllocationTypeToString(def->mem.allocation));
+if (def->mem.discard)
+virBufferAddLit(buf, "\n");

virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "\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



/* 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 */


Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v4 5/5] qemu: Introduce memoryBacking/discard

2018-04-20 Thread Michal Privoznik 
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.

Signed-off-by: Michal Privoznik 
---
 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(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index ada0df227f..ea9d77bd18 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1016,6 +1016,7 @@
 source type="file|anonymous"/
 access mode="shared|private"/
 allocation mode="immediate|ondemand"/
+discard/
   /memoryBacking
   ...
 /domain
@@ -1070,6 +1071,14 @@
  numa node by memAccess
allocation
Specify when allocate the memory
+   discard
+   When set and supported by hypervisor the memory
+ content is discarded just before guest shuts down (or
+ when DIMM module is unplugged). Please note that this is
+ just an optimization and is not guaranteed to work in
+ all cases (e.g. when hypervisor crashes).
+ Since 4.3.0 (QEMU/KVM only)
+   
 
 
 
@@ -1608,7 +1617,7 @@
 cpu
   ...
   numa
-cell id='0' cpus='0-3' memory='512000' unit='KiB'/
+cell id='0' cpus='0-3' memory='512000' unit='KiB' discard='yes'/
 cell id='1' cpus='4-7' memory='512000' unit='KiB' 
memAccess='shared'/
   /numa
   ...
@@ -1634,6 +1643,13 @@
   memAccess can control whether the memory is to be
   mapped as "shared" or "private".  This is valid only for
   hugepages-backed memory and nvdimm modules.
+
+  Each cell element can have an optional
+  discard attribute which fine tunes the discard
+  feature for given numa node as described under
+  Memory Backing.
+  Accepted values are yes and no.
+  Since 4.3.0
 
 
 
@@ -7849,7 +7865,7 @@ qemu-kvm -net nic,model=? /dev/null
 
 ...
 devices
-  memory model='dimm' access='private'
+  memory model='dimm' access='private' discard='yes'
 target
   size unit='KiB'524287/size
   node0/node
@@ -7903,6 +7919,20 @@ qemu-kvm -net nic,model=? /dev/null
 
   
 
+  discard
+  
+
+  An optional attribute discard
+  (since 4.3.0) that provides
+  capability to fine tune discard of data on per module
+  basis. Accepted values are yes and
+  no. The feature is described here:
+  Memory Backing.
+  This attribute is allowed only for
+  model='dimm'.
+
+  
+
   source
   
 
diff --git a/docs/schemas/cputypes.rng b/docs/schemas/cputypes.rng
index c45b6dfb28..1f1e0e36d5 100644
--- a/docs/schemas/cputypes.rng
+++ b/docs/schemas/cputypes.rng
@@ -129,6 +129,11 @@
   
 
   
+  
+
+  
+
+  
   
 
   
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 4cab55f05d..9650a779b7 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -633,6 +633,11 @@
 
   
 
+
+  
+
+  
+
   
 
   
@@ -5138,6 +5143,11 @@
   
 
   
+  
+
+  
+
+  
   
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 35666c1347..9585e38bc1 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5508,6 +5508,20 @@ virDomainVideoDefValidate(const virDomainVideoDef