virtio balloon: kill tell-host-first logic

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

---

 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

drivers/virtio/virtio_balloon.c renamed from drivers/virtio/virtio_balloon.c~kill-tell-host-first-logic +8 -13

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

_

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 <[email protected]>

---

 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

drivers/virtio/virtio_balloon.c renamed from drivers/virtio/virtio_balloon.c~kill-tell-host-first-logic +8 -13

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

_

Quoted 6 lines · Dave Hansen

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.

Quoted 2 lines

But, if the bit is _not_ set, we are under no obligation to reverse the order. Furthermore, all modern qemus set this bit.

Which qemus do this, specifically? Amit Shah just pointed out to me that upstream qemu.git and qemu-kvm.git don't seem to do this.

Which qemu did you test this with?

Quoted 6 lines · Dave Hansen

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.

Quoted 2 lines

But, if the bit is _not_ set, we are under no obligation to reverse the order. Furthermore, all modern qemus set this bit.

Which qemus do this, specifically? Amit Shah just pointed out to me that upstream qemu.git and qemu-kvm.git don't seem to do this.

Which qemu did you test this with?

Quoted 7 lines · Michael S. Tsirkin

Quoted 6 lines · Dave Hansen

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.

Quoted 5 lines

Quoted 2 lines

But, if the bit is _not_ set, we are under no obligation to reverse the order. Furthermore, all modern qemus set this bit.

Which qemus do this, specifically? Amit Shah just pointed out to me that upstream qemu.git and qemu-kvm.git don't seem to do this.

I had a conversation with Anthony about it, and I think I managed to confuse myself somewhere. Just to be clear, all that I see in the qemu-kvm git tree right now (df85c051d780bca0ee2462cfeb8ef6d9552a19b0) is this:

hw/virtio-balloon.h:#define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */

Quoted 1 line

Which qemu did you test this with?

Probably a week or two old qemu-kvm.

My changelog could probably use some work, but the patch still stands. The only requirement we have is that when VIRTIO_BALLOON_F_MUST_TELL_HOST is set we *MUST* tell the host, first. But, when it's not set, we can do whatever we want.

So, we might as well always have the ...F_MUST_TELL_HOST behavior all the time.

Quoted 7 lines · Michael S. Tsirkin

Quoted 6 lines · Dave Hansen

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.

Quoted 5 lines

Quoted 2 lines

But, if the bit is _not_ set, we are under no obligation to reverse the order. Furthermore, all modern qemus set this bit.

Which qemus do this, specifically? Amit Shah just pointed out to me that upstream qemu.git and qemu-kvm.git don't seem to do this.

I had a conversation with Anthony about it, and I think I managed to confuse myself somewhere. Just to be clear, all that I see in the qemu-kvm git tree right now (df85c051d780bca0ee2462cfeb8ef6d9552a19b0) is this:

hw/virtio-balloon.h:#define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */

Quoted 1 line

Which qemu did you test this with?

Probably a week or two old qemu-kvm.

My changelog could probably use some work, but the patch still stands. The only requirement we have is that when VIRTIO_BALLOON_F_MUST_TELL_HOST is set we *MUST* tell the host, first. But, when it's not set, we can do whatever we want.

So, we might as well always have the ...F_MUST_TELL_HOST behavior all the time.

Sure, the only contention was on the commit message, where you stated modern qemus set this... qemu doesn't, and it should. Care to do a patch for that?

Sure, the only contention was on the commit message, where you stated modern qemus set this... qemu doesn't, and it should. Care to do a patch for that?

Quoted 3 lines · Amit Shah

Sure, the only contention was on the commit message, where you stated modern qemus set this... qemu doesn't, and it should. Care to do a patch for that?

If Rusty hasn't pushed the commit out anywhere, we can still amend the commit. Otherwise, we're in a _bit_ of a pickle since you can't patch git logs. :)

Whatever is easiest for Rusty works for me.

How about this for a replacement log?

--

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 feature, 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; we're under no obligation to do _anything_. That's the state of affairs in current qemu:

	#define VIRTIO_BALLOON_F_MUST_TELL_HOST 0

Quoted 3 lines · Amit Shah

Sure, the only contention was on the commit message, where you stated modern qemus set this... qemu doesn't, and it should. Care to do a patch for that?

