[RFC][PATCH] virtio balloon: kill tell-host-first logic

[RFC][PATCH] virtio balloon: kill tell-host-first logic

[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.

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

Re: [RFC][PATCH] virtio balloon: kill tell-host-first logic

[Rusty Russell]

On Thu, 07 Apr 2011 10:43:25 -0700, Dave Hansen <dave@linux.vnet.ibm.com> wrote:
> 
> 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>

Thanks, applied!

Rusty.

Re: [RFC][PATCH] virtio balloon: kill tell-host-first logic

[Michael S. Tsirkin]

On Thu, Apr 07, 2011 at 10:43:25AM -0700, Dave Hansen wrote:
> 
> 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.

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?

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

Re: [RFC][PATCH] virtio balloon: kill tell-host-first logic

[Dave Hansen]

On Mon, 2011-04-11 at 14:01 +0300, Michael S. Tsirkin wrote:
> On Thu, Apr 07, 2011 at 10:43:25AM -0700, Dave Hansen wrote:
> > 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.
> 
> 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 */

> 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.  

-- Dave

Re: [RFC][PATCH] virtio balloon: kill tell-host-first logic

[Amit Shah]

On (Mon) 11 Apr 2011 [15:11:11], Dave Hansen wrote:
> On Mon, 2011-04-11 at 14:01 +0300, Michael S. Tsirkin wrote:
> > On Thu, Apr 07, 2011 at 10:43:25AM -0700, Dave Hansen wrote:
> > > 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.
> > 
> > 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 */
> 
> > 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?

Thanks,

		Amit

Re: [RFC][PATCH] virtio balloon: kill tell-host-first logic

[Dave Hansen]

On Tue, 2011-04-12 at 11:13 +0530, Amit Shah wrote:
> 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

This patch makes the "tell host first" logic the only case.  This
should make everybody happy, and reduce the amount of untested or
untestable code in the kernel.

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>

-- Dave

Re: [RFC][PATCH] virtio balloon: kill tell-host-first logic

[Amit Shah]

On (Tue) 12 Apr 2011 [09:22:32], Dave Hansen wrote:
> On Tue, 2011-04-12 at 11:13 +0530, Amit Shah wrote:
> > 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? :-)

> 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

Actually this is just the bit number; it doesn't get set.

		Amit

Re: [RFC][PATCH] virtio balloon: kill tell-host-first logic

[Rusty Russell]

On Tue, 12 Apr 2011 09:22:32 -0700, Dave Hansen <dave@linux.vnet.ibm.com> wrote:
> On Tue, 2011-04-12 at 11:13 +0530, Amit Shah wrote:
> > 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 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...

Thanks,
Rusty.

Re: [RFC][PATCH] virtio balloon: kill tell-host-first logic

[Dave Hansen]

On Tue, 2011-04-12 at 22:13 +0530, Amit Shah wrote:
> On (Tue) 12 Apr 2011 [09:22:32], Dave Hansen wrote:
> > On Tue, 2011-04-12 at 11:13 +0530, Amit Shah wrote:
> > > 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? :-)

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

-- Dave

Re: [RFC][PATCH] virtio balloon: kill tell-host-first logic

[Dave Hansen]

On Thu, 2011-04-14 at 21:07 +0930, Rusty Russell wrote:
> On Tue, 12 Apr 2011 09:22:32 -0700, Dave Hansen <dave@linux.vnet.ibm.com> wrote:
> > On Tue, 2011-04-12 at 11:13 +0530, Amit Shah wrote:
> > > 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 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:

--

The virtio balloon driver has a VIRTIO_BALLOON_F_MUST_TELL_HOST
feature bit.  Whenever the bit is set, the guest kernel 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_.
As of now, qemu-kvm defines the bit, but doesn't set it.

This patch makes the "tell host first" logic the only case.  This
should make everybody happy, and reduce the amount of untested or
untestable code in the kernel.

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>

Re: [RFC][PATCH] virtio balloon: kill tell-host-first logic

[Amit Shah]

On (Thu) 14 Apr 2011 [13:28:17], Dave Hansen wrote:
> On Tue, 2011-04-12 at 22:13 +0530, Amit Shah wrote:
> > On (Tue) 12 Apr 2011 [09:22:32], Dave Hansen wrote:
> > > On Tue, 2011-04-12 at 11:13 +0530, Amit Shah wrote:
> > > > 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? :-)
> 
> What do we want to do to qemu, though?  Set the bit?

Yes, that should ensure older guests behave fine too.

		Amit

Re: [RFC][PATCH] virtio balloon: kill tell-host-first logic

[Rusty Russell]

On Thu, 14 Apr 2011 13:30:05 -0700, Dave Hansen <dave@linux.vnet.ibm.com> wrote:
> Here's a new commit message:

Applied, thanks.

Rusty.