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@xxxxxxxxxxxxxxxxxx>
> ---
>
> 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:
> _
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/virtualization
