[PATCH v2 0/2] virtio-balloon spec: silent deflation

[PATCH v2 0/2] virtio-balloon spec: silent deflation

[Paolo Bonzini]

Here is the series, split in two patches, with small edits and new
commit messages.

Paolo Bonzini (2):
  virtio-balloon spec: rewrite description of
    VIRTIO_BALLOON_F_MUST_TELL_HOST
  virtio-balloon spec: reintroduce "silent deflation" feature

 virtio-spec.lyx | 238 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 233 insertions(+), 5 deletions(-)

-- 
1.8.2.1

[PATCH v2 1/2] virtio-balloon spec: rewrite description of VIRTIO_BALLOON_F_MUST_TELL_HOST

[Paolo Bonzini]

The idea of the VIRTIO_BALLOON_F_MUST_TELL_HOST feature was to let drivers
skip usage of the deflate queue when leaking the balloon ("silent
deflation").  Guests may benefit from silent deflate by aggressively
inflating the balloon; they know that they will be able to use ballooned
pages without issuing a (blocking) request to the device.

The original spec assumed that every driver supports
VIRTIO_BALLOON_F_MUST_TELL_HOST, but this was not explicitly documented
and in practice it turns out not to be the case; the Windows balloon
driver does not tell the host correctly.

Since all known device implementations support silent deflation, they
do not negotiate the feature and we are thus free to redefine what
the host should do about this feature.

The Windows driver is also the only known driver that does not
negotiate VIRTIO_BALLOON_F_MUST_TELL_HOST.  Thus, even though the
used to be meant for communication from the host, known drivers are
really using it to communicate was in the other direction.

Adjust the spec to conform.  The original intent is reintroduced with
a new feature bit in the next patch, while also fixing a problem with
its definition.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 virtio-spec.lyx | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 57 insertions(+), 5 deletions(-)

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index adec0a5..5c76a87 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -7219,11 +7219,46 @@ bits
 
 \begin_deeper
 \begin_layout Description
-VIRTIO_BALLOON_F_MUST_TELL_HOST
+VIRTIO_BALLOON_F_
+\change_deleted 1531152142 1347020601
+MUST
+\change_inserted 1531152142 1347020602
+CAN
+\change_unchanged
+_TELL_HOST
 \begin_inset space ~
 \end_inset
 
-(0) Host must be told before pages from the balloon are used.
+(0) 
+\change_deleted 1531152142 1347020625
+Host must be told
+\change_inserted 1531152142 1347020617
+Guest is able to tell host
+\change_unchanged
+ before pages from the balloon are used.
+
+\change_inserted 1531152142 1368005603
+ The host must propose this feature if it has to be told
+ before pages from the balloon are used.
+\begin_inset Foot
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 1531152142 1347022389
+This feature used to be named VIRTIO_BALLOON_F_\SpecialChar \-
+MUST_TELL_HOST.
+ However, after a few years it was observed that drivers were not using
+ it as specified.
+ The virtio-balloon spec was then adjusted to what the drivers had been
+ doing.
+\end_layout
+
+\end_inset
+
+
+\change_unchanged
+
 \end_layout
 
 \begin_layout Description
@@ -7382,9 +7417,15 @@ The driver constructs an array of addresses of memory pages it has previously
 \end_layout
 
 \begin_layout Enumerate
-If the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is set, the guest may not
- use these requested pages until that descriptor in the deflateq has been
- used by the device.
+If the VIRTIO_BALLOON_F_
+\change_deleted 1531152142 1369761770
+MUST
+\change_inserted 1531152142 1369761770
+CAN
+\change_unchanged
+_TELL_HOST feature is set, the guest may not use these requested pages until
+ that descriptor in the deflateq has been used by the device.
+
 \end_layout
 
 \begin_layout Enumerate
@@ -7396,10 +7437,21 @@ status open
 
 \begin_layout Plain Layout
 In this case, deflation advice is merely a courtesy
+\change_inserted 1531152142 1369761798
+.
+ The guest need not use the deflateq at all.
+\change_unchanged
+
 \end_layout
 
 \end_inset
 
+
+\change_inserted 1531152142 1369761801
+ If the host does not support this, it should not do anything when the balloon
+ is inflated or deflated, except put the descriptors on the used ring.
+
+\change_unchanged
  
 \end_layout
 
-- 
1.8.2.1

Re: [PATCH v2 1/2] virtio-balloon spec: rewrite description of VIRTIO_BALLOON_F_MUST_TELL_HOST

[Michael S. Tsirkin]

On Tue, May 28, 2013 at 07:40:17PM +0200, Paolo Bonzini wrote:
> The idea of the VIRTIO_BALLOON_F_MUST_TELL_HOST feature was to let drivers
> skip usage of the deflate queue when leaking the balloon ("silent
> deflation").  Guests may benefit from silent deflate by aggressively
> inflating the balloon; they know that they will be able to use ballooned
> pages without issuing a (blocking) request to the device.
> 
> The original spec assumed that every driver supports
> VIRTIO_BALLOON_F_MUST_TELL_HOST, but this was not explicitly documented
> and in practice it turns out not to be the case; the Windows balloon
> driver does not tell the host correctly.
> 
> Since all known device implementations support silent deflation, they
> do not negotiate the feature and we are thus free to redefine what
> the host should do about this feature.
> 
> The Windows driver is also the only known driver that does not
> negotiate VIRTIO_BALLOON_F_MUST_TELL_HOST.  Thus, even though the
> used to be meant for communication from the host, known drivers are
> really using it to communicate was in the other direction.
> 
> Adjust the spec to conform.  The original intent is reintroduced with
> a new feature bit in the next patch, while also fixing a problem with
> its definition.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

What you write in spec below and what you write above seems to
contradict.

Look, how about the simpler patch that I sent:
"[PATCH] virtio-spec: balloon: MUST_TELL_HOST is optional"

does it functionally do everything you want in this patch?

If yes maybe you could pick that up, and add
a patch with renames and text clarifications on top?

More comments below.

> ---
>  virtio-spec.lyx | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 57 insertions(+), 5 deletions(-)
> 
> diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> index adec0a5..5c76a87 100644
> --- a/virtio-spec.lyx
> +++ b/virtio-spec.lyx
> @@ -7219,11 +7219,46 @@ bits
>  
>  \begin_deeper
>  \begin_layout Description
> -VIRTIO_BALLOON_F_MUST_TELL_HOST
> +VIRTIO_BALLOON_F_
> +\change_deleted 1531152142 1347020601
> +MUST
> +\change_inserted 1531152142 1347020602
> +CAN
> +\change_unchanged
> +_TELL_HOST
>  \begin_inset space ~
>  \end_inset
>  
> -(0) Host must be told before pages from the balloon are used.
> +(0) 
> +\change_deleted 1531152142 1347020625
> +Host must be told
> +\change_inserted 1531152142 1347020617
> +Guest is able to tell host
> +\change_unchanged
> + before pages from the balloon are used.
> +

This "can" and "is able" is IMO more confusing than clarifying.
Let's be definite. If feature is negotiated, guest must tell host.
If it's not, it does not.

That's why it's MUST not CAN.

And text should be

"When negotiated, guest tells host
before pages from the balloon are used.
"


> +\change_inserted 1531152142 1368005603
> + The host must propose this feature

We don't say "propose feature" anyway in the spec.
You really mean "set this feature bit" ?

> if it has to be told
> + before pages from the balloon are used.

Has to?  Not at all. It can't assume anything
until guest has negotiated the feature.
So it should be
"if it wants to be told before pages from the balloon are used".


> +\begin_inset Foot
> +status open
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 1531152142 1347022389
> +This feature used to be named VIRTIO_BALLOON_F_\SpecialChar \-
> +MUST_TELL_HOST.
> + However, after a few years it was observed that drivers were not using
> + it as specified.
> + The virtio-balloon spec was then adjusted to what the drivers had been
> + doing.

I'm not sure what good does this historical note do us.
I would say:
	Historically the spec required all drivers
	to acknowledge this feature bit.
	However, no known hypervisor relies on this
	feature bit being acknowledged unconditionally.
	Thus, it's safe for drivers to ignore the TELL_HOST
	feature.


> +\end_layout
> +
> +\end_inset
> +
> +
> +\change_unchanged
> +
>  \end_layout
>  
>  \begin_layout Description
> @@ -7382,9 +7417,15 @@ The driver constructs an array of addresses of memory pages it has previously
>  \end_layout
>  
>  \begin_layout Enumerate
> -If the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is set, the guest may not
> - use these requested pages until that descriptor in the deflateq has been
> - used by the device.
> +If the VIRTIO_BALLOON_F_
> +\change_deleted 1531152142 1369761770
> +MUST
> +\change_inserted 1531152142 1369761770
> +CAN
> +\change_unchanged
> +_TELL_HOST feature is set, the guest may not use these requested pages until
> + that descriptor in the deflateq has been used by the device.

Not if it is set. If it's *negotiated*.

> +
>  \end_layout
>  
>  \begin_layout Enumerate
> @@ -7396,10 +7437,21 @@ status open
>  
>  \begin_layout Plain Layout
>  In this case, deflation advice is merely a courtesy
> +\change_inserted 1531152142 1369761798
> +.
> + The guest need not use the deflateq at all.
> +\change_unchanged
> +
>  \end_layout
>  
>  \end_inset
>  
> +
> +\change_inserted 1531152142 1369761801
> + If the host does not support this, it should not do anything when the balloon
> + is inflated or deflated, except put the descriptors on the used ring.

Not true. It simply can't rely on balloon to tell it
before pages are used. But, just as an example,
we can make kvm exit to qemu when guest
attempts to use such a page, and then we'll
know it is used without an explicit notification.

> +
> +\change_unchanged
>   
>  \end_layout
>  
> -- 
> 1.8.2.1
> 

Re: [PATCH v2 1/2] virtio-balloon spec: rewrite description of VIRTIO_BALLOON_F_MUST_TELL_HOST

[Paolo Bonzini]

Il 28/05/2013 20:15, Michael S. Tsirkin ha scritto:
> What you write in spec below and what you write above seems to
> contradict.
> 
> Look, how about the simpler patch that I sent:
> "[PATCH] virtio-spec: balloon: MUST_TELL_HOST is optional"
> 
> does it functionally do everything you want in this patch?

No, though it is a step forward.

But let's take a step back instead; here are the requirements:

1) old drivers work unmodified on new hosts

2) new drivers work unmodified on old guests, though it is okay if they
do not use silent deflation

