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@xxxxxxxxxx>
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
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
