Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756108Ab1DGRnc (ORCPT ); Thu, 7 Apr 2011 13:43:32 -0400 Received: from e2.ny.us.ibm.com ([32.97.182.142]:33546 "EHLO e2.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753765Ab1DGRnb (ORCPT ); Thu, 7 Apr 2011 13:43:31 -0400 Subject: [RFC][PATCH] virtio balloon: kill tell-host-first logic To: Rusty Russell Cc: "Michael S. Tsirkin" , virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, Dave Hansen From: Dave Hansen Date: Thu, 07 Apr 2011 10:43:25 -0700 Message-Id: <20110407174325.ED21C82B@kernel> X-Content-Scanned: Fidelis XPS MAILER Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2548 Lines: 74 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 --- 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: _ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/