If Rusty hasn't pushed the commit out anywhere, we can still amend the commit. Otherwise, we're in a _bit_ of a pickle since you can't patch git logs. :)

Whatever is easiest for Rusty works for me.

How about this for a replacement log?

--

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 feature, 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; we're under no obligation to do _anything_. That's the state of affairs in current qemu:

	#define VIRTIO_BALLOON_F_MUST_TELL_HOST 0

Quoted 8 lines · Dave Hansen

Quoted 3 lines · Amit Shah

Sure, the only contention was on the commit message, where you stated modern qemus set this... qemu doesn't, and it should. Care to do a patch for that?

If Rusty hasn't pushed the commit out anywhere, we can still amend the commit. Otherwise, we're in a _bit_ of a pickle since you can't patch git logs. :)

I should've been clearer: care to do the qemu patch? :-)

Quoted 8 lines · Dave Hansen

Quoted 3 lines · Amit Shah

Sure, the only contention was on the commit message, where you stated modern qemus set this... qemu doesn't, and it should. Care to do a patch for that?

If Rusty hasn't pushed the commit out anywhere, we can still amend the commit. Otherwise, we're in a _bit_ of a pickle since you can't patch git logs. :)

I should've been clearer: care to do the qemu patch? :-)

Quoted 6 lines · Amit Shah

Quoted 5 lines · Dave Hansen

Quoted 3 lines · Amit Shah

Sure, the only contention was on the commit message, where you stated modern qemus set this... qemu doesn't, and it should. Care to do a patch for that?

Quoted 5 lines

Quoted 3 lines

If Rusty hasn't pushed the commit out anywhere, we can still amend the commit. Otherwise, we're in a _bit_ of a pickle since you can't patch git logs. :)

I should've been clearer: care to do the qemu patch? :-)

What do we want to do to qemu, though? Set the bit?

Quoted 6 lines · Amit Shah

Quoted 5 lines · Dave Hansen

Quoted 3 lines · Amit Shah

Sure, the only contention was on the commit message, where you stated modern qemus set this... qemu doesn't, and it should. Care to do a patch for that?

Quoted 5 lines

Quoted 3 lines

If Rusty hasn't pushed the commit out anywhere, we can still amend the commit. Otherwise, we're in a _bit_ of a pickle since you can't patch git logs. :)

I should've been clearer: care to do the qemu patch? :-)

What do we want to do to qemu, though? Set the bit?

Quoted 3 lines · Amit Shah

Sure, the only contention was on the commit message, where you stated modern qemus set this... qemu doesn't, and it should. Care to do a patch for that?

If Rusty hasn't pushed the commit out anywhere, we can still amend the commit. Otherwise, we're in a _bit_ of a pickle since you can't patch git logs. :)

Quoted 3 lines · Amit Shah

Sure, the only contention was on the commit message, where you stated modern qemus set this... qemu doesn't, and it should. Care to do a patch for that?

If Rusty hasn't pushed the commit out anywhere, we can still amend the commit. Otherwise, we're in a _bit_ of a pickle since you can't patch git logs. :)

Quoted 5 lines · Dave Hansen

Quoted 3 lines · Amit Shah

Sure, the only contention was on the commit message, where you stated modern qemus set this... qemu doesn't, and it should. Care to do a patch for that?

Quoted 3 lines

If Rusty hasn't pushed the commit out anywhere, we can still amend the commit. Otherwise, we're in a _bit_ of a pickle since you can't patch git logs. :)

I only use git to send patches to Linus, and even if I did, I certainly wouldn't try to publish an non-rebasing tree for exactly this reason.

So send me the new commit, or just the new message...

Quoted 5 lines · Dave Hansen

Quoted 3 lines · Amit Shah

Sure, the only contention was on the commit message, where you stated modern qemus set this... qemu doesn't, and it should. Care to do a patch for that?

Quoted 3 lines

If Rusty hasn't pushed the commit out anywhere, we can still amend the commit. Otherwise, we're in a _bit_ of a pickle since you can't patch git logs. :)

I only use git to send patches to Linus, and even if I did, I certainly wouldn't try to publish an non-rebasing tree for exactly this reason.

So send me the new commit, or just the new message...

Here's a new commit message: