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

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

[Paolo Bonzini]

The idea of the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is 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 problem is that this feature is a "negative" feature: if
set, the guest _may not_ use ballooned pages directly.  Negative features
are not safe against migration; here is an explanation why this is so.

For 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

For a "negative" feature, migration is possible if the destination
supports it, or the source set it:

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

However, the F/T line violates the virtio specification because the
negotiated features are supposed to be the AND of the device-
and driver-supported features.

Furthermore, this assumes that the destination host knows which features
are "positive" and which are "negative", which obviously cannot be the
case in general.  (The original spec assumed that every device supports
VIRTIO_BALLOON_F_MUST_TELL_HOST, but this was not explicitly documented
and in practice it turns out not to be the case).

Not all is lost, however.  First, all known device implementations support
silent deflation, hence they do not negotiate the feature.  We are thus
somewhat free to redefine what the host should do about this feature.

Second, by chance, coincidence or an evil plot, the only known driver
that does not negotiate VIRTIO_BALLOON_F_MUST_TELL_HOST is also using
pages before telling the host.  Thus, even though the feature used to be
just for communication from the host, known drivers are really using it
to communicate was in the other direction, as if the feature was named
"VIRTIO_BALLOON_F_GUEST_TELLS_HOST".

Adjust the spec to conform, and add a new feature bit for the host to
tell the drivers if silent deflation is actually supported.  With this
new feature bit, the host can distinguish all three cases: will never
do silent deflation, will do silent deflation if available, will always
do silent deflation (as in the above buggy driver).

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

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index 73e22e7..033362f 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -63,7 +63,7 @@
 \author -385801441 "Cornelia Huck" cornelia.huck@de.ibm.com
 \author 460276516 "Dmitry Fleytman" dfleytma@redhat.com
 \author 1112500848 "Rusty Russell" rusty@rustcorp.com.au
-\author 1531152142 "Paolo Bonzini,,," 
+\author 1531152142 "Paolo Bonzini" pbonzini@redhat.com
 \author 1717892615 "Alexey Zaytsev,,," 
 \author 1986246365 "Michael S. Tsirkin" 
 \end_header
@@ -7179,11 +7179,49 @@ bits
 
 \begin_deeper
 \begin_layout Description
-VIRTIO_BALLOON_F_MUST_TELL_HOST
+VIRTIO_BALLOON_F_
+\change_deleted 1531152142 1347020601
+MUST
+\change_inserted 1531152142 1347020602
+GUEST
+\change_unchanged
+_TELL
+\change_inserted 1531152142 1368004486
+S
+\change_unchanged
+_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 will tell host
+\change_unchanged
+ before pages from the balloon are used.
+
+\change_inserted 1531152142 1368005603
+ The host should always propose this feature.
+\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
@@ -7192,6 +7230,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 does not need to tell host before pages from the balloon are used.
+\change_unchanged
+
 \end_layout
 
 \end_deeper
@@ -7342,9 +7394,27 @@ 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 
+\change_inserted 1531152142 1347025663
+the VIRTIO_BALLOON_F_\SpecialChar \-
+GUEST_\SpecialChar \-
+TELLS_\SpecialChar \-
+HOST feature is set, and 
+\change_unchanged
+the VIRTIO_BALLOON_F_
+\change_deleted 1531152142 1347021706
+MUST_TELL_HOST
+\change_inserted 1531152142 1347022257
+\SpecialChar \-
+SILENT_\SpecialChar \-
+DEFLATE
+\change_unchanged
+ feature is 
+\change_inserted 1531152142 1347025674
+not 
+\change_unchanged
+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
@@ -7535,6 +7605,188 @@ 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
+the host will always negotiate the VIRTIO_\SpecialChar \-
+BALLOON_F_\SpecialChar \-
+GUEST_\SpecialChar \-
+TELLS_\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.
+ If the host does not support silent deflate, it must not let the guest
+ discover any virtqueue, so that the driver will fail to start.
+\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 \-
+GUEST_\SpecialChar \-
+TELLS_\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 \-
+GUEST_\SpecialChar \-
+TELLS_\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
+Old hosts may fail to propose the VIRTIO_\SpecialChar \-
+BALLOON_F_\SpecialChar \-
+GUEST_\SpecialChar \-
+TELLS_\SpecialChar \-
+HOST feature.
+ This is not a problem as long as these old hosts support silent deflation;
+ when running on such a host:
+\end_layout
+
+\begin_layout Itemize
+
+\change_inserted 1531152142 1368005515
+a guest that 
+\emph on
+must
+\emph default
+ use, or 
+\emph on
+will never
+\emph default
+ use silent deflation will just work.
+\end_layout
+
+\begin_layout Itemize
+
+\change_inserted 1531152142 1368005691
+a guest that 
+\emph on
+can optionally
+\emph default
+ use silent deflation will typically not use silent deflation, because these
+ old hosts do not know about VIRTIO_\SpecialChar \-
+BALLOON_F_\SpecialChar \-
+SILENT_\SpecialChar \-
+DEFLATE, but the guest
+ will otherwise work normally.
+\end_layout
+
 \begin_layout Chapter*
 Appendix H: Rpmsg: Remote Processor Messaging
 \end_layout
