Re: [PATCH] virtio_balloon: Notify guest only after deflating the balloon

On Sun, Jul 03, 2011 at 01:52:29PM +0300, Sasha Levin wrote:

On Sun, 2011-07-03 at 13:30 +0300, Michael S. Tsirkin wrote:

quoted

On Sun, Jul 03, 2011 at 12:46:59PM +0300, Sasha Levin wrote:

quoted

On Sun, 2011-07-03 at 11:27 +0300, Michael S. Tsirkin wrote:

quoted

Doesn't this basically just revert
bf50e69f63d21091e525185c3ae761412be0ba72?

Yes. I haven't noticed that commit.

Ignoring the VIRTIO_BALLOON_F_MUST_TELL_HOST feature flag causes an
unnecessary delay due to having to kick and wait for the host to process
the deflate vq before actually using the memory pages.

OTOH, as the commit points out:
	Without this feature, we might free a page (and have another
	user touch it) while the hypervisor is unprepared for it

Are you doing so many deflates that the speed is important?

In the case of kvm tools and qemu for example, the hypervisor doesn't
need to be 'prepared' for it.

When the balloon if inflated, the pages are madvise(MADV_DONTNEED),
which means that we can access those pages immediately when deflating
without having to do anything special to prepare for it.

I'm not saying that speed is critical here, I'm just saying that the
feature was added to the spec for a reason - to prevent unnecessary
overhead like in the case of kvm tools and qemu, and we just choose to
ignore it completely.

quoted

quoted

I've stumbled on it when writing the virtio_balloon driver for kvm
tools. Initially my plan was to ignore the deflate vq completely, since
the virtio spec says that unless we set the MUST_TELL_HOST flag,
"deflation advice is merely a courtesy".

I've noticed that if I don't process the deflate vq the guest doesn't
use the deflated pages at all - something which contradicts the feature
that lets him use the deflated pages without waiting for the deflate vq.

OK, so you have a host that does not process the vq.
As a result tell_host blocks forever.
With your patch, guest will block after releasing the
page, so the next deflate request will get stuck.
This doesn't seem to be a huge improvement so
we can conclude such a host is broken?

No, the host is not doing anything wrong by not processing deflate vq.

This just leads me to believe that we should either not notify the host,
or not wait_for_completion() when telling the host.

Interesting. The spec says

(a) The driver constructs an array of addresses of memory pages it has
previously given to the balloon, as described above. This descriptor
is added to the deflateq.
(b) 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.
(c) Otherwise, the guest may begin to re-use pages previously given to
the balloon before the device has acknowledged their withdrawl.
 21 In this case, deflation advice is merely a courtesy

This does not discuss the following issue: what happens
if the device never uses the descriptor in the deflateq
and VIRTIO_BALLOON_F_MUST_TELL_HOST is not set?

Rusty, any comments?

As far as I can tell, linux drivers assume that
devices acknowledge all descriptors, and did this
from the very beginning, so if you want the
device not to process that queue at all,
you will probably need a new flag for this rather than reuse
MUST_TELL_HOST.

quoted

quoted

quoted

On Sat, Jul 02, 2011 at 06:06:56AM +0300, Sasha Levin wrote:

quoted

Unless the host requires that requested pages won't be used until
he us notified (VIRTIO_BALLOON_F_MUST_TELL_HOST), only notify after
deflating the balloon.

This will avoid having to take an exit before actually using the pages.

Cc: Rusty Russell <redacted>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <redacted>
---
 drivers/virtio/virtio_balloon.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index e058ace..055f95d 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c

@@ -148,14 +148,18 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
 		vb->num_pages--;
 	}
 
-
 	/*
-	 * Note that if
-	 * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
-	 * is true, we *have* to do it in this order
+	 * If the host doesn't require us to notify him before using
+	 * pages which belong to the balloon, update him only after
+	 * freeing those pages for guest use.
 	 */
-	tell_host(vb, vb->deflate_vq);
-	release_pages_by_pfn(vb->pfns, vb->num_pfns);
+	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST)) {
+		tell_host(vb, vb->deflate_vq);
+		release_pages_by_pfn(vb->pfns, vb->num_pfns);
+	} else {
+		release_pages_by_pfn(vb->pfns, vb->num_pfns);
+		tell_host(vb, vb->deflate_vq);
+	}
 }
 
 static inline void update_stat(struct virtio_balloon *vb, int idx,

-- 
1.7.6

-- 

Sasha.

-- 

Sasha.