From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54557) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tgesp-0004HT-DE for qemu-devel@nongnu.org; Thu, 06 Dec 2012 12:05:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tgesn-00057s-Dt for qemu-devel@nongnu.org; Thu, 06 Dec 2012 12:05:11 -0500 Received: from mail-oa0-f45.google.com ([209.85.219.45]:52315) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tgesm-000574-Rw for qemu-devel@nongnu.org; Thu, 06 Dec 2012 12:05:09 -0500 Received: by mail-oa0-f45.google.com with SMTP id i18so6263224oag.4 for ; Thu, 06 Dec 2012 09:05:08 -0800 (PST) Sender: fluxion Date: Thu, 6 Dec 2012 11:03:21 -0600 From: mdroth Message-ID: <20121206170321.GA30686@vm> References: <1354633488-24112-1-git-send-email-lcapitulino@redhat.com> <1354633488-24112-3-git-send-email-lcapitulino@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1354633488-24112-3-git-send-email-lcapitulino@redhat.com> Subject: Re: [Qemu-devel] [RFC 2/3] virtio-balloon: re-enable balloon stats List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, agl@us.ibm.com On Tue, Dec 04, 2012 at 01:04:47PM -0200, Luiz Capitulino wrote: > The statistics are now available through device properties via a > polling mechanism. First, a client has to enable polling, then it > can query the stats. > > The following control properties are introduced: > > o stats-polling-interval: a value greater than zero enables polling > in the specified interval (in seconds). When value equals zero, > polling is disabled. If polling is already enabled and a value > greater than zero is written, the polling interval time is changed > > o stats-last-update: last stats update timestamp, in seconds. > > The following stats properties are introduced: > > o stat-swap-in > o stat-swap-out > o stat-major-faults > o stat-minor-faults > o stat-free-memory > o stat-total-memory > > All values are in bytes. A value of -1 means that the statistic isn't > available right now. > > FIXME: Can balloon_stats_poll_cb(), balloon_stats_set_poll_interval(), > virtio_balloon_handle_output() can run in parallel? > > XXX: Should we return an error instead of -1? Might require a specific > error. Although this is not exactly a failure... > > Signed-off-by: Luiz Capitulino > --- > > NOTE: Anthony suggested having a bool to enable/disable polling, but I prefer > to let the client specify the polling interval. I can revisit this, > though. > > hw/virtio-balloon.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 155 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c > index 4398025..06af18f 100644 > --- a/hw/virtio-balloon.c > +++ b/hw/virtio-balloon.c > @@ -22,6 +22,8 @@ > #include "virtio-balloon.h" > #include "kvm.h" > #include "exec-memory.h" > +#include "qemu-timer.h" > +#include "qapi/qapi-visit-core.h" > > #if defined(__linux__) > #include > @@ -36,6 +38,9 @@ typedef struct VirtIOBalloon > uint64_t stats[VIRTIO_BALLOON_S_NR]; > VirtQueueElement stats_vq_elem; > size_t stats_vq_offset; > + QEMUTimer *stats_timer; > + int64_t stats_last_update; > + int64_t stats_poll_interval; > DeviceState *qdev; > } VirtIOBalloon; > > @@ -53,6 +58,16 @@ static void balloon_page(void *addr, int deflate) > #endif > } > > +static const char *balloon_stat_names[] = { > + [VIRTIO_BALLOON_S_SWAP_IN] = "stat-swap-in", > + [VIRTIO_BALLOON_S_SWAP_OUT] = "stat-swap-out", > + [VIRTIO_BALLOON_S_MAJFLT] = "stat-major-faults", > + [VIRTIO_BALLOON_S_MINFLT] = "stat-minor-faults", > + [VIRTIO_BALLOON_S_MEMFREE] = "stat-free-memory", > + [VIRTIO_BALLOON_S_MEMTOT] = "stat-total-memory", > + [VIRTIO_BALLOON_S_NR] = NULL > +}; > + > /* > * reset_stats - Mark all items in the stats array as unset > * > @@ -67,6 +82,119 @@ static inline void reset_stats(VirtIOBalloon *dev) > for (i = 0; i < VIRTIO_BALLOON_S_NR; dev->stats[i++] = -1); > } > > +static bool balloon_stats_supported(const VirtIOBalloon *s) > +{ > + return s->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ); > +} > + > +static bool balloon_stats_enabled(const VirtIOBalloon *s) > +{ > + return s->stats_poll_interval > 0; > +} > + > +static void balloon_stats_disable_timer(VirtIOBalloon *s) > +{ > + if (balloon_stats_enabled(s)) { > + qemu_del_timer(s->stats_timer); > + qemu_free_timer(s->stats_timer); > + s->stats_timer = NULL; > + s->stats_poll_interval = 0; > + } > +} > + > +static void balloon_stats_change_timer(VirtIOBalloon *s, int secs) > +{ > + qemu_mod_timer(s->stats_timer, qemu_get_clock_ms(vm_clock) + secs * 1000); > +} > + > +static void balloon_stats_poll_cb(void *opaque) > +{ > + VirtIOBalloon *s = opaque; > + > + virtqueue_push(s->svq, &s->stats_vq_elem, s->stats_vq_offset); > + virtio_notify(&s->vdev, s->svq); I think we'll want to add some logic to avoid the potential for re-pushing an elem we've already processed, as I think that violates the virtio spec. In the past the monitor blocked us from doing this but with timer-driven requests I think it's a possibility now. But the general approach seems sane to me and the code looks to be in decent shape. > -- > 1.8.0 > >