-- 
1.8.2

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Paolo Bonzini]

Il 08/05/2013 12:10, Paolo Bonzini ha scritto:
> The idea of the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is 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 problem is that this feature is a "negative" feature: if
> set, the guest _may not_ use ballooned pages directly.  Negative features
> are not safe against migration; here is an explanation why this is so.
> 
> For 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
> 
> For a "negative" feature, migration is possible if the destination
> supports it, or the source set it:
> 
>     dest support      source set          ok?
>           T                T              T
>           T                F              F
>           F                T              T
>           F                F              T
> 
> However, the F/T line violates the virtio specification because the
> negotiated features are supposed to be the AND of the device-
> and driver-supported features.
> 
> Furthermore, this assumes that the destination host knows which features
> are "positive" and which are "negative", which obviously cannot be the
> case in general.  (The original spec assumed that every device supports
> VIRTIO_BALLOON_F_MUST_TELL_HOST, but this was not explicitly documented
> and in practice it turns out not to be the case).
> 
> Not all is lost, however.  First, all known device implementations support
> silent deflation, hence they do not negotiate the feature.  We are thus
> somewhat free to redefine what the host should do about this feature.
> 
> Second, by chance, coincidence or an evil plot, the only known driver
> that does not negotiate VIRTIO_BALLOON_F_MUST_TELL_HOST is also using
> pages before telling the host.  Thus, even though the feature used to be
> just for communication from the host, known drivers are really using it
> to communicate was in the other direction, as if the feature was named
> "VIRTIO_BALLOON_F_GUEST_TELLS_HOST".
> 
> Adjust the spec to conform, and add a new feature bit for the host to
> tell the drivers if silent deflation is actually supported.  With this
> new feature bit, the host can distinguish all three cases: will never
> do silent deflation, will do silent deflation if available, will always
> do silent deflation (as in the above buggy driver).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Ping?

Paolo

