All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Hansen <dave@linux.vnet.ibm.com>
To: Rusty Russell <rusty@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
	virtualization@lists.linux-foundation.org,
	linux-kernel@vger.kernel.org,
	Dave Hansen <dave@linux.vnet.ibm.com>
Subject: [RFC][PATCH] virtio balloon: kill tell-host-first logic
Date: Thu, 07 Apr 2011 10:43:25 -0700	[thread overview]
Message-ID: <20110407174325.ED21C82B@kernel> (raw)


The virtio balloon driver has a VIRTIO_BALLOON_F_MUST_TELL_HOST
feature bit.  Whenever the bit is set, we must always tell the
host before we free pages back to the allocator.  Without this
we might free a page (and have another user touch it) while the
hypervisor is unprepared for it.

But, if the bit is _not_ set, we are under no obligation to
reverse the order.  Furthermore, all modern qemus set this bit.
So, the "tell second" code is completely unused and untestable.
Quoting Anthony: "untested code is broken code".

This _also_ means that we don't have to preserve a pfn list
after the pages are freed, which should let us get rid of some
temporary storage (vb->pfns) eventually.


Signed-off-by: Dave Hansen <dave@linux.vnet.ibm.com>
---

 linux-2.6.git-dave/drivers/virtio/virtio_balloon.c |   21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff -puN drivers/virtio/virtio_balloon.c~kill-tell-host-first-logic drivers/virtio/virtio_balloon.c
--- linux-2.6.git/drivers/virtio/virtio_balloon.c~kill-tell-host-first-logic	2011-04-07 10:23:12.016343374 -0700
+++ linux-2.6.git-dave/drivers/virtio/virtio_balloon.c	2011-04-07 10:23:12.024343370 -0700
@@ -40,9 +40,6 @@ struct virtio_balloon
 	/* Waiting for host to ack the pages we released. */
 	struct completion acked;
 
-	/* Do we have to tell Host *before* we reuse pages? */
-	bool tell_host_first;
-
 	/* The pages we've told the Host we're not using. */
 	unsigned int num_pages;
 	struct list_head pages;
@@ -151,13 +148,14 @@ static void leak_balloon(struct virtio_b
 		vb->num_pages--;
 	}
 
-	if (vb->tell_host_first) {
-		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);
-	}
+
+	/*
+	 * 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);
+	release_pages_by_pfn(vb->pfns, vb->num_pfns);
 }
 
 static inline void update_stat(struct virtio_balloon *vb, int idx,
@@ -325,9 +323,6 @@ static int virtballoon_probe(struct virt
 		goto out_del_vqs;
 	}
 
-	vb->tell_host_first
-		= virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
-
 	return 0;
 
 out_del_vqs:
_

             reply	other threads:[~2011-04-07 17:43 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-04-07 17:43 Dave Hansen [this message]
2011-04-09 22:48 ` [RFC][PATCH] virtio balloon: kill tell-host-first logic Rusty Russell
2011-04-09 22:48 ` Rusty Russell
2011-04-11 11:01 ` Michael S. Tsirkin
2011-04-11 11:01 ` Michael S. Tsirkin
2011-04-11 22:11   ` Dave Hansen
2011-04-11 22:11   ` Dave Hansen
2011-04-12  5:43     ` Amit Shah
2011-04-12 16:22       ` Dave Hansen
2011-04-12 16:22       ` Dave Hansen
2011-04-12 16:43         ` Amit Shah
2011-04-12 16:43         ` Amit Shah
2011-04-14 20:28           ` Dave Hansen
2011-04-15 10:39             ` Amit Shah
2011-04-15 10:39             ` Amit Shah
2011-04-14 20:28           ` Dave Hansen
2011-04-14 11:37         ` Rusty Russell
2011-04-14 11:37         ` Rusty Russell
2011-04-14 20:30           ` Dave Hansen
2011-04-19  1:25             ` Rusty Russell
2011-04-19  1:25               ` Rusty Russell
2011-04-14 20:30           ` Dave Hansen
2011-04-12  5:43     ` Amit Shah
  -- strict thread matches above, loose matches on Subject: below --
2011-04-07 17:43 Dave Hansen

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=20110407174325.ED21C82B@kernel \
    --to=dave@linux.vnet.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mst@redhat.com \
    --cc=rusty@rustcorp.com.au \
    --cc=virtualization@lists.linux-foundation.org \
    /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.