Re: [Qemu-devel] [RFC 1/5] virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2334 bytes --]

On Fri, Oct 12, 2018 at 09:40:24AM +0200, David Hildenbrand wrote:
> On 12/10/2018 05:24, David Gibson wrote:
> > When the balloon is inflated, we discard memory place in it using madvise()
> > with MADV_DONTNEED.  And when we deflate it we use MADV_WILLNEED, which
> > sounds like it makes sense but is actually unnecessary.
> > 
> > The misleadingly named MADV_DONTNEED just discards the memory in question,
> > it doesn't set any persistent state on it in-kernel; all that's necessary
> > to bring the memory back is to touch it.  MADV_WILLNEED in contrast
> > specifically says that the memory will be used soon and faults it in.
> > 
> > Memory that's being given back to the guest by deflating the balloon
> > *might* be used soon, but it equally could just sit around in the guest's
> > pools until it actually needs it.  And, over the general timescale that
> > memory ballooning operates, it seems unlikely that one extra fault for the
> > guest will be a vast performance issue.
> > 
> > So, simplify the balloon's operation by dropping the madvise() on deflate.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/virtio/virtio-balloon.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index 1728e4f83a..6ec4bcf4e1 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -35,9 +35,8 @@
> >  
> >  static void balloon_page(void *addr, int deflate)
> >  {
> > -    if (!qemu_balloon_is_inhibited()) {
> > -        qemu_madvise(addr, BALLOON_PAGE_SIZE,
> > -                deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
> > +    if (!qemu_balloon_is_inhibited() && !deflate) {
> > +        qemu_madvise(addr, BALLOON_PAGE_SIZE, QEMU_MADV_DONTNEED);
> >      }
> >  }
> >  
> > 
> 
> Care to rename the function and drop the deflate parameter?
> (call it only when actually inflating)

As you probably found, I do that in a later patch.

> apart from that
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]