3) if both the balloon and the driver supports silent deflation, it
should be used

4) if the balloon has to enter a "degraded" mode of operation when
guests do not "tell first", this has to happen only for old guests


Once SILENT_DEFLATE is added, your patch prevents fulfilling
requirements (3) and (4) together, at least in a Linux driver.

Basically, you're saying that the driver should set MUST_TELL_HOST to
!SILENT_DEFLATE.  However, in the Linux virtio implementation, features
are independent, and the feature list is told beforehand by the driver
to the virtio layer; with your proposal, the driver would have to
"retract" support for MUST_TELL_HOST after it has negotiated it.

This is why I'm making SILENT_DEFLATE=1 override MUST_TELL_HOST=1.

> If yes maybe you could pick that up, and add
> a patch with renames and text clarifications on top?
> 
> More comments below.
> 
>> ---
>>  virtio-spec.lyx | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 57 insertions(+), 5 deletions(-)
>>
>> diff --git a/virtio-spec.lyx b/virtio-spec.lyx
>> index adec0a5..5c76a87 100644
>> --- a/virtio-spec.lyx
>> +++ b/virtio-spec.lyx
>> @@ -7219,11 +7219,46 @@ bits
>>  
>>  \begin_deeper
>>  \begin_layout Description
>> -VIRTIO_BALLOON_F_MUST_TELL_HOST
>> +VIRTIO_BALLOON_F_
>> +\change_deleted 1531152142 1347020601
>> +MUST
>> +\change_inserted 1531152142 1347020602
>> +CAN
>> +\change_unchanged
>> +_TELL_HOST
>>  \begin_inset space ~
>>  \end_inset
>>  
>> -(0) Host must be told before pages from the balloon are used.
>> +(0) 
>> +\change_deleted 1531152142 1347020625
>> +Host must be told
>> +\change_inserted 1531152142 1347020617
>> +Guest is able to tell host
>> +\change_unchanged
>> + before pages from the balloon are used.
>> +
> 
> This "can" and "is able" is IMO more confusing than clarifying.
> Let's be definite. If feature is negotiated, guest must tell host.
> If it's not, it does not.
> 
> That's why it's MUST not CAN.

No, that won't fly.  First of all, it's not how other features work.  A
guest that asks for offloading can still do all the segmentation and
checksumming itself, a guest that knows about the virtio-net control
queue can still not use it, etc.

Second, this prevents fulfilling requirements (3) and (4) together as I
wrote above.

> And text should be
> 
> "When negotiated, guest tells host
> before pages from the balloon are used.
> "
> 
> 
>> +\change_inserted 1531152142 1368005603
>> + The host must propose this feature
> 
> We don't say "propose feature" anyway in the spec.
> You really mean "set this feature bit" ?

Yes.

>> if it has to be told
>> + before pages from the balloon are used.
> 
> Has to?  Not at all. It can't assume anything
> until guest has negotiated the feature.
> So it should be
> "if it wants to be told before pages from the balloon are used".

There can be implementations where the only choice is providing a fake
balloon.  For example if the page is removed from the IOMMU page tables,
you have to tell the host before a DMA access.

In this case, saying "has to be told" is not entirely precise, but
"wants to be told" does not say that the operation is degraded.  Any
improvements on the wording are welcome.

In the previous version I just said that all hosts should set this
feature bit (and commented that old hosts can still be
upwards-compatible).  I changed it because you didn't like it, but I
think the change is for the worse.  I still really prefer the version I
sent on May 8th.

>> +\begin_inset Foot
>> +status open
>> +
>> +\begin_layout Plain Layout
>> +
>> +\change_inserted 1531152142 1347022389
>> +This feature used to be named VIRTIO_BALLOON_F_\SpecialChar \-
>> +MUST_TELL_HOST.
>> + However, after a few years it was observed that drivers were not using
>> + it as specified.
>> + The virtio-balloon spec was then adjusted to what the drivers had been
>> + doing.
> 
> I'm not sure what good does this historical note do us.
> I would say:
> 	Historically the spec required all drivers
> 	to acknowledge this feature bit.
> 	However, no known hypervisor relies on this
> 	feature bit being acknowledged unconditionally.
> 	Thus, it's safe for drivers to ignore the TELL_HOST
> 	feature.

Ok.

> 
> 
>> +\end_layout
>> +
>> +\end_inset
>> +
>> +
>> +\change_unchanged
>> +
>>  \end_layout
>>  
>>  \begin_layout Description
>> @@ -7382,9 +7417,15 @@ The driver constructs an array of addresses of memory pages it has previously
>>  \end_layout
>>  
>>  \begin_layout Enumerate
>> -If the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is set, the guest may not
>> - use these requested pages until that descriptor in the deflateq has been
>> - used by the device.
>> +If the VIRTIO_BALLOON_F_
>> +\change_deleted 1531152142 1369761770
>> +MUST
>> +\change_inserted 1531152142 1369761770
>> +CAN
>> +\change_unchanged
>> +_TELL_HOST feature is set, the guest may not use these requested pages until
>> + that descriptor in the deflateq has been used by the device.
> 
> Not if it is set. If it's *negotiated*.

