All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paolo Bonzini <pbonzini@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: rusty@rustcorp.com.au, fes@google.com, aarcange@redhat.com,
	riel@redhat.com, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, mikew@google.com,
	yinghan@google.com, virtualization@lists.linux-foundation.org
Subject: Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
Date: Thu, 06 Sep 2012 11:24:02 +0200	[thread overview]
Message-ID: <50486BB2.7070108@redhat.com> (raw)
In-Reply-To: <20120906084736.GF17656@redhat.com>

Il 06/09/2012 10:47, Michael S. Tsirkin ha scritto:
>> > - a migration from non-MUST_TELL_HOST to MUST_TELL_HOST will succeed,
>> > which is wrong;
>> > 
>> > - a migration from MUST_TELL_HOST to non-MUST_TELL_HOST will fail, which
>> > is useless.
>> > 
>> > Add instead a new feature VIRTIO_BALLOON_F_SILENT_DEFLATE, and deprecate
>> > VIRTIO_BALLOON_F_MUST_TELL_HOST since it is never actually used.
>> > 
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Frankly I think it's a qemu migration bug. I don't see why
> we need to tweak spec: just teach qemu to be smarter
> during migration.

Of course you can just teach QEMU to be smarter, but that would be a
one-off hack for the only ill-defined feature that says something is
_not_ supported.  Since in practice all virtio_balloon-enbled
hypervisors supported silent deflate (so the bit was always zero), and
no client used it (so it was never checked), it's easier to just reverse
the direction.

In fact, it's not clear how the driver should use the feature.  My guess
is that, if it wants to use silent deflate, it tries to negotiate
VIRTIO_BALLOON_F_MUST_TELL_HOST, and can use silent deflate if
negotiation fails.  This is against the logic of all other features.

> Can you show a scenario with old driver/new hypervisor or
> new driver/old hypervisor that fails?

Suppose the driver tried to negotiate the feature as above.  We then
have the two scenarios above.

In the harmless but annoying scenario, the source hypervisor doesn't
support silent deflate, so VIRTIO_BALLOON_F_MUST_TELL_HOST has been
negotiated successfully.  The destination hypervisor supports silent
deflate, so it does _not_ include the feature.  It sees that the guest
requests VIRTIO_BALLOON_F_MUST_TELL_HOST, and fails migration.

In the incorrect scenario, you are migrating to an older hypervisor.
The source hypervisor is newer and supports silent deflate, so
VIRTIO_BALLOON_F_MUST_TELL_HOST was _not_ negotiated.  The destination
hypervisor does not supports silent deflate.  However, the guest is not
requesting VIRTIO_BALLOON_F_MUST_TELL_HOST, and migration succeeds.
Next time the guest tries to do use a page from the balloon, badness
happens, because the hypervisor does not expect it.

Paolo

WARNING: multiple messages have this Message-ID (diff)
From: Paolo Bonzini <pbonzini@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: fes@google.com, aarcange@redhat.com, riel@redhat.com,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	mikew@google.com, yinghan@google.com,
	virtualization@lists.linux-foundation.org
Subject: Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
Date: Thu, 06 Sep 2012 11:24:02 +0200	[thread overview]
Message-ID: <50486BB2.7070108@redhat.com> (raw)
In-Reply-To: <20120906084736.GF17656@redhat.com>

Il 06/09/2012 10:47, Michael S. Tsirkin ha scritto:
>> > - a migration from non-MUST_TELL_HOST to MUST_TELL_HOST will succeed,
>> > which is wrong;
>> > 
>> > - a migration from MUST_TELL_HOST to non-MUST_TELL_HOST will fail, which
>> > is useless.
>> > 
>> > Add instead a new feature VIRTIO_BALLOON_F_SILENT_DEFLATE, and deprecate
>> > VIRTIO_BALLOON_F_MUST_TELL_HOST since it is never actually used.
>> > 
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Frankly I think it's a qemu migration bug. I don't see why
> we need to tweak spec: just teach qemu to be smarter
> during migration.

Of course you can just teach QEMU to be smarter, but that would be a
one-off hack for the only ill-defined feature that says something is
_not_ supported.  Since in practice all virtio_balloon-enbled
hypervisors supported silent deflate (so the bit was always zero), and
no client used it (so it was never checked), it's easier to just reverse
the direction.

In fact, it's not clear how the driver should use the feature.  My guess
is that, if it wants to use silent deflate, it tries to negotiate
VIRTIO_BALLOON_F_MUST_TELL_HOST, and can use silent deflate if
negotiation fails.  This is against the logic of all other features.

> Can you show a scenario with old driver/new hypervisor or
> new driver/old hypervisor that fails?

Suppose the driver tried to negotiate the feature as above.  We then
have the two scenarios above.

In the harmless but annoying scenario, the source hypervisor doesn't
support silent deflate, so VIRTIO_BALLOON_F_MUST_TELL_HOST has been
negotiated successfully.  The destination hypervisor supports silent
deflate, so it does _not_ include the feature.  It sees that the guest
requests VIRTIO_BALLOON_F_MUST_TELL_HOST, and fails migration.