> ---
>  virtio-spec.lyx | 264 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 258 insertions(+), 6 deletions(-)
> 
> diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> index 73e22e7..033362f 100644
> --- a/virtio-spec.lyx
> +++ b/virtio-spec.lyx
> @@ -63,7 +63,7 @@
>  \author -385801441 "Cornelia Huck" cornelia.huck@de.ibm.com
>  \author 460276516 "Dmitry Fleytman" dfleytma@redhat.com
>  \author 1112500848 "Rusty Russell" rusty@rustcorp.com.au
> -\author 1531152142 "Paolo Bonzini,,," 
> +\author 1531152142 "Paolo Bonzini" pbonzini@redhat.com
>  \author 1717892615 "Alexey Zaytsev,,," 
>  \author 1986246365 "Michael S. Tsirkin" 
>  \end_header
> @@ -7179,11 +7179,49 @@ bits
>  
>  \begin_deeper
>  \begin_layout Description
> -VIRTIO_BALLOON_F_MUST_TELL_HOST
> +VIRTIO_BALLOON_F_
> +\change_deleted 1531152142 1347020601
> +MUST
> +\change_inserted 1531152142 1347020602
> +GUEST
> +\change_unchanged
> +_TELL
> +\change_inserted 1531152142 1368004486
> +S
> +\change_unchanged
> +_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 will tell host
> +\change_unchanged
> + before pages from the balloon are used.
> +
> +\change_inserted 1531152142 1368005603
> + The host should always propose this feature.
> +\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
> @@ -7192,6 +7230,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 does not need to tell host before pages from the balloon are used.
> +\change_unchanged
> +
>  \end_layout
>  
>  \end_deeper
> @@ -7342,9 +7394,27 @@ 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 
> +\change_inserted 1531152142 1347025663
> +the VIRTIO_BALLOON_F_\SpecialChar \-
> +GUEST_\SpecialChar \-
> +TELLS_\SpecialChar \-
> +HOST feature is set, and 
> +\change_unchanged
> +the VIRTIO_BALLOON_F_
> +\change_deleted 1531152142 1347021706
> +MUST_TELL_HOST
> +\change_inserted 1531152142 1347022257
> +\SpecialChar \-
> +SILENT_\SpecialChar \-
> +DEFLATE
> +\change_unchanged
> + feature is 
> +\change_inserted 1531152142 1347025674
> +not 
> +\change_unchanged
> +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
> @@ -7535,6 +7605,188 @@ 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
> +the host will always negotiate the VIRTIO_\SpecialChar \-
> +BALLOON_F_\SpecialChar \-
> +GUEST_\SpecialChar \-
> +TELLS_\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.
> + If the host does not support silent deflate, it must not let the guest
> + discover any virtqueue, so that the driver will fail to start.
> +\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 \-
> +GUEST_\SpecialChar \-
> +TELLS_\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 \-
> +GUEST_\SpecialChar \-
> +TELLS_\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
> +Old hosts may fail to propose the VIRTIO_\SpecialChar \-
> +BALLOON_F_\SpecialChar \-
> +GUEST_\SpecialChar \-
> +TELLS_\SpecialChar \-
> +HOST feature.
> + This is not a problem as long as these old hosts support silent deflation;
> + when running on such a host:
> +\end_layout
> +
> +\begin_layout Itemize
> +
> +\change_inserted 1531152142 1368005515
> +a guest that 
> +\emph on
> +must
> +\emph default
> + use, or 
> +\emph on
> +will never
> +\emph default
> + use silent deflation will just work.
> +\end_layout
> +
> +\begin_layout Itemize
> +
> +\change_inserted 1531152142 1368005691
> +a guest that 
> +\emph on
> +can optionally
> +\emph default
> + use silent deflation will typically not use silent deflation, because these
> + old hosts do not know about VIRTIO_\SpecialChar \-
> +BALLOON_F_\SpecialChar \-
> +SILENT_\SpecialChar \-
> +DEFLATE, but the guest
> + will otherwise work normally.
> +\end_layout
> +
>  \begin_layout Chapter*
>  Appendix H: Rpmsg: Remote Processor Messaging
>  \end_layout
> 

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Michael S. Tsirkin]

On Mon, May 27, 2013 at 05:55:05PM +0200, Paolo Bonzini wrote:
> Il 08/05/2013 12:10, Paolo Bonzini ha scritto:
> > The idea of the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is 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 problem is that this feature is a "negative" feature: if
> > set, the guest _may not_ use ballooned pages directly.  Negative features
> > are not safe against migration; here is an explanation why this is so.
> > 
> > For 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
> > 
> > For a "negative" feature, migration is possible if the destination
> > supports it, or the source set it:
> > 
> >     dest support      source set          ok?
> >           T                T              T
> >           T                F              F
> >           F                T              T
> >           F                F              T
> > 
> > However, the F/T line violates the virtio specification because the
> > negotiated features are supposed to be the AND of the device-
> > and driver-supported features.
> > 
> > Furthermore, this assumes that the destination host knows which features
> > are "positive" and which are "negative", which obviously cannot be the
> > case in general.  (The original spec assumed that every device supports
> > VIRTIO_BALLOON_F_MUST_TELL_HOST, but this was not explicitly documented
> > and in practice it turns out not to be the case).
> > 
> > Not all is lost, however.  First, all known device implementations support
> > silent deflation, hence they do not negotiate the feature.  We are thus
> > somewhat free to redefine what the host should do about this feature.
> > 
> > Second, by chance, coincidence or an evil plot, the only known driver
> > that does not negotiate VIRTIO_BALLOON_F_MUST_TELL_HOST is also using
> > pages before telling the host.  Thus, even though the feature used to be
> > just for communication from the host, known drivers are really using it
> > to communicate was in the other direction, as if the feature was named
> > "VIRTIO_BALLOON_F_GUEST_TELLS_HOST".
> > 
> > Adjust the spec to conform, and add a new feature bit for the host to
> > tell the drivers if silent deflation is actually supported.  With this
> > new feature bit, the host can distinguish all three cases: will never
> > do silent deflation, will do silent deflation if available, will always
> > do silent deflation (as in the above buggy driver).
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Ping?
> 
> Paolo

