Linux Virtualization - May 2013 - [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

Il 28/05/2013 13:44, Michael S. Tsirkin ha scritto:
> negotiated in spec means "present and acked by guest".
> We can try and replace "negotiated" by "present and acked by
guest"
> everywhere - think it will be clearer?
No, I understand what negotiated means.  But in this case,
"negotiated"
is not the word that you want.
>>>> Now rereading it, it may be correct, but it is not clear
enough.
>>>>
>>>> Perhaps my patch is even too verbose, but it doesn't leave
anything open
>>>> for interpretation.
>>>
>>> I'm fine with adding more clarifications but I don't yet
see why
>>> do we need a new bit.
>>
>> There are three cases:
>>
>> 1) the drivers is not able to tell the host first (or never tell the
>> host at all), like the Windows driver or the Google fileballoon driver.
>>  If the host always wants to be told first (e.g. a hypothetical
>> virtio-balloon running on Xen) it should somehow prevent these drivers
>> from running.
> 
> I don't think hosts that always want to be told first can exist.
> Handling guests that don't tell first is super easy -
> e.g. just don't do anything.
Of course you can work around it and not do anything.  This doesn't
change the fact that the host needs to know whether to actually balloon
out pages, or fake it.
>> 2) the driver will always tell the host first, like the Linux driver.
>> The host can trust the guest to do the right thing.
> 
> It should not if guest did not ack the feature bit.
Of course.

1 = guest doesn't ack feature bit, host may have to "fake"
ballooning
2 or 3 = guest acks feature bit, host assumes that guest will tell first
>> 3) the driver wants to optimize if the host can be told last (or not
>> told altogether).  Again, the host can trust the guest to do the right
>> thing, but there are two possible behaviors for the guest driver.
>>
>> The existing bit lets the host distinguish 1 from 2+3.  The other bit
is
>> needed for the guest to pick the right behavior in case 3.
> 
> 1 does not exist.
1 does not exist in the sense that it's always possible to work around
the problem.  But you need to know when to work around it.  In that
sense, 1 exists.
> And guests simply do not treat the existing bit as your spec patch says
> they should. Instead they treat it as they would treat your new bit.
How so?  I even changed the name of the existing bit to
VIRTIO_F_GUEST_TELLS_HOST.

Paolo

On Tue, May 28, 2013 at 02:04:03PM +0200, Paolo Bonzini
wrote:
> Il 28/05/2013 13:44, Michael S. Tsirkin ha scritto:
> > negotiated in spec means "present and acked by guest".
> > We can try and replace "negotiated" by "present and
acked by guest"
> > everywhere - think it will be clearer?
> 
> No, I understand what negotiated means.  But in this case,
"negotiated"
> is not the word that you want.
> 
> >>>> Now rereading it, it may be correct, but it is not clear
enough.
> >>>>
> >>>> Perhaps my patch is even too verbose, but it doesn't
leave anything open
> >>>> for interpretation.
> >>>
> >>> I'm fine with adding more clarifications but I don't
yet see why
> >>> do we need a new bit.
> >>
> >> There are three cases:
> >>
> >> 1) the drivers is not able to tell the host first (or never tell
the
> >> host at all), like the Windows driver or the Google fileballoon
driver.
> >>  If the host always wants to be told first (e.g. a hypothetical
> >> virtio-balloon running on Xen) it should somehow prevent these
drivers
> >> from running.
> > 
> > I don't think hosts that always want to be told first can exist.
> > Handling guests that don't tell first is super easy -
> > e.g. just don't do anything.
> 
> Of course you can work around it and not do anything.  This doesn't
> change the fact that the host needs to know whether to actually balloon
> out pages, or fake it.
> 
> >> 2) the driver will always tell the host first, like the Linux
driver.
> >> The host can trust the guest to do the right thing.
> > 
> > It should not if guest did not ack the feature bit.
> 
> Of course.
> 
> 1 = guest doesn't ack feature bit, host may have to "fake"
ballooning
> 2 or 3 = guest acks feature bit, host assumes that guest will tell first
> 
> >> 3) the driver wants to optimize if the host can be told last (or
not
> >> told altogether).  Again, the host can trust the guest to do the
right
> >> thing, but there are two possible behaviors for the guest driver.
> >>
> >> The existing bit lets the host distinguish 1 from 2+3.  The other
bit is
> >> needed for the guest to pick the right behavior in case 3.
> > 
> > 1 does not exist.
> 
> 1 does not exist in the sense that it's always possible to work around
> the problem.  But you need to know when to work around it.  In that
> sense, 1 exists.
> 
> > And guests simply do not treat the existing bit as your spec patch
says
> > they should. Instead they treat it as they would treat your new bit.
> 
> How so?  I even changed the name of the existing bit to
> VIRTIO_F_GUEST_TELLS_HOST.
> 
> Paolo

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.

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.

How about splitting the patches so we can discuss them separately?

-- 
MST

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