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

Il 28/05/2013 12:45, Michael S. Tsirkin ha scritto:
> On Tue, May 28, 2013 at 10:38:35AM +0200, Paolo Bonzini wrote:
>> Il 27/05/2013 19:02, Michael S. Tsirkin ha scritto:
>>>>> So if we don't want to require all guests to tell host
>>>>> first, all we need to do is admit it's not a bug.
>>>>
>>>> I think we want the possibility for the host to require that.
>>>
>>> But why? TELL_HOST makes some optimizations possible, but if
>>> guest won't cooperate, balloon is useless anyway.
>>
>> If the guest won't tell host and still propose the feature,
> 
> Ack feature but don't tell host? That would be a clear guest bug.
> AFAIK that's not what windows drivers do.
> Am I wrong?
Yes.  I think we are in agreement on this part.
>>>>> Please see
>>>>> 	[PATCH] virtio-spec: balloon: MUST_TELL_HOST is optional
>>>>> that does exactly this.
>>>>
>>>> That patch mandates a change in guest behavior that is not
compatible
>>>> with the existing Windows driver.  Mine doesn't.
>>>
>>> Hmm I don't see it.
>>> In fact the goal was to document the Windows driver behaviour
>>> as correct. Can you explain the incompatibility please?
>>
>> Whenever "If the X feature is (not) negotiated" is used in
the spec, it
>> means "in general you should be ready to implement both
behaviors", or
>> perhaps the guest should fail to initialize if the feature is not
available.
> 
> "you" meaning host. Yes.
Even guest.  A virtio-net guest driver should be ready to use an older
method if the host doesn't support merged rx buffers, for example.

In this case, a "tell host first" guest has to do nothing special if
the
host doesn't advertise the feature.  It is a bit different from other
uses of "negotiated" in the spec.
> But here guest can tell host first *always if it wants to - it will
> just be a bit slower when reusing pages from balloon.
> If it acked the feature, it *must* tell host first.
Yes.
>> The way I read it yesterday I didn't see any change from the
current
>> specification, so the problem of having a "negative feature"
remains.
> 
> This is standard behaviour:
> 
> - guest can ignore any feature that it does not ack
> - host must implement both behaviours for guests that
>   do and for guests that do not ack features
> 
> This is exactly what I'm proposing for TELL_HOST.
I know, but I think the use "negotiated" part is unclear.
>> 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.

2) the driver will always tell the host first, like the Linux driver.
The host can trust the guest to do the right thing.

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.

Case (3) would be a trivial optimization to implement on the Linux
driver for example, but one could also imagine switching the
implementation entirely: use something like Luiz's shrinker if the host
needs to be told, use something like Google's fileballoon if it doesn't.

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.

Paolo

On Tue, May 28, 2013 at 01:13:03PM +0200, Paolo Bonzini
wrote:
> Il 28/05/2013 12:45, Michael S. Tsirkin ha scritto:
> > On Tue, May 28, 2013 at 10:38:35AM +0200, Paolo Bonzini wrote:
> >> Il 27/05/2013 19:02, Michael S. Tsirkin ha scritto:
> >>>>> So if we don't want to require all guests to tell
host
> >>>>> first, all we need to do is admit it's not a bug.
> >>>>
> >>>> I think we want the possibility for the host to require
that.
> >>>
> >>> But why? TELL_HOST makes some optimizations possible, but if
> >>> guest won't cooperate, balloon is useless anyway.
> >>
> >> If the guest won't tell host and still propose the feature,
> > 
> > Ack feature but don't tell host? That would be a clear guest bug.
> > AFAIK that's not what windows drivers do.
> > Am I wrong?
> 
> Yes.  I think we are in agreement on this part.
> 
> >>>>> Please see
> >>>>> 	[PATCH] virtio-spec: balloon: MUST_TELL_HOST is
optional
> >>>>> that does exactly this.
> >>>>
> >>>> That patch mandates a change in guest behavior that is not
compatible
> >>>> with the existing Windows driver.  Mine doesn't.
> >>>
> >>> Hmm I don't see it.
> >>> In fact the goal was to document the Windows driver behaviour
> >>> as correct. Can you explain the incompatibility please?
> >>
> >> Whenever "If the X feature is (not) negotiated" is used
in the spec, it
> >> means "in general you should be ready to implement both
behaviors", or
> >> perhaps the guest should fail to initialize if the feature is not
available.
> > 
> > "you" meaning host. Yes.
> 
> Even guest.  A virtio-net guest driver should be ready to use an older
> method if the host doesn't support merged rx buffers, for example.
> 
> In this case, a "tell host first" guest has to do nothing special
if the
> host doesn't advertise the feature.  It is a bit different from other
> uses of "negotiated" in the spec.
If you just want to add this line
"It's legal for guest to tell host first even if
 it did not acknowledge the TELL_HOST feature"
then that's fine.
> > But here guest can tell host first *always if it wants to - it will
> > just be a bit slower when reusing pages from balloon.
> > If it acked the feature, it *must* tell host first.
> 
> Yes.
> 
> >> The way I read it yesterday I didn't see any change from the
current
> >> specification, so the problem of having a "negative
feature" remains.
> > 
> > This is standard behaviour:
> > 
> > - guest can ignore any feature that it does not ack
> > - host must implement both behaviours for guests that
> >   do and for guests that do not ack features
> > 
> > This is exactly what I'm proposing for TELL_HOST.
> 
> I know, but I think the use "negotiated" part is unclear.
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?
> >> 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.
> 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.
And no host we ever released thankfully does this.
> 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.
> 
> Case (3) would be a trivial optimization to implement on the Linux
> driver for example, but one could also imagine switching the
> implementation entirely: use something like Luiz's shrinker if the host
> needs to be told, use something like Google's fileballoon if it
doesn't.
> 
> 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.
> 
> Paolo
1 does not exist.

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.  So
let's just make spec match driver behaviour.  Less work for everyone,
and no need for the new bit.

-- 
MST

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