Right.

>> +
>>  \end_layout
>>  
>>  \begin_layout Enumerate
>> @@ -7396,10 +7437,21 @@ status open
>>  
>>  \begin_layout Plain Layout
>>  In this case, deflation advice is merely a courtesy
>> +\change_inserted 1531152142 1369761798
>> +.
>> + The guest need not use the deflateq at all.
>> +\change_unchanged
>> +
>>  \end_layout
>>  
>>  \end_inset
>>  
>> +
>> +\change_inserted 1531152142 1369761801
>> + If the host does not support this, it should not do anything when the balloon
>> + is inflated or deflated, except put the descriptors on the used ring.
> 
> Not true. It simply can't rely on balloon to tell it
> before pages are used. But, just as an example,
> we can make kvm exit to qemu when guest
> attempts to use such a page, and then we'll
> know it is used without an explicit notification.

Nice, but this is not always possible (I gave an example above).

Paolo

>> +
>> +\change_unchanged
>>   
>>  \end_layout
>>  
>> -- 
>> 1.8.2.1
>>

Re: [PATCH v2 1/2] virtio-balloon spec: rewrite description of VIRTIO_BALLOON_F_MUST_TELL_HOST

[Michael S. Tsirkin]

On Wed, May 29, 2013 at 08:24:10AM +0200, Paolo Bonzini wrote:
> Il 28/05/2013 20:15, Michael S. Tsirkin ha scritto:
> > What you write in spec below and what you write above seems to
> > contradict.
> > 
> > Look, how about the simpler patch that I sent:
> > "[PATCH] virtio-spec: balloon: MUST_TELL_HOST is optional"
> > 
> > does it functionally do everything you want in this patch?
> 
> No, though it is a step forward.
> 
> But let's take a step back instead; here are the requirements:
> 
> 1) old drivers work unmodified on new hosts
> 
> 2) new drivers work unmodified on old guests, though it is okay if they
> do not use silent deflation
> 
> 3) if both the balloon and the driver supports silent deflation, it
> should be used

I don't agree with this one.  We used to have this:
bf50e69f63d21091e525185c3ae761412be0ba72
and we dropped this patch.

> 4) if the balloon has to enter a "degraded" mode of operation when
> guests do not "tell first", this has to happen only for old guests

I don't agree with this either.
I think if we make tell host optional, it's up to the guest whether
to tell host.

> 
> Once SILENT_DEFLATE is added, your patch prevents fulfilling
> requirements (3) and (4) together, at least in a Linux driver.

Let's keep adding new bits out of it, we'll discuss patch 2
when we are done with patch 1.

> Basically, you're saying that the driver should set MUST_TELL_HOST to
> !SILENT_DEFLATE.

No, I am saying if you don't want to tell host do not ack
MUST_TELL_HOST.

>  However, in the Linux virtio implementation, features
> are independent,

Not just in the implementation. We try to keep it like this in the spec
too. One feature overriding another is messy.

> and the feature list is told beforehand by the driver
> to the virtio layer; with your proposal, the driver would have to
> "retract" support for MUST_TELL_HOST after it has negotiated it.

And that's planned by design - we just never had the need to
do this before.

You are confused by the implementation. Look at the spec.
Also look at commit logs for c45a6816c19dee67b8f725e6646d428901a6dc24
and c624896e488ba2bff5ae497782cfb265c8b00646.
Specifically:
    Drivers can still remove feature bits in their probe routine if they
    really have to.


> This is why I'm making SILENT_DEFLATE=1 override MUST_TELL_HOST=1.

And that's messy I think. It is cleaner if features are independent.


> > If yes maybe you could pick that up, and add
> > a patch with renames and text clarifications on top?
> > 
> > More comments below.
> > 
> >> ---
> >>  virtio-spec.lyx | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
> >>  1 file changed, 57 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> >> index adec0a5..5c76a87 100644
> >> --- a/virtio-spec.lyx
> >> +++ b/virtio-spec.lyx
> >> @@ -7219,11 +7219,46 @@ bits
> >>  
> >>  \begin_deeper
> >>  \begin_layout Description
> >> -VIRTIO_BALLOON_F_MUST_TELL_HOST
> >> +VIRTIO_BALLOON_F_
> >> +\change_deleted 1531152142 1347020601
> >> +MUST
> >> +\change_inserted 1531152142 1347020602
> >> +CAN
> >> +\change_unchanged
> >> +_TELL_HOST
> >>  \begin_inset space ~
> >>  \end_inset
> >>  
> >> -(0) Host must be told before pages from the balloon are used.
> >> +(0) 
> >> +\change_deleted 1531152142 1347020625
> >> +Host must be told
> >> +\change_inserted 1531152142 1347020617
> >> +Guest is able to tell host
> >> +\change_unchanged
> >> + before pages from the balloon are used.
> >> +
> > 
> > This "can" and "is able" is IMO more confusing than clarifying.
> > Let's be definite. If feature is negotiated, guest must tell host.
> > If it's not, it does not.
> > 
> > That's why it's MUST not CAN.
> 
> No, that won't fly.  First of all, it's not how other features work.  A
> guest that asks for offloading can still do all the segmentation and
> checksumming itself, a guest that knows about the virtio-net control
> queue can still not use it, etc.

Check out VIRTIO_NET_F_MRG_RXBUF as an example.
Same for VIRTIO_NET_F_GUEST_ANNOUNCE.
I'm sure there are other examples.

Basically a feature that's acked can change behaviour
in any way.

> Second, this prevents fulfilling requirements (3) and (4) together as I
> wrote above.

But where do these requirements come from?

> > And text should be
> > 
> > "When negotiated, guest tells host
> > before pages from the balloon are used.
> > "
> > 
> > 
> >> +\change_inserted 1531152142 1368005603
> >> + The host must propose this feature
> > 
> > We don't say "propose feature" anyway in the spec.
> > You really mean "set this feature bit" ?
> 
> Yes.
> 
> >> if it has to be told
> >> + before pages from the balloon are used.
> > 
> > Has to?  Not at all. It can't assume anything
> > until guest has negotiated the feature.
> > So it should be
> > "if it wants to be told before pages from the balloon are used".
> 
> There can be implementations where the only choice is providing a fake
> balloon.  For example if the page is removed from the IOMMU page tables,
> you have to tell the host before a DMA access.

Well this means that if guest did not ack TELL_HOST, host can't unmap
the page from IOMMU. Thus, such a host should probably set TELL_HOST,
and hope that guest acks it. But it's still up to the guest.

> In this case, saying "has to be told" is not entirely precise, but
> "wants to be told" does not say that the operation is degraded.  Any
> improvements on the wording are welcome.

Well I think if host "wants" something then that's exactly because
it can optimize it better.
If you like, add "host should set this feature bit if
being notified before page reuse allows some host-side
optimizations, for example conserving memory".

> In the previous version I just said that all hosts should set this
> feature bit (and commented that old hosts can still be
> upwards-compatible).
>  I changed it because you didn't like it, but I
> think the change is for the worse.  I still really prefer the version I
> sent on May 8th.

Then you get into a mess with yet another feature bit
which overrides an old feature bit.

Stop thinking about your new SILENT_DEFLATE for a moment,
that's what confuses I think. Look at this change in isolation.

