Re: [PATCH v1 01/17] exec: Introduce ram_block_discard_set_(unreliable|required)()

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* David Hildenbrand (david@redhat.com) wrote:
> We want to replace qemu_balloon_inhibit() by something more generic.
> Especially, we want to make sure that technologies that really rely on
> RAM block discards to work reliably to run mutual exclusive with
> technologies that break it.
> 
> E.g., vfio will usually pin all guest memory, turning the virtio-balloon
> basically useless and make the VM consume more memory than reported via
> the balloon. While the balloon is special already (=> no guarantees, same
> behavior possible afer reboots and with huge pages), this will be
> different, especially, with virtio-mem.
> 
> Let's implement a way such that we can make both types of technology run
> mutually exclusive. We'll convert existing balloon inhibitors in successive
> patches and add some new ones. Add the check to
> qemu_balloon_is_inhibited() for now. We might want to make
> virtio-balloon an acutal inhibitor in the future - however, that
> requires more thought to not break existing setups.
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  balloon.c             |  3 ++-
>  exec.c                | 48 +++++++++++++++++++++++++++++++++++++++++++
>  include/exec/memory.h | 41 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 91 insertions(+), 1 deletion(-)
> 
> diff --git a/balloon.c b/balloon.c
> index f104b42961..c49f57c27b 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -40,7 +40,8 @@ static int balloon_inhibit_count;
>  
>  bool qemu_balloon_is_inhibited(void)
>  {
> -    return atomic_read(&balloon_inhibit_count) > 0;
> +    return atomic_read(&balloon_inhibit_count) > 0 ||
> +           ram_block_discard_is_broken();
>  }
>  
>  void qemu_balloon_inhibit(bool state)
> diff --git a/exec.c b/exec.c
> index 2874bb5088..52a6e40e99 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -4049,4 +4049,52 @@ void mtree_print_dispatch(AddressSpaceDispatch *d, MemoryRegion *root)
>      }
>  }
>  
> +static int ram_block_discard_broken;

This could do with a comment; if I'm reading this right then
  +ve means broken
  -ve means required

> +int ram_block_discard_set_broken(bool state)
> +{
> +    int old;
> +
> +    if (!state) {
> +        atomic_dec(&ram_block_discard_broken);
> +        return 0;
> +    }
> +
> +    do {
> +        old = atomic_read(&ram_block_discard_broken);
> +        if (old < 0) {
               /* Currently required */
> +            return -EBUSY;
> +        }
> +    } while (atomic_cmpxchg(&ram_block_discard_broken, old, old + 1) != old);
> +    return 0;
> +}
> +
> +int ram_block_discard_set_required(bool state)
> +{
> +    int old;
> +
> +    if (!state) {
> +        atomic_inc(&ram_block_discard_broken);
> +        return 0;
> +    }
> +
> +    do {
> +        old = atomic_read(&ram_block_discard_broken);
> +        if (old > 0) {
               /* Currently broken */
> +            return -EBUSY;
> +        }
> +    } while (atomic_cmpxchg(&ram_block_discard_broken, old, old - 1) != old);
> +    return 0;
> +}
> +
> +bool ram_block_discard_is_broken(void)
> +{
> +    return atomic_read(&ram_block_discard_broken) > 0;
> +}
> +
> +bool ram_block_discard_is_required(void)
> +{
> +    return atomic_read(&ram_block_discard_broken) < 0;
> +}
> +
>  #endif
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e000bd2f97..9bb5ced38d 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2463,6 +2463,47 @@ static inline MemOp devend_memop(enum device_endian end)
>  }
>  #endif
>  
> +/*
> + * Inhibit technologies that rely on discarding of parts of RAM blocks to work
> + * reliably, e.g., to manage the actual amount of memory consumed by the VM
> + * (then, the memory provided by RAM blocks might be bigger than the desired
> + * memory consumption). This *must* be set if:

'technologies that rely on discarding of parts of RAM blocks to work
reliably' is pretty long; I'm not sure of a better way of saying it
though.

Other than the comments;


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> + * - Discarding parts of a RAM blocks does not result in the change being
> + *   reflected in the VM and the pages getting freed.
> + * - All memory in RAM blocks is pinned or duplicated, invaldiating any previous
> + *   discards blindly.
> + * - Discarding parts of a RAM blocks will result in integrity issues (e.g.,
> + *   encrypted VMs).
> + * Technologies that only temporarily pin the current working set of a
> + * driver are fine, because we don't expect such pages to be discarded
> + * (esp. based on guest action like balloon inflation).
> + *
> + * This is *not* to be used to protect from concurrent discards (esp.,
> + * postcopy).
> + *
> + * Returns 0 if successful. Returns -EBUSY if a technology that relies on
> + * discards to work reliably is active.
> + */
> +int ram_block_discard_set_broken(bool state);
> +
> +/*
> + * Inhibit technologies that will break discarding of pages in RAM blocks.
> + *
> + * Returns 0 if successful. Returns -EBUSY if discards are already set to
> + * broken.
> + */
> +int ram_block_discard_set_required(bool state);
> +
> +/*
> + * Test if discarding of memory in ram blocks is broken.
> + */
> +bool ram_block_discard_is_broken(void);
> +
> +/*
> + * Test if discarding of memory in ram blocks is required to work reliably.
> + */
> +bool ram_block_discard_is_required(void);
> +
>  #endif
>  
>  #endif
> -- 
> 2.25.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK