Re: [libvirt] [PATCH v2 5/5] qemu: Enable memory-backend-file.discard-data whenever possible

[Eduardo Habkost]

On Tue, Apr 17, 2018 at 01:16:42PM +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1480668
> 
> The cases when we cannot enable this optimization are:
>   1) nvdimms
>   2) if memAccess='shared'

The specific use case for discard-data=on uses share=on, see:
https://bugzilla.redhat.com/show_bug.cgi?id=1460848#c4

It looks like this will require a explicit XML element, after
all.


> 
> Otherwise it is safe to enable it.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 527a35779d..b920f5c3e4 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3121,6 +3121,13 @@ qemuBuildMemoryBackendStr(virJSONValuePtr 
> *backendProps,
>NULL) < 0)
>  goto cleanup;
>  
> +if (!mem->nvdimmPath && memAccess != VIR_DOMAIN_MEMORY_ACCESS_SHARED 
> &&
> +virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE_DISCARD) &&
> +virJSONValueObjectAdd(props,
> +  "B:discard-data", true,
> +  NULL) < 0)
> +goto cleanup;
> +
>  switch (memAccess) {
>  case VIR_DOMAIN_MEMORY_ACCESS_SHARED:
>  if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0)
> -- 
> 2.16.1
> 

-- 
Eduardo

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

Re: [libvirt] [PATCH v2 5/5] qemu: Enable memory-backend-file.discard-data whenever possible

[Daniel P . Berrangé]

On Tue, Apr 17, 2018 at 02:57:50PM +0200, Peter Krempa wrote:
> On Tue, Apr 17, 2018 at 13:18:02 +0100, Daniel Berrange wrote:
> > On Tue, Apr 17, 2018 at 01:16:42PM +0200, Michal Privoznik wrote:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1480668
> > > 
> > > The cases when we cannot enable this optimization are:
> > >   1) nvdimms
> > >   2) if memAccess='shared'
> > > 
> > > Otherwise it is safe to enable it.
> > 
> > This is basically saying that if memAccess==private, then no person/app 
> > cares
> > about the contents of the memory-backend-file storage after QEMU has exited,
> > because (implicitly) whatever data was in that storage is not going to be 
> > used
> > again. While I accept that's going to be a common case, I'm wary of saying 
> > that
> > is a 100% safe/valid assumption for every user.
> > 
> > I tend think it should require an explicit opt-in to turn on this behaviour
> 
> Well, we used that behaviour prior to storing the backing files in a
> "predictible" path as the file was unlinked right after creating it
> (so that some memory management software can do some magic behind our
> backs with that) so I don't think anybody would depend on the described
> behaviour.

Hmm, I guess that's true.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v2 5/5] qemu: Enable memory-backend-file.discard-data whenever possible

[Peter Krempa]

On Tue, Apr 17, 2018 at 13:18:02 +0100, Daniel Berrange wrote:
> On Tue, Apr 17, 2018 at 01:16:42PM +0200, Michal Privoznik wrote:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1480668
> > 
> > The cases when we cannot enable this optimization are:
> >   1) nvdimms
> >   2) if memAccess='shared'
> > 
> > Otherwise it is safe to enable it.
> 
> This is basically saying that if memAccess==private, then no person/app cares
> about the contents of the memory-backend-file storage after QEMU has exited,
> because (implicitly) whatever data was in that storage is not going to be used
> again. While I accept that's going to be a common case, I'm wary of saying 
> that
> is a 100% safe/valid assumption for every user.
> 
> I tend think it should require an explicit opt-in to turn on this behaviour

Well, we used that behaviour prior to storing the backing files in a
"predictible" path as the file was unlinked right after creating it
(so that some memory management software can do some magic behind our
backs with that) so I don't think anybody would depend on the described
behaviour.


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

Re: [libvirt] [PATCH v2 5/5] qemu: Enable memory-backend-file.discard-data whenever possible

[Daniel P . Berrangé]

On Tue, Apr 17, 2018 at 01:16:42PM +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1480668
> 
> The cases when we cannot enable this optimization are:
>   1) nvdimms
>   2) if memAccess='shared'
> 
> Otherwise it is safe to enable it.

This is basically saying that if memAccess==private, then no person/app cares
about the contents of the memory-backend-file storage after QEMU has exited,
because (implicitly) whatever data was in that storage is not going to be used
again. While I accept that's going to be a common case, I'm wary of saying that
is a 100% safe/valid assumption for every user.

I tend think it should require an explicit opt-in to turn on this behaviour


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH v2 5/5] qemu: Enable memory-backend-file.discard-data whenever possible

[Michal Privoznik]

https://bugzilla.redhat.com/show_bug.cgi?id=1480668

The cases when we cannot enable this optimization are:
  1) nvdimms
  2) if memAccess='shared'

Otherwise it is safe to enable it.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 527a35779d..b920f5c3e4 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3121,6 +3121,13 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
   NULL) < 0)
 goto cleanup;
 
+if (!mem->nvdimmPath && memAccess != VIR_DOMAIN_MEMORY_ACCESS_SHARED &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE_DISCARD) &&
+virJSONValueObjectAdd(props,
+  "B:discard-data", true,
+  NULL) < 0)
+goto cleanup;
+
 switch (memAccess) {
 case VIR_DOMAIN_MEMORY_ACCESS_SHARED:
 if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0)
-- 
2.16.1

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