> 
> >> +\begin_inset Foot
> >> +status open
> >> +
> >> +\begin_layout Plain Layout
> >> +
> >> +\change_inserted 1531152142 1347022389
> >> +This feature used to be named VIRTIO_BALLOON_F_\SpecialChar \-
> >> +MUST_TELL_HOST.
> >> + However, after a few years it was observed that drivers were not using
> >> + it as specified.
> >> + The virtio-balloon spec was then adjusted to what the drivers had been
> >> + doing.
> > 
> > I'm not sure what good does this historical note do us.
> > I would say:
> > 	Historically the spec required all drivers
> > 	to acknowledge this feature bit.
> > 	However, no known hypervisor relies on this
> > 	feature bit being acknowledged unconditionally.
> > 	Thus, it's safe for drivers to ignore the TELL_HOST
> > 	feature.
> 
> Ok.
> 
> > 
> > 
> >> +\end_layout
> >> +
> >> +\end_inset
> >> +
> >> +
> >> +\change_unchanged
> >> +
> >>  \end_layout
> >>  
> >>  \begin_layout Description
> >> @@ -7382,9 +7417,15 @@ The driver constructs an array of addresses of memory pages it has previously
> >>  \end_layout
> >>  
> >>  \begin_layout Enumerate
> >> -If the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is set, the guest may not
> >> - use these requested pages until that descriptor in the deflateq has been
> >> - used by the device.
> >> +If the VIRTIO_BALLOON_F_
> >> +\change_deleted 1531152142 1369761770
> >> +MUST
> >> +\change_inserted 1531152142 1369761770
> >> +CAN
> >> +\change_unchanged
> >> +_TELL_HOST feature is set, the guest may not use these requested pages until
> >> + that descriptor in the deflateq has been used by the device.
> > 
> > Not if it is set. If it's *negotiated*.
> 
> Right.
> 
> >> +
> >>  \end_layout
> >>  
> >>  \begin_layout Enumerate
> >> @@ -7396,10 +7437,21 @@ status open
> >>  
> >>  \begin_layout Plain Layout
> >>  In this case, deflation advice is merely a courtesy
> >> +\change_inserted 1531152142 1369761798
> >> +.
> >> + The guest need not use the deflateq at all.
> >> +\change_unchanged
> >> +
> >>  \end_layout
> >>  
> >>  \end_inset
> >>  
> >> +
> >> +\change_inserted 1531152142 1369761801
> >> + If the host does not support this, it should not do anything when the balloon
> >> + is inflated or deflated, except put the descriptors on the used ring.
> > 
> > Not true. It simply can't rely on balloon to tell it
> > before pages are used. But, just as an example,
> > we can make kvm exit to qemu when guest
> > attempts to use such a page, and then we'll
> > know it is used without an explicit notification.
> 
> Nice, but this is not always possible (I gave an example above).
> 
> Paolo

Some hosts *might* not do anything when the balloon
is inflated or deflated. But should above is still wrong.

> >> +
> >> +\change_unchanged
> >>   
> >>  \end_layout
> >>  
> >> -- 
> >> 1.8.2.1
> >>

Re: [PATCH v2 1/2] virtio-balloon spec: rewrite description of VIRTIO_BALLOON_F_MUST_TELL_HOST

[Paolo Bonzini]

Il 29/05/2013 09:45, Michael S. Tsirkin ha scritto:
> On Wed, May 29, 2013 at 08:24:10AM +0200, Paolo Bonzini wrote:
>> Il 28/05/2013 20:15, Michael S. Tsirkin ha scritto:
>>> What you write in spec below and what you write above seems to
>>> contradict.
>>>
>>> Look, how about the simpler patch that I sent:
>>> "[PATCH] virtio-spec: balloon: MUST_TELL_HOST is optional"
>>>
>>> does it functionally do everything you want in this patch?
>>
>> No, though it is a step forward.
>>
>> But let's take a step back instead; here are the requirements:
>>
>> 1) old drivers work unmodified on new hosts
>>
>> 2) new drivers work unmodified on old guests, though it is okay if they
>> do not use silent deflation
>>
>> 3) if both the balloon and the driver supports silent deflation, it
>> should be used
> 
> I don't agree with this one.  We used to have this:
> bf50e69f63d21091e525185c3ae761412be0ba72
> and we dropped this patch.

I think it's telling that none of the benefits in the commit message
were ever reaped.  In fact they don't matter if you do not tell the host
at all.

>> 4) if the balloon has to enter a "degraded" mode of operation when
>> guests do not "tell first", this has to happen only for old guests
> 
> I don't agree with this either.
> I think if we make tell host optional, it's up to the guest whether
> to tell host.

Yes, let me rephrase: if the balloon has to enter a "degraded" mode of
operation when guests do not "tell first", new guests must have a way to
know that they should "tell first" and avoid that.

>> Basically, you're saying that the driver should set MUST_TELL_HOST to
>> !SILENT_DEFLATE.
> 
> No, I am saying if you don't want to tell host do not ack
> MUST_TELL_HOST.

I want to tell host _if it is necessary to do so_.  I don't want to tell
host if it can be avoided.

>>  However, in the Linux virtio implementation, features
>> are independent,
> 
> Not just in the implementation. We try to keep it like this in the spec
> too. One feature overriding another is messy.

There is at least one such case already.  VIRTIO_NET_F_HOST_TSO4
requires the guest to use large buffers, _unless_ VIRTIO_NET_F_MRG_RXBUF
is also negotiated.

Note that in the May 8th version there was no such overriding.  In that
version communication was unidirectional, host->guest for SILENT_DEFLATE
and guest->host for CAN_TELL_HOST.  SILENT_DEFLATE is set if the host
doesn't really care about CAN_TELL_HOST.  It is much much simpler.

>> and the feature list is told beforehand by the driver
>> to the virtio layer; with your proposal, the driver would have to
>> "retract" support for MUST_TELL_HOST after it has negotiated it.
> 
> And that's planned by design - we just never had the need to
> do this before.

And that's exactly why I want to make the feature bits unidirectional.
The guest tells the host what it can do, the host tells the guest what
it can accept.  The guest then acts based on what the host told it.

> You are confused by the implementation. Look at the spec.
> Also look at commit logs for c45a6816c19dee67b8f725e6646d428901a6dc24
> and c624896e488ba2bff5ae497782cfb265c8b00646.
> Specifically:
>     Drivers can still remove feature bits in their probe routine if they
>     really have to.

How so? virtio.c has this:

        dev->config->finalize_features(dev);
        err = drv->probe(dev);

>> This is why I'm making SILENT_DEFLATE=1 override MUST_TELL_HOST=1.
> 
> And that's messy I think. It is cleaner if features are independent.

In the original wording, they are independent.  I messed it up because
you wanted split patches.

>> In this case, saying "has to be told" is not entirely precise, but
>> "wants to be told" does not say that the operation is degraded.  Any
>> improvements on the wording are welcome.
> 
> Well I think if host "wants" something then that's exactly because
> it can optimize it better.
> If you like, add "host should set this feature bit if
> being notified before page reuse allows some host-side
> optimizations, for example conserving memory".

Ok.

>> In the previous version I just said that all hosts should set this
>> feature bit (and commented that old hosts can still be
>> upwards-compatible).
>>  I changed it because you didn't like it, but I
>> think the change is for the worse.  I still really prefer the version I
>> sent on May 8th.
> 
> Then you get into a mess with yet another feature bit
> which overrides an old feature bit.
> 
> Stop thinking about your new SILENT_DEFLATE for a moment,
> that's what confuses I think. Look at this change in isolation.

I am trying, but at the same time I don't want to put myself in the corner.

> Some hosts *might* not do anything when the balloon
> is inflated or deflated. But should above is still wrong.

Ok.

I'll try making a patch with the old feature bit only, and with the
assumption that all hosts support silent deflate.  Let's see what we
come up with.

Paolo

Re: [PATCH v2 1/2] virtio-balloon spec: rewrite description of VIRTIO_BALLOON_F_MUST_TELL_HOST

[Paolo Bonzini]

