postcopy: Synchronize usage of the balloon inhibitor

While the qemu_balloon_inhibit() interface appears rather general purpose,
postcopy uses it in a last-caller-wins approach with no guarantee of balanced
inhibits and de-inhibits.  Wrap postcopy's usage of the inhibitor to give it
one vote overall, using the same last-caller-wins approach as previously
implemented at the balloon level.

Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Fixes: 01ccbec7bdf6 ("balloon: Allow multiple inhibit users")
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

Note that checkpatch.pl doesn't appreciate the initialization of the
static variable to false, imo it improves the readability of the code
in this instance, so I've left it.  Shout if you disagree.

If this gets acks I can include it with a pull request for a couple
other fixes to the original series.  Thanks

 migration/postcopy-ram.c |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

On 08/22/2018 09:32 PM, Alex Williamson wrote:
> While the qemu_balloon_inhibit() interface appears rather general purpose,
> postcopy uses it in a last-caller-wins approach with no guarantee of balanced
> inhibits and de-inhibits.  Wrap postcopy's usage of the inhibitor to give it
> one vote overall, using the same last-caller-wins approach as previously
> implemented at the balloon level.
> 
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Fixes: 01ccbec7bdf6 ("balloon: Allow multiple inhibit users")

This make qemu iotest 169 working again. 

Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>


> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> Note that checkpatch.pl doesn't appreciate the initialization of the
> static variable to false, imo it improves the readability of the code
> in this instance, so I've left it.  Shout if you disagree.
> 
> If this gets acks I can include it with a pull request for a couple
> other fixes to the original series.  Thanks
> 
>  migration/postcopy-ram.c |   18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 932f18894990..c2e387ed44b4 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -509,6 +509,20 @@ int postcopy_ram_incoming_init(MigrationIncomingState *mis)
>      return 0;
>  }
>  
> +/*
> + * Manage a single vote to the QEMU balloon inhibitor for all postcopy usage,
> + * last caller wins.
> + */
> +static void postcopy_balloon_inhibit(bool state)
> +{
> +    static bool cur_state = false;
> +
> +    if (state != cur_state) {
> +        qemu_balloon_inhibit(state);
> +        cur_state = state;
> +    }
> +}
> +
>  /*
>   * At the end of a migration where postcopy_ram_incoming_init was called.
>   */
> @@ -539,7 +553,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
>          mis->have_fault_thread = false;
>      }
>  
> -    qemu_balloon_inhibit(false);
> +    postcopy_balloon_inhibit(false);
>  
>      if (enable_mlock) {
>          if (os_mlock() < 0) {
> @@ -1107,7 +1121,7 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
>       * Ballooning can mark pages as absent while we're postcopying
>       * that would cause false userfaults.
>       */
> -    qemu_balloon_inhibit(true);
> +    postcopy_balloon_inhibit(true);
>  
>      trace_postcopy_ram_enable_notify();
>  
>

On Wed, 22 Aug 2018 13:32:24 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> While the qemu_balloon_inhibit() interface appears rather general purpose,
> postcopy uses it in a last-caller-wins approach with no guarantee of balanced
> inhibits and de-inhibits.  Wrap postcopy's usage of the inhibitor to give it
> one vote overall, using the same last-caller-wins approach as previously
> implemented at the balloon level.
> 
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Fixes: 01ccbec7bdf6 ("balloon: Allow multiple inhibit users")
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> Note that checkpatch.pl doesn't appreciate the initialization of the
> static variable to false, imo it improves the readability of the code
> in this instance, so I've left it.  Shout if you disagree.

I agree that it improves readability.

> 
> If this gets acks I can include it with a pull request for a couple
> other fixes to the original series.  Thanks
> 
>  migration/postcopy-ram.c |   18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Alex Williamson <alex.williamson@redhat.com> wrote:
> While the qemu_balloon_inhibit() interface appears rather general purpose,
> postcopy uses it in a last-caller-wins approach with no guarantee of balanced
> inhibits and de-inhibits.  Wrap postcopy's usage of the inhibitor to give it
> one vote overall, using the same last-caller-wins approach as previously
> implemented at the balloon level.
>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Fixes: 01ccbec7bdf6 ("balloon: Allow multiple inhibit users")
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

You can include it or I can include it on my next PULL.  But I "emptied"
my queue yesterday, so if you have anything to push, feel free to
include it.