Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, May 28, 2013 at 04:06:02PM +0200, Paolo Bonzini wrote:
> Il 28/05/2013 15:32, Michael S. Tsirkin ha scritto:
> > At this point I am confused. I think there are two changes in your patch:
> > 
> > 1. Handling of VIRTIO_F_GUEST_MUST_TELL_HOST
> >  Is this functionally identical to what I proposed?
> >  If yes, I am fine with either change being applied.
> 
> Yes.
> 
> > 2. New SILENT_DEFLATE feature
> >  Since guest can get same functionality by not acking
> >  TELL_HOST, I still don't see what good it does:
> >  Historically a host with no features supports silent
> >  deflate and guest with no features can do silent deflate.
> >  I conclude silent deflate is the default behaviour for
> >  both host and guest, and we can't change default without
> >  breaking compatibility.
> 
> You're right that for correctness the existing feature is enough:
> if it is not negotiated by the guest, the host ensures correctness by
> only giving the guest a fake balloon.
> 
> However, the new feature is about optimization, not correctness.
> In fact, VIRTIO_BALLOON_F_SILENT_DEFLATE is the optimization
> feature that VIRTIO_BALLOON_F_MUST_TELL_HOST was meant to be.
> 
> What I'm interested in, is drivers that can _optionally_ use silent 
> deflation (as an optimization).  These should not get a fake balloon!
> 
> With the new feature bit, these drivers should propose both
> VIRTIO_BALLOON_F_GUEST_TELLS_HOST and VIRTIO_BALLOON_F_SILENT_DEFLATE.
> The driver can then use silent  deflation if and only if the host
> has negotiated  VIRTIO_BALLOON_F_SILENT_DEFLATE too.  Like this:
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index bd3ae32..05fe948 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -186,12 +186,8 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
>  		vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
>  	}
>  
> -	/*
> -	 * Note that if
> -	 * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> -	 * is true, we *have* to do it in this order
> -	 */
> -	tell_host(vb, vb->deflate_vq);
> +	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_SILENT_DEFLATE)
> +		tell_host(vb, vb->deflate_vq);
>  	mutex_unlock(&vb->balloon_lock);
>  	release_pages_by_pfn(vb->pfns, vb->num_pfns);
>  }
> @@ -543,6 +539,7 @@ static int virtballoon_restore(struct virtio_device *vdev)
>  static unsigned int features[] = {
>  	VIRTIO_BALLOON_F_MUST_TELL_HOST,
>  	VIRTIO_BALLOON_F_STATS_VQ,
> +	VIRTIO_BALLOON_F_SILENT_DEFLATE,
>  };
>  
>  static struct virtio_driver virtio_balloon_driver = {
> 
> 
> Of course with the current implementation of the balloon it does not
> matter much.  But for example, with Luiz's work, releasing pages as soon
> as the shrinker is called will increase effectiveness of the shrinker.
> At the same time, not all is lost if the guest prefers not to allow
> silent deflation (e.g. because there is an assigned device).
> 
> On old hosts, a guest that can optionally use silent deflation will
> not use it.  That's the same as for any other feature bit.
> 
> > How about splitting the patches so we can discuss them separately?
> 
> I can do that, but I hope the above clarifies it.
> 
> Paolo

Maybe I'm just dense.
Let's see the split spec patchset?

-- 
MST