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
--
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