In the incorrect scenario, you are migrating to an older hypervisor.
The source hypervisor is newer and supports silent deflate, so
VIRTIO_BALLOON_F_MUST_TELL_HOST was _not_ negotiated.  The destination
hypervisor does not supports silent deflate.  However, the guest is not
requesting VIRTIO_BALLOON_F_MUST_TELL_HOST, and migration succeeds.
Next time the guest tries to do use a page from the balloon, badness
happens, because the hypervisor does not expect it.

Paolo

  reply	other threads:[~2012-09-06  9:24 UTC|newest]

Thread overview: 71+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-06  7:46 [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works Paolo Bonzini
2012-09-06  7:46 ` Paolo Bonzini
2012-09-06  8:47 ` Michael S. Tsirkin
2012-09-06  8:47   ` Michael S. Tsirkin
2012-09-06  9:24   ` Paolo Bonzini [this message]
2012-09-06  9:24     ` Paolo Bonzini
2012-09-06  9:44     ` Michael S. Tsirkin
2012-09-06  9:44       ` Michael S. Tsirkin
2012-09-06  9:57       ` Paolo Bonzini
2012-09-06  9:57         ` Paolo Bonzini
2012-09-06 10:53         ` Michael S. Tsirkin
2012-09-06 10:53           ` Michael S. Tsirkin
2012-09-06 12:13           ` Paolo Bonzini
2012-09-06 12:13             ` Paolo Bonzini
2012-09-06 12:51             ` Michael S. Tsirkin
2012-09-06 12:51               ` Michael S. Tsirkin
2012-09-06 13:12               ` Paolo Bonzini
2012-09-06 13:12                 ` Paolo Bonzini
2012-09-06 23:45             ` Rusty Russell
2012-09-07  5:42               ` Michael S. Tsirkin
2012-09-07  5:42                 ` Michael S. Tsirkin
2012-09-07  6:39                 ` Rusty Russell
2012-09-07  6:39                   ` Rusty Russell
2012-09-07  9:27                   ` Paolo Bonzini
2012-09-07  9:27                     ` Paolo Bonzini
2012-09-07 10:53                     ` Michael S. Tsirkin
2012-09-07 10:53                       ` Michael S. Tsirkin
2012-09-07 11:20                       ` Paolo Bonzini
2012-09-07 11:20                         ` Paolo Bonzini
2012-09-07 12:17                         ` Michael S. Tsirkin
2012-09-07 12:17                           ` Michael S. Tsirkin
2012-09-07 12:22                           ` Paolo Bonzini
2012-09-07 12:22                             ` Paolo Bonzini
2012-09-07 12:44                             ` Michael S. Tsirkin
2012-09-07 12:44                               ` Michael S. Tsirkin
2012-09-07 14:04                               ` Paolo Bonzini
2012-09-07 14:04                                 ` Paolo Bonzini
2012-09-07 14:25                                 ` Michael S. Tsirkin
2012-09-07 14:25                                   ` Michael S. Tsirkin
2012-09-07 14:44                                   ` Paolo Bonzini
2012-09-07 14:44                                     ` Paolo Bonzini
2012-09-08 22:22                                     ` Michael S. Tsirkin
2012-09-08 22:22                                       ` Michael S. Tsirkin
2012-09-10  5:50                                       ` Paolo Bonzini
2012-09-10  5:50                                       ` Paolo Bonzini
2012-09-10  6:03                                         ` Michael S. Tsirkin
2012-09-10  6:03                                           ` Michael S. Tsirkin
2012-09-10  6:38                                           ` Paolo Bonzini
2012-09-10  6:38                                             ` Paolo Bonzini
2012-09-10  6:47                                             ` Michael S. Tsirkin
2012-09-10  6:47                                               ` Michael S. Tsirkin
2012-09-10  6:55                                               ` Paolo Bonzini
2012-09-10  6:55                                                 ` Paolo Bonzini
2012-09-07 10:43                   ` Michael S. Tsirkin
2012-09-07 10:43                     ` Michael S. Tsirkin
2012-09-08  5:06                     ` Rusty Russell
2012-09-08  5:06                       ` Rusty Russell
2012-09-08 10:32                       ` Paolo Bonzini
2012-09-08 10:32                       ` Paolo Bonzini
2012-09-08 22:37                       ` Michael S. Tsirkin
2012-09-08 22:37                         ` Michael S. Tsirkin
2012-09-10  1:43                         ` Rusty Russell
2012-09-10  1:43                           ` Rusty Russell
2012-09-10  5:51                           ` Paolo Bonzini
2012-09-10  5:51                             ` Paolo Bonzini
2012-09-10  5:51                           ` Michael S. Tsirkin
2012-09-10  5:51                             ` Michael S. Tsirkin
2012-09-12  6:24                             ` Rusty Russell
2012-09-12  6:24                               ` Rusty Russell
2012-09-06 23:41 ` Michael S. Tsirkin
2012-09-06 23:41   ` Michael S. Tsirkin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=50486BB2.7070108@redhat.com \
    --to=pbonzini@redhat.com \
    --cc=aarcange@redhat.com \
    --cc=fes@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mikew@google.com \
    --cc=mst@redhat.com \
    --cc=riel@redhat.com \
    --cc=rusty@rustcorp.com.au \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=yinghan@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.