Il 29/05/2013 09:45, Michael S. Tsirkin ha scritto:
> On Wed, May 29, 2013 at 08:24:10AM +0200, Paolo Bonzini wrote:
>> Il 28/05/2013 20:15, Michael S. Tsirkin ha scritto:
>>> What you write in spec below and what you write above seems to
>>> contradict.
>>>
>>> Look, how about the simpler patch that I sent:
>>> "[PATCH] virtio-spec: balloon: MUST_TELL_HOST is optional"
>>>
>>> does it functionally do everything you want in this patch?
>>
>> No, though it is a step forward.
>>
>> But let's take a step back instead; here are the requirements:
>>
>> 1) old drivers work unmodified on new hosts
>>
>> 2) new drivers work unmodified on old guests, though it is okay if they
>> do not use silent deflation
>>
>> 3) if both the balloon and the driver supports silent deflation, it
>> should be used
> 
> I don't agree with this one.  We used to have this:
> bf50e69f63d21091e525185c3ae761412be0ba72
> and we dropped this patch.

I think it's telling that none of the benefits in the commit message
were ever reaped.  In fact they don't matter if you do not tell the host
at all.

>> 4) if the balloon has to enter a "degraded" mode of operation when
>> guests do not "tell first", this has to happen only for old guests
> 
> I don't agree with this either.
> I think if we make tell host optional, it's up to the guest whether
> to tell host.

Yes, let me rephrase: if the balloon has to enter a "degraded" mode of
operation when guests do not "tell first", new guests must have a way to
know that they should "tell first" and avoid that.

>> Basically, you're saying that the driver should set MUST_TELL_HOST to
>> !SILENT_DEFLATE.
> 
> No, I am saying if you don't want to tell host do not ack
> MUST_TELL_HOST.

I want to tell host _if it is necessary to do so_.  I don't want to tell
host if it can be avoided.

>>  However, in the Linux virtio implementation, features
>> are independent,
> 
> Not just in the implementation. We try to keep it like this in the spec
> too. One feature overriding another is messy.

There is at least one such case already.  VIRTIO_NET_F_HOST_TSO4
requires the guest to use large buffers, _unless_ VIRTIO_NET_F_MRG_RXBUF
is also negotiated.

Note that in the May 8th version there was no such overriding.  In that
version communication was unidirectional, host->guest for SILENT_DEFLATE
and guest->host for CAN_TELL_HOST.  SILENT_DEFLATE is set if the host
doesn't really care about CAN_TELL_HOST.  It is much much simpler.

>> and the feature list is told beforehand by the driver
>> to the virtio layer; with your proposal, the driver would have to
>> "retract" support for MUST_TELL_HOST after it has negotiated it.
> 
> And that's planned by design - we just never had the need to
> do this before.

And that's exactly why I want to make the feature bits unidirectional.
The guest tells the host what it can do, the host tells the guest what
it can accept.  The guest then acts based on what the host told it.

> You are confused by the implementation. Look at the spec.
> Also look at commit logs for c45a6816c19dee67b8f725e6646d428901a6dc24
> and c624896e488ba2bff5ae497782cfb265c8b00646.
> Specifically:
>     Drivers can still remove feature bits in their probe routine if they
>     really have to.

How so? virtio.c has this:

        dev->config->finalize_features(dev);
        err = drv->probe(dev);

>> This is why I'm making SILENT_DEFLATE=1 override MUST_TELL_HOST=1.
> 
> And that's messy I think. It is cleaner if features are independent.

In the original wording, they are independent.  I messed it up because
you wanted split patches.

>> In this case, saying "has to be told" is not entirely precise, but
>> "wants to be told" does not say that the operation is degraded.  Any
>> improvements on the wording are welcome.
> 
> Well I think if host "wants" something then that's exactly because
> it can optimize it better.
> If you like, add "host should set this feature bit if
> being notified before page reuse allows some host-side
> optimizations, for example conserving memory".

Ok.

>> In the previous version I just said that all hosts should set this
>> feature bit (and commented that old hosts can still be
>> upwards-compatible).
>>  I changed it because you didn't like it, but I
>> think the change is for the worse.  I still really prefer the version I
>> sent on May 8th.
> 
> Then you get into a mess with yet another feature bit
> which overrides an old feature bit.
> 
> Stop thinking about your new SILENT_DEFLATE for a moment,
> that's what confuses I think. Look at this change in isolation.

I am trying, but at the same time I don't want to put myself in the corner.

> Some hosts *might* not do anything when the balloon
> is inflated or deflated. But should above is still wrong.

Ok.

I'll try making a patch with the old feature bit only, and with the
assumption that all hosts support silent deflate.  Let's see what we
come up with.

Paolo

Re: [PATCH v2 1/2] virtio-balloon spec: rewrite description of VIRTIO_BALLOON_F_MUST_TELL_HOST

[Paolo Bonzini]

Il 28/05/2013 20:15, Michael S. Tsirkin ha scritto:
> What you write in spec below and what you write above seems to
> contradict.
> 
> Look, how about the simpler patch that I sent:
> "[PATCH] virtio-spec: balloon: MUST_TELL_HOST is optional"
> 
> does it functionally do everything you want in this patch?

No, though it is a step forward.

But let's take a step back instead; here are the requirements:

1) old drivers work unmodified on new hosts

2) new drivers work unmodified on old guests, though it is okay if they
do not use silent deflation

3) if both the balloon and the driver supports silent deflation, it
should be used

4) if the balloon has to enter a "degraded" mode of operation when
guests do not "tell first", this has to happen only for old guests


Once SILENT_DEFLATE is added, your patch prevents fulfilling
requirements (3) and (4) together, at least in a Linux driver.

Basically, you're saying that the driver should set MUST_TELL_HOST to
!SILENT_DEFLATE.  However, in the Linux virtio implementation, features
are independent, and the feature list is told beforehand by the driver
to the virtio layer; with your proposal, the driver would have to
"retract" support for MUST_TELL_HOST after it has negotiated it.

This is why I'm making SILENT_DEFLATE=1 override MUST_TELL_HOST=1.

> If yes maybe you could pick that up, and add
> a patch with renames and text clarifications on top?
> 
> More comments below.
> 
>> ---
>>  virtio-spec.lyx | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 57 insertions(+), 5 deletions(-)
>>
>> diff --git a/virtio-spec.lyx b/virtio-spec.lyx
>> index adec0a5..5c76a87 100644
>> --- a/virtio-spec.lyx
>> +++ b/virtio-spec.lyx
>> @@ -7219,11 +7219,46 @@ bits
>>  
>>  \begin_deeper
>>  \begin_layout Description
>> -VIRTIO_BALLOON_F_MUST_TELL_HOST
>> +VIRTIO_BALLOON_F_
>> +\change_deleted 1531152142 1347020601
>> +MUST
>> +\change_inserted 1531152142 1347020602
>> +CAN
>> +\change_unchanged
>> +_TELL_HOST
>>  \begin_inset space ~
>>  \end_inset
>>  
>> -(0) Host must be told before pages from the balloon are used.
>> +(0) 
>> +\change_deleted 1531152142 1347020625
>> +Host must be told
>> +\change_inserted 1531152142 1347020617
>> +Guest is able to tell host
>> +\change_unchanged
>> + before pages from the balloon are used.
>> +
> 
> This "can" and "is able" is IMO more confusing than clarifying.
> Let's be definite. If feature is negotiated, guest must tell host.
> If it's not, it does not.
> 
> That's why it's MUST not CAN.

No, that won't fly.  First of all, it's not how other features work.  A
guest that asks for offloading can still do all the segmentation and
checksumming itself, a guest that knows about the virtio-net control
queue can still not use it, etc.

Second, this prevents fulfilling requirements (3) and (4) together as I
wrote above.

> And text should be
> 
> "When negotiated, guest tells host
> before pages from the balloon are used.
> "
> 
> 
>> +\change_inserted 1531152142 1368005603
>> + The host must propose this feature
> 
> We don't say "propose feature" anyway in the spec.
> You really mean "set this feature bit" ?

Yes.

