| Message ID | 1354633488-24112-3-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <lcapitulino@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. But the general approach seems sane to me and the code looks to be in decent shape. > -- > 1.8.0 > >
On Thu, 6 Dec 2012 11:03:21 -0600 mdroth <mdroth@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 <lcapitulino@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 > > > > >
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); + + balloon_stats_change_timer(s, s->stats_poll_interval); +} + +static void balloon_stats_get_last_update(Object *obj, struct Visitor *v, + void *opaque, const char *name, + Error **errp) +{ + VirtIOBalloon *s = opaque; + visit_type_int(v, &s->stats_last_update, name, errp); +} + +static void balloon_stats_get_stat(Object *obj, struct Visitor *v, + void *opaque, const char *name, Error **errp) +{ + VirtIOBalloon *s = opaque; + int i; + + for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) { + if (!strcmp(balloon_stat_names[i], name)) { + visit_type_int(v, (int64_t *) &s->stats[i], name, errp); + return; + } + } + + error_setg(errp, "invalid property name '%s'", name); +} + +static void balloon_stats_get_poll_interval(Object *obj, struct Visitor *v, + void *opaque, const char *name, + Error **errp) +{ + VirtIOBalloon *s = opaque; + visit_type_int(v, &s->stats_poll_interval, name, errp); +} + +static void balloon_stats_set_poll_interval(Object *obj, struct Visitor *v, + void *opaque, const char *name, + Error **errp) +{ + VirtIOBalloon *s = opaque; + int64_t value; + + if (!balloon_stats_supported(s)) { + error_setg(errp, "guest doesn\'t support balloon stats"); + return; + } + + visit_type_int(v, &value, name, errp); + if (error_is_set(errp)) { + return; + } + + if (value < 0) { + error_setg(errp, "timer value must be positive"); + return; + } + + if (value == s->stats_poll_interval) { + return; + } + + if (value == 0) { + /* timer=0 disables the timer */ + balloon_stats_disable_timer(s); + return; + } + + if (balloon_stats_enabled(s)) { + /* timer interval change */ + s->stats_poll_interval = value; + balloon_stats_change_timer(s, value); + return; + } + + /* create a new timer */ + g_assert(s->stats_timer == NULL); + s->stats_timer = qemu_new_timer_ms(vm_clock, balloon_stats_poll_cb, s); + s->stats_poll_interval = value; + balloon_stats_change_timer(s, 0); +} + static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) { VirtIOBalloon *s = to_virtio_balloon(vdev); @@ -107,11 +235,16 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq) VirtQueueElement *elem = &s->stats_vq_elem; VirtIOBalloonStat stat; size_t offset = 0; + qemu_timeval tv; if (!virtqueue_pop(vq, elem)) { return; } + if (!balloon_stats_enabled(s)) { + return; + } + /* Initialize the stats to get rid of any stale values. This is only * needed to handle the case where a guest supports fewer stats than it * used to (ie. it has booted into an old kernel). @@ -128,6 +261,13 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq) s->stats[tag] = val; } s->stats_vq_offset = offset; + + if (qemu_gettimeofday(&tv) < 0) { + fprintf(stderr, "warning: %s: failed to get time of day\n", __func__); + return; + } + + s->stats_last_update = tv.tv_sec; } static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data) @@ -212,7 +352,7 @@ static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id) VirtIODevice *virtio_balloon_init(DeviceState *dev) { VirtIOBalloon *s; - int ret; + int i, ret; s = (VirtIOBalloon *)virtio_common_init("virtio-balloon", VIRTIO_ID_BALLOON, @@ -239,6 +379,19 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev) register_savevm(dev, "virtio-balloon", -1, 1, virtio_balloon_save, virtio_balloon_load, s); + for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) { + object_property_add(OBJECT(dev), balloon_stat_names[i], "int", + balloon_stats_get_stat, NULL, NULL, s, NULL); + } + + object_property_add(OBJECT(dev), "stats-last-update", "int", + balloon_stats_get_last_update, NULL, NULL, s, NULL); + + object_property_add(OBJECT(dev), "stats-polling-interval", "int", + balloon_stats_get_poll_interval, + balloon_stats_set_poll_interval, + NULL, s, NULL); + return &s->vdev; } @@ -246,6 +399,7 @@ void virtio_balloon_exit(VirtIODevice *vdev) { VirtIOBalloon *s = DO_UPCAST(VirtIOBalloon, vdev, vdev); + balloon_stats_disable_timer(s); qemu_remove_balloon_handler(s); unregister_savevm(s->qdev, "virtio-balloon", s); virtio_cleanup(vdev);
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 <lcapitulino@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(-)