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 06:23:19PM +0200, Paolo Bonzini wrote:
> Il 28/05/2013 17:09, Michael S. Tsirkin ha scritto:
> > On Tue, May 28, 2013 at 04:32:44PM +0200, Paolo Bonzini wrote:
> >> Il 28/05/2013 16:29, Michael S. Tsirkin ha scritto:
> >>> 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.
> >>>
> >>> Maybe I'm just dense.
> >>> Let's see the split spec patchset?
> >>
> >> What's unclear exactly?  I'm not sure the spec patchset improves things
> >> that much, I can split it in two or three (change old feature, add new
> >> feature, add explanation) but it's not like changing logic in a program.
> >>
> >> Paolo
> > 
> > Both your code and what you say here about the new bit seem to break
> > compatibility with old hosts and guests.
> 
> What is the exact scenario that you have in mind?

Existing host follows spec, advertises MUST_TELL_HOST (only)
guest acks that and still does not tell host.

> Here are all the possibilities:

Basically it looks like besides TELL_HOST you want another bit
"DONT_TELL_HOST". This just seems weird, and interactions
between the two become very complex. Look at the amount of
text in this thread.


> 
> - host that requires tell-first for a real balloon (doesn't
> propose new bit), any guest (doesn't matter if they are old
> or new since the new bit is never negotiated).
> 
> Here, the old text suggested that the host need not do anything special,
> but it wouldn't have worked with Windows guests.  The host has to
> provide a fake balloon, or prevent the driver from working (e.g. hide
> the virtqueues).  So this is a change indeed, but the same change is
> present with your 1-word change too.  We have:
> 
> negotiated old bit          host operation          guest operation
>       F                     fake balloon            need not tell host
>       T                     real balloon            tells host
> 
> Note that the guest ignores the result of negotiating the old bit,
> only the host cares and only if it requires tell-first.
> 
> 
> The other cases have no such change:
> 
> - old host, doesn't require tell-first (doesn't propose new bit):
> 
> negotiated old bit          host operation          guest operation
>       F                     real balloon            need not tell host
>       T                     real balloon            tells host
> 
> 
> - new host, doesn't require tell-first (proposes new bit), old guest
> (doesn't propose new bit, hence it is never negotiated):
> 
> negotiated old bit          host operation          guest operation
>       F                     real balloon            need not tell host
>       T                     real balloon            tells host
> 
> Same as the previous case, since in both cases the new bit is not
> negotiated.
> 
> Note that the host ignores the result of negotiating the new bit,
> only the guest cares.
> 
> 
> - new host, doesn't require tell-first (proposes new bit), new guest
> (proposes new bit, thus it is negotiated):
> 
> negotiated old bit   negotiated new bit      host operation          guest operation
>       F                    T                 real balloon            need not tell host
>       T                    T                 real balloon            need not tell host
> 
> 
> - host that requires tell-first for a real balloon (doesn't matter
> if old or new since it doesn't propose the new bit, hence it is
> never negotiated), new guest:
> 
> negotiated old bit   negotiated new bit      host operation          guest operation
>       T                    F                 real balloon            need not tell host
> 
> The very last case is the interesting one.  Without the new bit, the
> guest has to promise it will tell the host first when deflating.
> With the new bit, the guest is just telling that it *can* tell
> the host first; the host can still say "don't worry about that".
> So the guest sees the new bit and thinks "I told the host I *could*
> tell it about deflated pages, but the host told me I need not, so
> I won't do it".  This is the optimized case I was talking about.

If host does not need to be told about reclaimed pages, why advertise
MUST_TELL_HOST?


root of all evil and all that ...

> > If it's in spec, I think it would be clearer what are we trying to
> > achieve, and how.
> 
> Having one or three patches doesn't change the final text...
> 
> Paolo

It changes the fact that we can stop arguing about
the thing we agree on (making TELL_HOST optional
for guests).

We can separately argue about the one we don't seem
to agree on (need for a new SILENT_DEFLATE).


-- 
MST