From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51294) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TkZtT-0005GD-OQ for qemu-devel@nongnu.org; Mon, 17 Dec 2012 07:34:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TkZtO-0003nH-RZ for qemu-devel@nongnu.org; Mon, 17 Dec 2012 07:34:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39912) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TkZtO-0003nC-JJ for qemu-devel@nongnu.org; Mon, 17 Dec 2012 07:33:58 -0500 Date: Mon, 17 Dec 2012 10:33:57 -0200 From: Luiz Capitulino Message-ID: <20121217103357.1216a46f@doriath.home> In-Reply-To: <24E144B8C0207547AD09C467A8259F75578B4B5E@lisa.maurer-it.com> References: <1355500182-12743-1-git-send-email-lcapitulino@redhat.com> <1355500182-12743-2-git-send-email-lcapitulino@redhat.com> <24E144B8C0207547AD09C467A8259F75578B48B1@lisa.maurer-it.com> <20121217095729.057f7c1f@doriath.home> <24E144B8C0207547AD09C467A8259F75578B4B5E@lisa.maurer-it.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] balloon: drop old stats code & API List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dietmar Maurer Cc: "mdroth@linux.vnet.ibm.com" , "aliguori@us.ibm.com" , "qemu-devel@nongnu.org" , "agl@us.ibm.com" On Mon, 17 Dec 2012 12:23:59 +0000 Dietmar Maurer wrote: > > > I don't really see any conflicts here? > > > > query-balloon resets all stats to -1. So, if it's issued after the stats have been > > polled all values are lost. > > I can't see that in the code. I also tested with my patch, and it does not reset > any stats. So I can't see that conflict? > > There is no code to reset stats inside virtio_balloon_stat() static void virtio_balloon_stat(void *opaque, BalloonInfo *info) { VirtIOBalloon *dev = opaque; #if 0 /* Disable guest-provided stats for now. For more details please check: * https://bugzilla.redhat.com/show_bug.cgi?id=623903 * * If you do enable it (which is probably not going to happen as we * need a new command for it), remember that you also need to fill the * appropriate members of the BalloonInfo structure so that the stats * are returned to the client. */ if (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ)) { virtqueue_push(dev->svq, &dev->stats_vq_elem, dev->stats_vq_offset); virtio_notify(&dev->vdev, dev->svq); return; } #endif /* Stats are not supported. Clear out any stale values that might * have been set by a more featureful guest kernel. */ reset_stats(dev); <<<<======================================================== ^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^ info->actual = ram_size - ((uint64_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT); } > > > > > It's important to note that the QMP and HMP interfaces are also > > > > dropped by this commit. That shouldn't be a problem though, because: > > > > > > > > 1. All QMP fields are optional > > > > 2. This has never been really used > > > > > > Why don't we simply implement the missing parts - it's just a few lines of > > code. > > > > For the reasons explained in my last email. We'd need at least two > > commands, one for to enable polling/set time interval and also query- > > balloon-stats, as adding this to query-balloon would mix semantics (ie. one > > field is always there and the others have polling semantics). > > Yes, we need 2 commands - and what is the problem? The problem is adding new commands when that's not required. With QOM everything can be done with existing commands.