I don't think we need a new feature. Hosts do not in practice
treat the feature as "negative" (that is, required), whatever the spec
says.  Further, windows guests don't treat it is such either.  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.

Please see
	[PATCH] virtio-spec: balloon: MUST_TELL_HOST is optional
that does exactly this.


> > ---
> >  virtio-spec.lyx | 264 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 258 insertions(+), 6 deletions(-)
> > 
> > diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> > index 73e22e7..033362f 100644
> > --- a/virtio-spec.lyx
> > +++ b/virtio-spec.lyx
> > @@ -63,7 +63,7 @@
> >  \author -385801441 "Cornelia Huck" cornelia.huck@de.ibm.com
> >  \author 460276516 "Dmitry Fleytman" dfleytma@redhat.com
> >  \author 1112500848 "Rusty Russell" rusty@rustcorp.com.au
> > -\author 1531152142 "Paolo Bonzini,,," 
> > +\author 1531152142 "Paolo Bonzini" pbonzini@redhat.com
> >  \author 1717892615 "Alexey Zaytsev,,," 
> >  \author 1986246365 "Michael S. Tsirkin" 
> >  \end_header
> > @@ -7179,11 +7179,49 @@ bits
> >  
> >  \begin_deeper
> >  \begin_layout Description
> > -VIRTIO_BALLOON_F_MUST_TELL_HOST
> > +VIRTIO_BALLOON_F_
> > +\change_deleted 1531152142 1347020601
> > +MUST
> > +\change_inserted 1531152142 1347020602
> > +GUEST
> > +\change_unchanged
> > +_TELL
> > +\change_inserted 1531152142 1368004486
> > +S
> > +\change_unchanged
> > +_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 will tell host
> > +\change_unchanged
> > + before pages from the balloon are used.
> > +
> > +\change_inserted 1531152142 1368005603
> > + The host should always propose this feature.
> > +\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
> > @@ -7192,6 +7230,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 does not need to tell host before pages from the balloon are used.
> > +\change_unchanged
> > +
> >  \end_layout
> >  
> >  \end_deeper
> > @@ -7342,9 +7394,27 @@ 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 
> > +\change_inserted 1531152142 1347025663
> > +the VIRTIO_BALLOON_F_\SpecialChar \-
> > +GUEST_\SpecialChar \-
> > +TELLS_\SpecialChar \-
> > +HOST feature is set, and 
> > +\change_unchanged
> > +the VIRTIO_BALLOON_F_
> > +\change_deleted 1531152142 1347021706
> > +MUST_TELL_HOST
> > +\change_inserted 1531152142 1347022257
> > +\SpecialChar \-
> > +SILENT_\SpecialChar \-
> > +DEFLATE
> > +\change_unchanged
> > + feature is 
> > +\change_inserted 1531152142 1347025674
> > +not 
> > +\change_unchanged
> > +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
> > @@ -7535,6 +7605,188 @@ 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
> > +the host will always negotiate the VIRTIO_\SpecialChar \-
> > +BALLOON_F_\SpecialChar \-
> > +GUEST_\SpecialChar \-
> > +TELLS_\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.
> > + If the host does not support silent deflate, it must not let the guest
> > + discover any virtqueue, so that the driver will fail to start.
> > +\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 \-
> > +GUEST_\SpecialChar \-
> > +TELLS_\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 \-
> > +GUEST_\SpecialChar \-
> > +TELLS_\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
> > +Old hosts may fail to propose the VIRTIO_\SpecialChar \-
> > +BALLOON_F_\SpecialChar \-
> > +GUEST_\SpecialChar \-
> > +TELLS_\SpecialChar \-
> > +HOST feature.
> > + This is not a problem as long as these old hosts support silent deflation;
> > + when running on such a host:
> > +\end_layout
> > +
> > +\begin_layout Itemize
> > +
> > +\change_inserted 1531152142 1368005515
> > +a guest that 
> > +\emph on
> > +must
> > +\emph default
> > + use, or 
> > +\emph on
> > +will never
> > +\emph default
> > + use silent deflation will just work.
> > +\end_layout
> > +
> > +\begin_layout Itemize
> > +
> > +\change_inserted 1531152142 1368005691
> > +a guest that 
> > +\emph on
> > +can optionally
> > +\emph default
> > + use silent deflation will typically not use silent deflation, because these
> > + old hosts do not know about VIRTIO_\SpecialChar \-
> > +BALLOON_F_\SpecialChar \-
> > +SILENT_\SpecialChar \-
> > +DEFLATE, but the guest
> > + will otherwise work normally.
> > +\end_layout
> > +
> >  \begin_layout Chapter*
> >  Appendix H: Rpmsg: Remote Processor Messaging
> >  \end_layout
> > 

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Paolo Bonzini]

