Re: [Qemu-devel] [PATCH v3 1/4] balloon: Allow multiple inhibit users

[Alex Williamson]

On Tue, 7 Aug 2018 22:44:11 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> On Tue, Aug 07, 2018 at 01:31:22PM -0600, Alex Williamson wrote:
> > A simple true/false internal state does not allow multiple users.  Fix
> > this within the existing interface by converting to a counter, so long
> > as the counter is elevated, ballooning is inhibited.
> > 
> > Reviewed-by: David Hildenbrand <address@hidden>
> > Reviewed-by: Peter Xu <address@hidden>
> > Reviewed-by: Cornelia Huck <address@hidden>
> > Signed-off-by: Alex Williamson <address@hidden>
> > ---
> >  balloon.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/balloon.c b/balloon.c
> > index 6bf0a9681377..931987983858 100644
> > --- a/balloon.c
> > +++ b/balloon.c
> > @@ -26,6 +26,7 @@
> >  
> >  #include "qemu/osdep.h"
> >  #include "qemu-common.h"
> > +#include "qemu/atomic.h"
> >  #include "exec/cpu-common.h"
> >  #include "sysemu/kvm.h"
> >  #include "sysemu/balloon.h"
> > @@ -37,16 +38,22 @@
> >  static QEMUBalloonEvent *balloon_event_fn;
> >  static QEMUBalloonStatus *balloon_stat_fn;
> >  static void *balloon_opaque;
> > -static bool balloon_inhibited;
> > +static int balloon_inhibit_count;
> >  
> >  bool qemu_balloon_is_inhibited(void)
> >  {
> > -    return balloon_inhibited;
> > +    return atomic_read(&balloon_inhibit_count) > 0;
> >  }
> >  
> >  void qemu_balloon_inhibit(bool state)
> >  {
> > -    balloon_inhibited = state;
> > +    if (state) {
> > +        atomic_inc(&balloon_inhibit_count);
> > +    } else {
> > +        atomic_dec(&balloon_inhibit_count);
> > +    }
> > +
> > +    assert(atomic_read(&balloon_inhibit_count) >= 0);
> >  }
> >  
> >  static bool have_balloon(Error **errp)  
> 
> This blocks QEMU_MADV_WONTNEED but it also blocks QEMU_MADV_WILLNEED.
> Is this necessarily a good idea?

This is existing balloon inhibitor behavior, but do you have some
reason to suspect WILLNEED is necessary?  It's my impression that
WILLNEED is a purely optional prefetch directive that's entirely
unnecessary if the page wasn't previously zapped with WONTNEED.  If the
page was zapped, it will fault in on demand, potentially with higher
latency, but functionally correct.  With vfio usage of the inhibitor,
we expect pinning to fault in any previously ballooned pages, so
calling WILLNEED on pages where the inhibit count is elevated due to an
assigned device seems unnecessary.  Thanks,

Alex