>> if it has to be told
>> + before pages from the balloon are used.
> 
> Has to?  Not at all. It can't assume anything
> until guest has negotiated the feature.
> So it should be
> "if it wants to be told before pages from the balloon are used".

There can be implementations where the only choice is providing a fake
balloon.  For example if the page is removed from the IOMMU page tables,
you have to tell the host before a DMA access.

In this case, saying "has to be told" is not entirely precise, but
"wants to be told" does not say that the operation is degraded.  Any
improvements on the wording are welcome.

In the previous version I just said that all hosts should set this
feature bit (and commented that old hosts can still be
upwards-compatible).  I changed it because you didn't like it, but I
think the change is for the worse.  I still really prefer the version I
sent on May 8th.

>> +\begin_inset Foot
>> +status open
>> +
>> +\begin_layout Plain Layout
>> +
>> +\change_inserted 1531152142 1347022389
>> +This feature used to be named VIRTIO_BALLOON_F_\SpecialChar \-
>> +MUST_TELL_HOST.
>> + However, after a few years it was observed that drivers were not using
>> + it as specified.
>> + The virtio-balloon spec was then adjusted to what the drivers had been
>> + doing.
> 
> I'm not sure what good does this historical note do us.
> I would say:
> 	Historically the spec required all drivers
> 	to acknowledge this feature bit.
> 	However, no known hypervisor relies on this
> 	feature bit being acknowledged unconditionally.
> 	Thus, it's safe for drivers to ignore the TELL_HOST
> 	feature.

Ok.

> 
> 
>> +\end_layout
>> +
>> +\end_inset
>> +
>> +
>> +\change_unchanged
>> +
>>  \end_layout
>>  
>>  \begin_layout Description
>> @@ -7382,9 +7417,15 @@ The driver constructs an array of addresses of memory pages it has previously
>>  \end_layout
>>  
>>  \begin_layout Enumerate
>> -If the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is set, the guest may not
>> - use these requested pages until that descriptor in the deflateq has been
>> - used by the device.
>> +If the VIRTIO_BALLOON_F_
>> +\change_deleted 1531152142 1369761770
>> +MUST
>> +\change_inserted 1531152142 1369761770
>> +CAN
>> +\change_unchanged
>> +_TELL_HOST feature is set, the guest may not use these requested pages until
>> + that descriptor in the deflateq has been used by the device.
> 
> Not if it is set. If it's *negotiated*.

Right.

>> +
>>  \end_layout
>>  
>>  \begin_layout Enumerate
>> @@ -7396,10 +7437,21 @@ status open
>>  
>>  \begin_layout Plain Layout
>>  In this case, deflation advice is merely a courtesy
>> +\change_inserted 1531152142 1369761798
>> +.
>> + The guest need not use the deflateq at all.
>> +\change_unchanged
>> +
>>  \end_layout
>>  
>>  \end_inset
>>  
>> +
>> +\change_inserted 1531152142 1369761801
>> + If the host does not support this, it should not do anything when the balloon
>> + is inflated or deflated, except put the descriptors on the used ring.
> 
> Not true. It simply can't rely on balloon to tell it
> before pages are used. But, just as an example,
> we can make kvm exit to qemu when guest
> attempts to use such a page, and then we'll
> know it is used without an explicit notification.

Nice, but this is not always possible (I gave an example above).

Paolo

>> +
>> +\change_unchanged
>>   
>>  \end_layout
>>  
>> -- 
>> 1.8.2.1
>>

[PATCH v2 2/2] virtio-balloon spec: reintroduce "silent deflation" feature

[Paolo Bonzini]

The original idea of the VIRTIO_BALLOON_F_MUST_TELL_HOST feature was to
let drivers skip usage of the deflate queue when leaking the balloon
("silent deflation").  Guests may benefit from silent deflate by
aggressively inflating the balloon; they know that they will be able to
use ballooned pages without issuing a (blocking) request to the device.

The previous patch redefined the feature to ensure correctness of the
operation when drivers do not correctly report deflation.  This patch
adds back the optimization.

The new feature bit is for the host to tell the drivers if silent
deflation is actually supported.  The meaning of the feature bit is
reversed compared to the original, because the original meaning was
not safe against migration.

For features to be safe against migration, they have to be defined as
"this is true if the guest _can_ do X".  For such a "positive" feature,
migration is possible if the destination supports it, or the source
didn't set it:

    dest support      source set          ok?
          T                T              T
          T                F              T
          F                T              F
          F                F              T

Instead, the old feature was defined as "this is true if the guest
_cannot_ do X".  For such a "negative" feature, migration is possible
if the destination supports it, or the source sets it:

    dest support      source set          ok?
          T                T              T
          T                F              F
          F                T              T
          F                F              T

However, the negotiated features are supposed to be the AND of the
device- and driver-supported features.  In the F/T case, the feature
would be negotiated by the source as T, and become F when negotiated on
the destination.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 virtio-spec.lyx | 197 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 197 insertions(+)

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index 5c76a87..5da4fde 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -7267,6 +7267,20 @@ VIRTIO_BALLOON_F_STATS_VQ
 \end_inset
 
 (1) A virtqueue for reporting guest memory statistics is present.
+\change_inserted 1531152142 1347020627
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 1531152142 1347020648
+VIRTIO_BALLOON_F_SILENT_DEFLATE
+\begin_inset space ~
+\end_inset
+
+(2) Guest need not tell host before pages from the balloon are used.
+\change_unchanged
+
 \end_layout
 
 \end_deeper
@@ -7627,6 +7641,168 @@ VIRTIO_BALLOON_S_MEMFREE The amount of memory not being used for any purpose
 VIRTIO_BALLOON_S_MEMTOT The total amount of memory available (in bytes).
 \end_layout
 
+\begin_layout Description
+
+\change_inserted 1531152142 1368005515
+Silent
+\begin_inset space ~
+\end_inset
+
+deflation
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1531152142 1368005515
+
+\series medium
+Some implementation of the balloon device may not require the guest to deflate
+ the balloon explicitly; instead, the guest may just take a page from its
+ reserve and start using it.
+ This is called 
+\begin_inset Quotes eld
+\end_inset
+
+silent deflate
+\begin_inset Quotes erd
+\end_inset
+
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1531152142 1368005515
+In order to use this feature effectively, both the guest and the host need
+ to know how the other part intends to use the balloon.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1531152142 1368005515
+
+\series medium
+Guests may benefit from silent deflate by aggressively inflating the balloon;
+ they know that they will be able to use ballooned pages without issuing
+ a (blocking) request to the device.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1531152142 1368005515
+Knowing that the guest will 
+\emph on
+not
+\emph default
+ deflate silently also benefits the host.
+ For example, if the host is pinning the guest's memory, it may unpin ballooned
+ pages and pin them again upon deflation.
+ This allows cooperative memory overcommit even if the guest's memory is
+ pinned.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1531152142 1368005515
+Thus, there are two possibilities for the host (either it does not support
+ silent deflate, or it does), and three for the guest (it doesn't need silent
+ deflate, it may choose to use it if available, it requires it).
+ Because there are three possibilities for the guest, support for silent
+ deflate is represented by two different feature bits.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1531152142 1368005515
+The feature bits are used as follows:
+\end_layout
+
+\begin_layout Itemize
+
+\change_inserted 1531152142 1368005671
+if the host does not support silent deflate, it must propose the
+VIRTIO_\SpecialChar \-
+BALLOON_F_\SpecialChar \-
+CAN_\SpecialChar \-
+TELL_\SpecialChar \-
+HOST feature.
+ If the guest does 
+\emph on
+not
+\emph default
+ negotiate it, the host should assume that the guest will use silent deflate.
+ A host that does not support silent deflate then must do nothing on attempts
+ of the guest to deflate/inflate the balloon, except put the descriptors on
+ the used ring.
+\end_layout
+
+\begin_layout Itemize
+
+\change_inserted 1531152142 1368005515
+a guest that 
+\emph on
+must
+\emph default
+ use silent deflation does not need to negotiate any feature.
+\end_layout
+
+\begin_layout Itemize
+
+\change_inserted 1531152142 1368005676
+a guest that 
+\emph on
+will never
+\emph default
+ use silent deflation only has to propose VIRTIO_\SpecialChar \-
+BALLOON_F_\SpecialChar \-
+CAN_\SpecialChar \-
+TELL_\SpecialChar \-
+HOST.
+ It need not do anything if the host does not negotiate the feature.
+\end_layout
+
+\begin_layout Itemize
+
+\change_inserted 1531152142 1368005686
+a guest that 
+\emph on
+can optionally
+\emph default
+ use silent deflation should propose both VIRTIO_\SpecialChar \-
+BALLOON_F_\SpecialChar \-
+CAN_\SpecialChar \-
+TELL_\SpecialChar \-
+HOST
+ and VIRTIO_\SpecialChar \-
+BALLOON_F_\SpecialChar \-
+SILENT_\SpecialChar \-
+DEFLATE.
+ The guest driver can then use silent deflation if and only if the host
+ has negotiated VIRTIO_\SpecialChar \-
+BALLOON_F_\SpecialChar \-
+SILENT_\SpecialChar \-
+DEFLATE too.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1531152142 1368005690
+Hosts are free to propose the VIRTIO_\SpecialChar \-
+BALLOON_F_\SpecialChar \-
+CAN_\SpecialChar \-
+TELL_\SpecialChar \-
+HOST feature even if they support silent deflation.
+ It will
+ not affect operation of a well-behaving guest; such a guest
+ should never check if the host proposed the feature, and
+ should not assume that lack of this feature implies support
+ for silent deflation. The only bit to check for this purpose is
+ VIRTIO_\SpecialChar \-
+BALLOON_F_\SpecialChar \-
+SILENT_\SpecialChar \-
+DEFLATE.
+\end_layout
+
 \begin_layout Chapter*
 Appendix H: Rpmsg: Remote Processor Messaging
 \end_layout