Il 27/05/2013 18:04, Michael S. Tsirkin ha scritto:
> I don't think we need a new feature. Hosts do not in practice
> treat the feature as "negative" (that is, required), whatever the spec
> says.  Further, windows guests don't treat it is such either.

Windows guests not treating as such is what makes the spec change work.

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

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

Paolo

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Michael S. Tsirkin]

On Mon, May 27, 2013 at 06:09:54PM +0200, Paolo Bonzini wrote:
> Il 27/05/2013 18:04, Michael S. Tsirkin ha scritto:
> > I don't think we need a new feature. Hosts do not in practice
> > treat the feature as "negative" (that is, required), whatever the spec
> > says.  Further, windows guests don't treat it is such either.
> 
> Windows guests not treating as such is what makes the spec change work.
> 
> > 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 guest cooperates we don't have to require anything,
just go with what guest tells us it will do.

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

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?


-- 
MST

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Paolo Bonzini]

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, then we can
crash it.  So we need to know what the guest is going to do, in order to
enable/disable the optimization.

> If guest cooperates we don't have to require anything,
> just go with what guest tells us it will do.

Yes.

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

Here it is the other way round.  The existing guest is not checking the
outcome of the negotiation, so the host must check whether negotiation
happened and possibly fail the initialization of the device.  It is
sufficiently different from any other case that I don't think a one-word
change is enough.

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

Paolo

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Michael S. Tsirkin]

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?

> then we can
> crash it.  So we need to know what the guest is going to do, in order to
> enable/disable the optimization.
> 
> > If guest cooperates we don't have to require anything,
> > just go with what guest tells us it will do.
> 
> Yes.
> 
> >>> 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.
> >>
> >> Paolo
> > 
> > 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.
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.

> 
> Here it is the other way round.  The existing guest is not checking the
> outcome of the negotiation, so the host must check whether negotiation
> happened and possibly fail the initialization of the device.  It is
> sufficiently different from any other case that I don't think a one-word
> change is enough.
> 
> 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.

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

I'm fine with adding more clarifications but I don't yet see why
do we need a new bit.

-- 
MST

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Paolo Bonzini]

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

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Michael S. Tsirkin]

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

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Paolo Bonzini]

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

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Michael S. Tsirkin]

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

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Paolo Bonzini]

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

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Michael S. Tsirkin]

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

Maybe I'm just dense.
Let's see the split spec patchset?

-- 
MST

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Paolo Bonzini]

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

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Michael S. Tsirkin]

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.
Again, could be just me seeing things.
If it's in spec, I think it would be clearer what are we trying to
achieve, and how.

-- 
MST

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Paolo Bonzini]

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?  Here are all the
possibilities:

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

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Michael S. Tsirkin]

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

Re: [PATCH] virtio-balloon spec: rework VIRTIO_BALLOON_F_MUST_TELL_HOST feature, support silent deflation

[Paolo Bonzini]

Il 28/05/2013 18:50, Michael S. Tsirkin ha scritto:
>>> 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.

Guest bug.  Guest should only do so if it sees SILENT_DEFLATE too.

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

Well, there are three cases, you need two bits.

The amount of text is because I'm writing the same thing 20 times in
different ways.  I'm still more satisfied with the text in the patch
than with anything I've written here.  It's possible I contradicted
myself, in fact.

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

Okay.  If you agree that your patch does entail a change for old hosts,
that would be a step forward indeed.

Paolo