Re: [Qemu-ppc] [RFC 2/2] virtio-balloon: Restore MADV_WILLNEED hint on balloon deflate

[Michael S. Tsirkin]

On Tue, Mar 05, 2019 at 03:15:38PM +0100, David Hildenbrand wrote:
> On 05.03.19 06:11, David Gibson wrote:
> > Prior to f6deb6d9 "virtio-balloon: Remove unnecessary MADV_WILLNEED on
> > deflate", the balloon device issued an madvise() MADV_WILLNEED on
> > pages removed from the balloon.  That would hint to the host kernel
> > that the pages were likely to be needed by the guest in the near
> > future.
> > 
> > It's unclear if this is actually valuable or not, and so f6deb6d9
> > removed this, essentially ignoring balloon deflate requests.  However,
> > concerns have been raised that this might cause a performance
> > regression by causing extra latency for the guest in certain
> > configurations.
> 
> I mean, it will mainly create page tables as far as I know. Any write to
> a page will have an overhead either way (COW zero page). Reads *might*
> be faster.
> 
> As we are working on 4k granularity in the balloon (and doing
> MADV_DONTNEED on 4k granularity!), there will most probably be page
> tables already either way. A page table could only be zapped if all
> pages of that page table are MADV_DONTNEED'ed (or I assume never were
> touched), and I am not sure if "random MADV_DONTNEED'ing of 4k pages"
> will actually get rid of page tables (my assumption would be: only if a
> complete range is zapped at once). I haven't looked into the details,
> though (plenty of other stuff to do).
> 
> I am not sure if I share the concerns. Real-time workload should never
> use the virtio-balloon in a way that anything like that would be possible.
> 
> > 
> > So, until we can get actual benchmark data to see if that's the case,
> > this restores (by default) the old behaviour, issuing a MADV_WILLNEED
> > when a page is removed from the balloon.  A new property on the
> > balloon device "hint-on-deflate" can be set to false to remove this
> > behaviour for testing.
> 
> This is certainly a good approach for you to finally be able to leave
> the ugly land of virtio-balloon :)
> 
> But at least to me, this looks completely useless. I'll be happy to be
> proven wrong as always :)

Point is if we don't intend to extend balloon any further then let's
not make random untested changes to its behaviour.

> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  hw/virtio/virtio-balloon.c         | 15 +++++++++++++++
> >  include/hw/virtio/virtio-balloon.h |  1 +
> >  2 files changed, 16 insertions(+)
> > 
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index e5e82b556d..69968502d9 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -146,6 +146,20 @@ static void balloon_deflate_page(VirtIOBalloon 
> > *balloon,
> >              balloon->pbp = NULL;
> >          }
> >      }
> > +
> > +    if (balloon->hint_on_deflate) {
> > +        void *host_addr = (void *)((uintptr_t)addr & ~(rb_page_size - 1));
> > +        int ret;
> > +
> > +        /* When a page is deflated, we hint the whole host page it
> > +         * lives on, since we can't do anything smaller */
> > +        ret = qemu_madvise(host_addr, rb_page_size, QEMU_MADV_WILLNEED);
> > +        if (ret != 0) {
> > +            warn_report("Couldn't MADV_WILLNEED on balloon deflate: %s",
> > +                        strerror(errno));
> > +            /* Otherwise ignore, failing to page hint shouldn't be fatal */
> > +        }
> > +    }
> >  }
> >  
> >  static const char *balloon_stat_names[] = {
> > @@ -622,6 +636,7 @@ static const VMStateDescription vmstate_virtio_balloon 
> > = {
> >  static Property virtio_balloon_properties[] = {
> >      DEFINE_PROP_BIT("deflate-on-oom", VirtIOBalloon, host_features,
> >                      VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
> > +    DEFINE_PROP_BOOL("hint-on-deflate", VirtIOBalloon, hint_on_deflate, 
> > true),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > diff --git a/include/hw/virtio/virtio-balloon.h 
> > b/include/hw/virtio/virtio-balloon.h
> > index 99dcd6d105..69732cedaa 100644
> > --- a/include/hw/virtio/virtio-balloon.h
> > +++ b/include/hw/virtio/virtio-balloon.h
> > @@ -45,6 +45,7 @@ typedef struct VirtIOBalloon {
> >      int64_t stats_poll_interval;
> >      uint32_t host_features;
> >      PartiallyBalloonedPage *pbp;
> > +    bool hint_on_deflate;
> >  } VirtIOBalloon;
> >  
> >  #endif
> > 
> 
> 
> -- 
> 
> Thanks,
> 
> David / dhildenb