-- 
1.8.2.1

Re: [PATCH v2 2/2] virtio-balloon spec: reintroduce "silent deflation" feature

[Michael S. Tsirkin]

On Tue, May 28, 2013 at 07:40:18PM +0200, Paolo Bonzini wrote:
> The original idea of the VIRTIO_BALLOON_F_MUST_TELL_HOST feature was to
> let drivers skip usage of the deflate queue when leaking the balloon
> ("silent deflation").  Guests may benefit from silent deflate by
> aggressively inflating the balloon; they know that they will be able to
> use ballooned pages without issuing a (blocking) request to the device.
> 
> The previous patch redefined the feature to ensure correctness of the
> operation when drivers do not correctly report deflation.  This patch
> adds back the optimization.
> 
> The new feature bit is for the host to tell the drivers if silent
> deflation is actually supported.  The meaning of the feature bit is
> reversed compared to the original, because the original meaning was
> not safe against migration.
> 
> For features to be safe against migration, they have to be defined as
> "this is true if the guest _can_ do X".  For such a "positive" feature,
> migration is possible if the destination supports it, or the source
> didn't set it:
> 
>     dest support      source set          ok?
>           T                T              T
>           T                F              T
>           F                T              F
>           F                F              T
> 
> Instead, the old feature was defined as "this is true if the guest
> _cannot_ do X".  For such a "negative" feature, migration is possible
> if the destination supports it, or the source sets it:
> 
>     dest support      source set          ok?
>           T                T              T
>           T                F              F
>           F                T              T
>           F                F              T
> 
> However, the negotiated features are supposed to be the AND of the
> device- and driver-supported features.  In the F/T case, the feature
> would be negotiated by the source as T, and become F when negotiated on
> the destination.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Do you have any numbers showing how this new feature improves
performance?
We are able to batch quite a lot of pages in a single deflate
request - is the overhead measureable in practice?

> ---
>  virtio-spec.lyx | 197 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 197 insertions(+)
> 
> diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> index 5c76a87..5da4fde 100644
> --- a/virtio-spec.lyx
> +++ b/virtio-spec.lyx
> @@ -7267,6 +7267,20 @@ VIRTIO_BALLOON_F_STATS_VQ
>  \end_inset
>  
>  (1) A virtqueue for reporting guest memory statistics is present.
> +\change_inserted 1531152142 1347020627
> +
> +\end_layout
> +
> +\begin_layout Description
> +
> +\change_inserted 1531152142 1347020648
> +VIRTIO_BALLOON_F_SILENT_DEFLATE
> +\begin_inset space ~
> +\end_inset
> +
> +(2) Guest need not tell host before pages from the balloon are used.
> +\change_unchanged
> +
>  \end_layout
>  
>  \end_deeper
> @@ -7627,6 +7641,168 @@ VIRTIO_BALLOON_S_MEMFREE The amount of memory not being used for any purpose
>  VIRTIO_BALLOON_S_MEMTOT The total amount of memory available (in bytes).
>  \end_layout
>  
> +\begin_layout Description
> +
> +\change_inserted 1531152142 1368005515
> +Silent
> +\begin_inset space ~
> +\end_inset
> +
> +deflation
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 1531152142 1368005515
> +
> +\series medium
> +Some implementation of the balloon device may not require the guest to deflate
> + the balloon explicitly; instead, the guest may just take a page from its
> + reserve and start using it.
> + This is called 
> +\begin_inset Quotes eld
> +\end_inset
> +
> +silent deflate
> +\begin_inset Quotes erd
> +\end_inset
> +
> +
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 1531152142 1368005515
> +In order to use this feature effectively, both the guest and the host need
> + to know how the other part intends to use the balloon.
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 1531152142 1368005515
> +
> +\series medium
> +Guests may benefit from silent deflate by aggressively inflating the balloon;
> + they know that they will be able to use ballooned pages without issuing
> + a (blocking) request to the device.
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 1531152142 1368005515
> +Knowing that the guest will 
> +\emph on
> +not
> +\emph default
> + deflate silently also benefits the host.
> + For example, if the host is pinning the guest's memory, it may unpin ballooned
> + pages and pin them again upon deflation.
> + This allows cooperative memory overcommit even if the guest's memory is
> + pinned.
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 1531152142 1368005515
> +Thus, there are two possibilities for the host (either it does not support
> + silent deflate, or it does), and three for the guest (it doesn't need silent
> + deflate, it may choose to use it if available, it requires it).
> + Because there are three possibilities for the guest, support for silent
> + deflate is represented by two different feature bits.
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 1531152142 1368005515
> +The feature bits are used as follows:
> +\end_layout
> +
> +\begin_layout Itemize
> +
> +\change_inserted 1531152142 1368005671
> +if the host does not support silent deflate, it must propose the
> +VIRTIO_\SpecialChar \-
> +BALLOON_F_\SpecialChar \-
> +CAN_\SpecialChar \-
> +TELL_\SpecialChar \-
> +HOST feature.
> + If the guest does 
> +\emph on
> +not
> +\emph default
> + negotiate it, the host should assume that the guest will use silent deflate.
> + A host that does not support silent deflate then must do nothing on attempts
> + of the guest to deflate/inflate the balloon, except put the descriptors on
> + the used ring.
> +\end_layout
> +
> +\begin_layout Itemize
> +
> +\change_inserted 1531152142 1368005515
> +a guest that 
> +\emph on
> +must
> +\emph default
> + use silent deflation does not need to negotiate any feature.
> +\end_layout
> +
> +\begin_layout Itemize
> +
> +\change_inserted 1531152142 1368005676
> +a guest that 
> +\emph on
> +will never
> +\emph default
> + use silent deflation only has to propose VIRTIO_\SpecialChar \-
> +BALLOON_F_\SpecialChar \-
> +CAN_\SpecialChar \-
> +TELL_\SpecialChar \-
> +HOST.
> + It need not do anything if the host does not negotiate the feature.
> +\end_layout
> +
> +\begin_layout Itemize
> +
> +\change_inserted 1531152142 1368005686
> +a guest that 
> +\emph on
> +can optionally
> +\emph default
> + use silent deflation should propose both VIRTIO_\SpecialChar \-
> +BALLOON_F_\SpecialChar \-
> +CAN_\SpecialChar \-
> +TELL_\SpecialChar \-
> +HOST
> + and VIRTIO_\SpecialChar \-
> +BALLOON_F_\SpecialChar \-
> +SILENT_\SpecialChar \-
> +DEFLATE.
> + The guest driver can then use silent deflation if and only if the host
> + has negotiated VIRTIO_\SpecialChar \-
> +BALLOON_F_\SpecialChar \-
> +SILENT_\SpecialChar \-
> +DEFLATE too.
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 1531152142 1368005690
> +Hosts are free to propose the VIRTIO_\SpecialChar \-
> +BALLOON_F_\SpecialChar \-
> +CAN_\SpecialChar \-
> +TELL_\SpecialChar \-
> +HOST feature even if they support silent deflation.
> + It will
> + not affect operation of a well-behaving guest; such a guest
> + should never check if the host proposed the feature, and
> + should not assume that lack of this feature implies support
> + for silent deflation. The only bit to check for this purpose is
> + VIRTIO_\SpecialChar \-
> +BALLOON_F_\SpecialChar \-
> +SILENT_\SpecialChar \-
> +DEFLATE.
> +\end_layout
> +
>  \begin_layout Chapter*
>  Appendix H: Rpmsg: Remote Processor Messaging
>  \end_layout
> -- 
> 1.8.2.1

