On Thu, 6 Dec 2012 11:03:21 -0600 mdroth <mdr...@linux.vnet.ibm.com> wrote:
> 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 <lcapitul...@redhat.com> > > --- > > > > 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 <sys/mman.h> > > @@ -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. I'll check that, thanks for the feedback Mike. > > But the general approach seems sane to me and the code looks to > be in decent shape. > > > -- > > 1.8.0 > > > > >