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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