Re: [PATCH v2 2/2] virtio-balloon spec: reintroduce "silent deflation" feature

[Paolo Bonzini]

Il 29/05/2013 09:49, Michael S. Tsirkin ha scritto:
> On Tue, May 28, 2013 at 07:40:18PM +0200, Paolo Bonzini wrote:
>> The original idea of the VIRTIO_BALLOON_F_MUST_TELL_HOST feature was to
>> let drivers skip usage of the deflate queue when leaking the balloon
>> ("silent deflation").  Guests may benefit from silent deflate by
>> aggressively inflating the balloon; they know that they will be able to
>> use ballooned pages without issuing a (blocking) request to the device.
>>
>> The previous patch redefined the feature to ensure correctness of the
>> operation when drivers do not correctly report deflation.  This patch
>> adds back the optimization.
>>
>> The new feature bit is for the host to tell the drivers if silent
>> deflation is actually supported.  The meaning of the feature bit is
>> reversed compared to the original, because the original meaning was
>> not safe against migration.
>>
>> For features to be safe against migration, they have to be defined as
>> "this is true if the guest _can_ do X".  For such a "positive" feature,
>> migration is possible if the destination supports it, or the source
>> didn't set it:
>>
>>     dest support      source set          ok?
>>           T                T              T
>>           T                F              T
>>           F                T              F
>>           F                F              T
>>
>> Instead, the old feature was defined as "this is true if the guest
>> _cannot_ do X".  For such a "negative" feature, migration is possible
>> if the destination supports it, or the source sets it:
>>
>>     dest support      source set          ok?
>>           T                T              T
>>           T                F              F
>>           F                T              T
>>           F                F              T
>>
>> However, the negotiated features are supposed to be the AND of the
>> device- and driver-supported features.  In the F/T case, the feature
>> would be negotiated by the source as T, and become F when negotiated on
>> the destination.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Do you have any numbers showing how this new feature improves
> performance?
> We are able to batch quite a lot of pages in a single deflate
> request - is the overhead measureable in practice?

It's not only about better times, but also about better algorithms.  I
started writing this after seeing the Google fileballoon driver.  For
that implementation, the deflateq cannot be used at all.

Paolo

Re: [PATCH v2 2/2] virtio-balloon spec: reintroduce "silent deflation" feature

[Michael S. Tsirkin]

On Wed, May 29, 2013 at 10:47:08AM +0200, Paolo Bonzini wrote:
> Il 29/05/2013 09:49, Michael S. Tsirkin ha scritto:
> > On Tue, May 28, 2013 at 07:40:18PM +0200, Paolo Bonzini wrote:
> >> The original idea of the VIRTIO_BALLOON_F_MUST_TELL_HOST feature was to
> >> let drivers skip usage of the deflate queue when leaking the balloon
> >> ("silent deflation").  Guests may benefit from silent deflate by
> >> aggressively inflating the balloon; they know that they will be able to
> >> use ballooned pages without issuing a (blocking) request to the device.
> >>
> >> The previous patch redefined the feature to ensure correctness of the
> >> operation when drivers do not correctly report deflation.  This patch
> >> adds back the optimization.
> >>
> >> The new feature bit is for the host to tell the drivers if silent
> >> deflation is actually supported.  The meaning of the feature bit is
> >> reversed compared to the original, because the original meaning was
> >> not safe against migration.
> >>
> >> For features to be safe against migration, they have to be defined as
> >> "this is true if the guest _can_ do X".  For such a "positive" feature,
> >> migration is possible if the destination supports it, or the source
> >> didn't set it:
> >>
> >>     dest support      source set          ok?
> >>           T                T              T
> >>           T                F              T
> >>           F                T              F
> >>           F                F              T
> >>
> >> Instead, the old feature was defined as "this is true if the guest
> >> _cannot_ do X".  For such a "negative" feature, migration is possible
> >> if the destination supports it, or the source sets it:
> >>
> >>     dest support      source set          ok?
> >>           T                T              T
> >>           T                F              F
> >>           F                T              T
> >>           F                F              T
> >>
> >> However, the negotiated features are supposed to be the AND of the
> >> device- and driver-supported features.  In the F/T case, the feature
> >> would be negotiated by the source as T, and become F when negotiated on
> >> the destination.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > Do you have any numbers showing how this new feature improves
> > performance?
> > We are able to batch quite a lot of pages in a single deflate
> > request - is the overhead measureable in practice?
> 
> It's not only about better times, but also about better algorithms.  I
> started writing this after seeing the Google fileballoon driver.  For
> that implementation, the deflateq cannot be used at all.
> 
> Paolo

It seems weak to claim the algorithm is better without trying it
out in practice.

I asked the fileballoon driver author whether
it can use TELL_HOST or if that will be too much overhead.
He said don't know.


And in particular, I think a feature bit is a bad fit for deciding how
page will be reclaimed.  Whether you will want a page back soon can
change over time depending on load etc.  So if we can't just tell host
first always, then I think we need to give guest control to do this per
inflate command.


-- 
MST

Re: [PATCH v2 0/2] virtio-balloon spec: silent deflation

[Michael S. Tsirkin]

On Tue, May 28, 2013 at 07:40:16PM +0200, Paolo Bonzini wrote:
> Here is the series, split in two patches, with small edits and new
> commit messages.
> 
> Paolo Bonzini (2):
>   virtio-balloon spec: rewrite description of
>     VIRTIO_BALLOON_F_MUST_TELL_HOST
>   virtio-balloon spec: reintroduce "silent deflation" feature
> 
>  virtio-spec.lyx | 238 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 233 insertions(+), 5 deletions(-)

Okay we talked off-line a bit, here's a summary:

- we both agree we want my patch making TELL_HOST optional,
  just to make current practice with windows drivers, legal.
  Will ask Rusty to re-consider it for inclusion.

- Paolo's patches need some experimentation, it's not 100%
  clear what the benefit is, and maybe it's possible to
  get same gains without spec/guest driver changes.


> -- 
> 1.8.2.1