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.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 balloon.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/balloon.c b/balloon.c
index 6bf0a9681377..2a6a7e1a22a0 100644
--- a/balloon.c
+++ b/balloon.c
@@ -37,16 +37,17 @@
 static QEMUBalloonEvent *balloon_event_fn;
 static QEMUBalloonStatus *balloon_stat_fn;
 static void *balloon_opaque;
-static bool balloon_inhibited;
+static int balloon_inhibited;
 
 bool qemu_balloon_is_inhibited(void)
 {
-    return balloon_inhibited;
+    return balloon_inhibited > 0;
 }
 
 void qemu_balloon_inhibit(bool state)
 {
-    balloon_inhibited = state;
+    balloon_inhibited += (state ? 1 : -1);
+    assert(balloon_inhibited >= 0);
 }
 
 static bool have_balloon(Error **errp)

On Tue, Jul 17, 2018 at 04:47:37PM -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.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  balloon.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/balloon.c b/balloon.c
> index 6bf0a9681377..2a6a7e1a22a0 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -37,16 +37,17 @@
>  static QEMUBalloonEvent *balloon_event_fn;
>  static QEMUBalloonStatus *balloon_stat_fn;
>  static void *balloon_opaque;
> -static bool balloon_inhibited;
> +static int balloon_inhibited;
>  
>  bool qemu_balloon_is_inhibited(void)
>  {
> -    return balloon_inhibited;
> +    return balloon_inhibited > 0;
>  }
>  
>  void qemu_balloon_inhibit(bool state)
>  {
> -    balloon_inhibited = state;
> +    balloon_inhibited += (state ? 1 : -1);
> +    assert(balloon_inhibited >= 0);

Better do it atomically?

>  }
>  
>  static bool have_balloon(Error **errp)
> 
> 

Regards,

-- 
Peter Xu

On Wed, 18 Jul 2018 14:40:15 +0800
Peter Xu <peterx@redhat.com> wrote:

> On Tue, Jul 17, 2018 at 04:47:37PM -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.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  balloon.c |    7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/balloon.c b/balloon.c
> > index 6bf0a9681377..2a6a7e1a22a0 100644
> > --- a/balloon.c
> > +++ b/balloon.c
> > @@ -37,16 +37,17 @@
> >  static QEMUBalloonEvent *balloon_event_fn;
> >  static QEMUBalloonStatus *balloon_stat_fn;
> >  static void *balloon_opaque;
> > -static bool balloon_inhibited;
> > +static int balloon_inhibited;
> >  
> >  bool qemu_balloon_is_inhibited(void)
> >  {
> > -    return balloon_inhibited;
> > +    return balloon_inhibited > 0;
> >  }
> >  
> >  void qemu_balloon_inhibit(bool state)
> >  {
> > -    balloon_inhibited = state;
> > +    balloon_inhibited += (state ? 1 : -1);
> > +    assert(balloon_inhibited >= 0);  
> 
> Better do it atomically?

I'd assumed we're protected by the BQL anywhere this is called.  Is
that not the case?  Generally when I try to add any sort of locking to
QEMU it's shot down because the code paths are already serialized.
Thanks,

Alex

On Wed, Jul 18, 2018 at 10:37:36AM -0600, Alex Williamson wrote:
> On Wed, 18 Jul 2018 14:40:15 +0800
> Peter Xu <peterx@redhat.com> wrote:
> 
> > On Tue, Jul 17, 2018 at 04:47:37PM -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.
> > > 
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > ---
> > >  balloon.c |    7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/balloon.c b/balloon.c
> > > index 6bf0a9681377..2a6a7e1a22a0 100644
> > > --- a/balloon.c
> > > +++ b/balloon.c
> > > @@ -37,16 +37,17 @@
> > >  static QEMUBalloonEvent *balloon_event_fn;
> > >  static QEMUBalloonStatus *balloon_stat_fn;
> > >  static void *balloon_opaque;
> > > -static bool balloon_inhibited;
> > > +static int balloon_inhibited;
> > >  
> > >  bool qemu_balloon_is_inhibited(void)
> > >  {
> > > -    return balloon_inhibited;
> > > +    return balloon_inhibited > 0;
> > >  }
> > >  
> > >  void qemu_balloon_inhibit(bool state)
> > >  {
> > > -    balloon_inhibited = state;
> > > +    balloon_inhibited += (state ? 1 : -1);
> > > +    assert(balloon_inhibited >= 0);  
> > 
> > Better do it atomically?
> 
> I'd assumed we're protected by the BQL anywhere this is called.  Is
> that not the case?  Generally when I try to add any sort of locking to
> QEMU it's shot down because the code paths are already serialized.

Ah I see. :-)

But I guess current code might call these without BQL.  For example,
qemu_balloon_inhibit() has:

postcopy_ram_listen_thread
  qemu_loadvm_state_main
    loadvm_process_command
      loadvm_postcopy_handle_listen
        postcopy_ram_enable_notify
          qemu_balloon_inhibit

While the whole stack is inside a standalone thread to load QEMU for
postcopy incoming migration rather than the main thread.

Thanks,

-- 
Peter Xu