Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Luiz Capitulino]

On Thu, 9 Feb 2012 13:26:40 -0600
Adam Litke <address@hidden> wrote:

> On Wed, Feb 08, 2012 at 06:30:40PM -0200, Luiz Capitulino wrote:
> > This commit adds a QMP API for the guest provided memory statistics
> > (long disabled by commit 07b0403dfc2b2ac179ae5b48105096cc2d03375a).
> > 
> > The approach taken by the original commit
> > (625a5befc2e3200b396594f002218d235e375da5) was to extend the
> > query-balloon command. It introduced a severe bug though: query-balloon
> > would hang if the guest didn't respond.
> > 
> > The approach taken by this commit is asynchronous and thus avoids
> > any QMP hangs.
> > 
> > First, a client has to issue the balloon-get-memory-stats command.
> > That command gets the process started by only sending a request to
> > the guest, it doesn't block. When the memory stats are made available
> > by the guest, they are returned to the client as an QMP event.
> > 
> > Signed-off-by: Luiz Capitulino <address@hidden>
> > ---
> >  QMP/qmp-events.txt  |   36 ++++++++++++++++++++++++
> >  balloon.c           |   30 +++++++++++++++++++-
> >  balloon.h           |    4 ++-
> >  hw/virtio-balloon.c |   76 
> > ++++++++++++++++++++++++++++++++++-----------------
> >  monitor.c           |    2 +
> >  monitor.h           |    1 +
> >  qapi-schema.json    |   21 ++++++++++++++
> >  qmp-commands.hx     |    5 +++
> >  8 files changed, 147 insertions(+), 28 deletions(-)
> > 
> > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> > index 06cb404..b34b289 100644
> > --- a/QMP/qmp-events.txt
> > +++ b/QMP/qmp-events.txt
> > @@ -1,6 +1,42 @@
> >                     QEMU Monitor Protocol Events
> >                     ============================
> > 
> > +BALLOON_MEMORY_STATS
> > +--------------------
> > +
> > +Emitted when memory statistics information is made available by the guest.
> > +
> > +Data:
> > +
> > +- "memory-swapped-in": number of pages swapped in within the guest
> > +  (json-int, optional)
> > +- "memory-swapped-out": number of pages swapped out within the guest
> > +  (json-int, optional)
> 
> These are in units of pages where memory-total and memory-free are in bytes.
> Maybe it makes sense to name these "memory-pages-swapped-in/out" to avoid
> confusion.  Yes it makes them longer, but I can imagine all of the questions 
> in
> the future when users don't expect the units to be in pages.

Seems like a good idea.

> 
> > +- "major-page-faults": number of major page faults within the guest
> > +  (json-int, optional)
> > +- "minor-page-faults": number of minor page faults within the guest
> > +  (json-int, optional)
> > +- "memory-free": amount of memory (in bytes) free in the guest
> > +  (json-int, optional)
> > +- "memory-total": amount of memory (in bytes) visible to the guest
> > +  (json-int, optional)
> > +
> > +Example:
> > +
> > +{ "event": "BALLOON_MEMORY_STATS",
> > +    "data": { "memory-free": 847941632,
> > +              "major-page-faults": 225,
> > +              "memory-swapped-in": 0,
> > +              "minor-page-faults": 222317,
> > +              "memory-total": 1045516288,
> > +              "memory-swapped-out": 0 },
> > +    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> > +
> > +Notes: o The balloon-get-memory-stats command must be issued first, to tell
> > +         the guest to make the memory statistics available.
> > +       o The event may not contain all data members when emitted
> > +
> > +
> >  BLOCK_IO_ERROR
> >  --------------
> > 
> > diff --git a/balloon.c b/balloon.c
> > index d340ae3..1c1a3c1 100644
> > --- a/balloon.c
> > +++ b/balloon.c
> > @@ -33,12 +33,15 @@
> > 
> >  static QEMUBalloonEvent *balloon_event_fn;
> >  static QEMUBalloonInfo *balloon_info_fn;
> > +static QEMUBalloonStats *balloon_stats_fn;
> >  static void *balloon_opaque;
> > 
> >  int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
> > -                             QEMUBalloonInfo *info_func, void *opaque)
> > +                             QEMUBalloonInfo *info_func,
> > +                             QEMUBalloonStats *stats_func, void *opaque)
> >  {
> > -    if (balloon_event_fn || balloon_info_fn || balloon_opaque) {
> > +    if (balloon_event_fn || balloon_info_fn || balloon_stats_fn ||
> > +        balloon_opaque) {
> >          /* We're already registered one balloon handler.  How many can
> >           * a guest really have?
> >           */
> > @@ -47,6 +50,7 @@ int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
> >      }
> >      balloon_event_fn = event_func;
> >      balloon_info_fn = info_func;
> > +    balloon_stats_fn = stats_func;
> >      balloon_opaque = opaque;
> >      return 0;
> >  }
> > @@ -58,6 +62,7 @@ void qemu_remove_balloon_handler(void *opaque)
> >      }
> >      balloon_event_fn = NULL;
> >      balloon_info_fn = NULL;
> > +    balloon_stats_fn = NULL;
> >      balloon_opaque = NULL;
> >  }
> > 
> > @@ -80,6 +85,15 @@ static int qemu_balloon_info(BalloonInfo *info)
> >      return 1;
> >  }
> > 
> > +static int qemu_balloon_stats(void)
> > +{
> > +    if (!balloon_stats_fn) {
> > +        return 0;
> > +    }
> > +    balloon_stats_fn(balloon_opaque);
> > +    return 1;
> > +}
> > +
> >  static bool check_kvm_sync_mmu(Error **errp)
> >  {
> >      if (kvm_enabled() && !kvm_has_sync_mmu()) {
> > @@ -89,6 +103,18 @@ static bool check_kvm_sync_mmu(Error **errp)
> >      return true;
> >  }
> > 
> > +void qmp_balloon_get_memory_stats(Error **errp)
> > +{
> > +    if (!check_kvm_sync_mmu(errp)) {
> > +        return;
> > +    }
> > +
> > +    if (qemu_balloon_stats() == 0) {
> > +        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
> > +        return;
> > +    }
> > +}
> > +
> >  BalloonInfo *qmp_query_balloon(Error **errp)
> >  {
> >      BalloonInfo *info;
> > diff --git a/balloon.h b/balloon.h
> > index a539354..509e477 100644
> > --- a/balloon.h
> > +++ b/balloon.h
> > @@ -18,9 +18,11 @@
> > 
> >  typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
> >  typedef void (QEMUBalloonInfo)(void *opaque, BalloonInfo *info);
> > +typedef void (QEMUBalloonStats)(void *opaque);
> > 
> >  int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
> > -                        QEMUBalloonInfo *info_func, void *opaque);
> > +                        QEMUBalloonInfo *info_func, QEMUBalloonStats 
> > *stats_func,
> > +                 void *opaque);
> >  void qemu_remove_balloon_handler(void *opaque);
> > 
> >  #endif
> > diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> > index 4307f4c..8bc842c 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 "monitor.h"
> > +#include "qemu-objects.h"
> > 
> >  #if defined(__linux__)
> >  #include <sys/mman.h>
> > @@ -34,6 +36,7 @@ typedef struct VirtIOBalloon
> >      uint32_t num_pages;
> >      uint32_t actual;
> >      uint64_t stats[VIRTIO_BALLOON_S_NR];
> > +    bool stats_requested;
> >      VirtQueueElement stats_vq_elem;
> >      size_t stats_vq_offset;
> >      DeviceState *qdev;
> > @@ -56,12 +59,10 @@ static void balloon_page(void *addr, int deflate)
> >  /*
> >   * reset_stats - Mark all items in the stats array as unset
> >   *
> > - * This function needs to be called at device intialization and before
> > - * before updating to a set of newly-generated stats.  This will ensure 
> > that no
> > - * stale values stick around in case the guest reports a subset of the 
> > supported
> > - * statistics.
> > + * This function ensures that no stale values stick around in case the 
> > guest
> > + * reports a subset of the supported statistics.
> >   */
> > -static inline void reset_stats(VirtIOBalloon *dev)
> > +static void reset_stats(VirtIOBalloon *dev)
> >  {
> >      int i;
> >      for (i = 0; i < VIRTIO_BALLOON_S_NR; dev->stats[i++] = -1);
> > @@ -101,6 +102,38 @@ static void virtio_balloon_handle_output(VirtIODevice 
> > *vdev, VirtQueue *vq)
> >      }
> >  }
> > 
> > +static void emit_qmp_balloon_event(const VirtIOBalloon *dev)
> > +{
> > +    int i;
> > +    QDict *stats;
> > +    const struct stats_strings {
> > +        int stat_idx;
> > +        const char *stat_name;
> > +    } stats_strings[] = {
> > +        { VIRTIO_BALLOON_S_SWAP_IN, "memory-swapped-in" },
> > +        { VIRTIO_BALLOON_S_SWAP_OUT, "memory-swapped-out" },
> > +        { VIRTIO_BALLOON_S_MAJFLT, "major-page-faults" },
> > +        { VIRTIO_BALLOON_S_MINFLT, "minor-page-faults" },
> > +        { VIRTIO_BALLOON_S_MEMFREE, "memory-free" },
> > +        { VIRTIO_BALLOON_S_MEMTOT, "memory-total" },
> > +        { 0, NULL },
> > +    };
> > +
> > +    stats = qdict_new();
> > +    for (i = 0; stats_strings[i].stat_name != NULL; i++) {
> > +        if (dev->stats[i] != -1) {
> > +            qdict_put(stats, stats_strings[i].stat_name,
> > +                      qint_from_int(dev->stats[i]));
> > +        }
> > +    }
> > +
> > +    if (qdict_size(stats) > 0) {
> > +        monitor_protocol_event(QEVENT_BALLOON_STATS, QOBJECT(stats));
> > +    }
> > +
> > +    QDECREF(stats);
> > +}
> > +
> >  static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
> >  {
> >      VirtIOBalloon *s = DO_UPCAST(VirtIOBalloon, vdev, vdev);
> > @@ -108,7 +141,7 @@ static void virtio_balloon_receive_stats(VirtIODevice 
> > *vdev, VirtQueue *vq)
> >      VirtIOBalloonStat stat;
> >      size_t offset = 0;
> > 
> > -    if (!virtqueue_pop(vq, elem)) {
> > +    if (!virtqueue_pop(vq, elem) || !s->stats_requested) {
> >          return;
> >      }
> > 
> > @@ -128,6 +161,9 @@ static void virtio_balloon_receive_stats(VirtIODevice 
> > *vdev, VirtQueue *vq)
> >              s->stats[tag] = val;
> >      }
> >      s->stats_vq_offset = offset;
> > +    s->stats_requested = false;
> > +
> > +    emit_qmp_balloon_event(s);
> >  }
> > 
> >  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t 
> > *config_data)
> > @@ -156,31 +192,21 @@ static uint32_t 
> > virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
> >      return f;
> >  }
> > 
> > -static void virtio_balloon_info(void *opaque, BalloonInfo *info)
> > +static void virtio_balloon_stats(void *opaque)
> >  {
> >      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)) {
> > +        dev->stats_requested = true;
> >          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);
> > +}
> > 
> > +static void virtio_balloon_info(void *opaque, BalloonInfo *info)
> > +{
> > +    VirtIOBalloon *dev = opaque;
> >      info->actual = ram_size - ((uint64_t) dev->actual <<
> >                                 VIRTIO_BALLOON_PFN_SHIFT);
> >  }
> > @@ -236,18 +262,18 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
> >      s->vdev.get_features = virtio_balloon_get_features;
> > 
> >      ret = qemu_add_balloon_handler(virtio_balloon_to_target,
> > -                                   virtio_balloon_info, s);
> > +                                   virtio_balloon_info,
> > +                                   virtio_balloon_stats, s);
> >      if (ret < 0) {
> >          virtio_cleanup(&s->vdev);
> >          return NULL;
> >      }
> > 
> > +    s->stats_requested = false;
> >      s->ivq = virtio_add_queue(&s->vdev, 128, virtio_balloon_handle_output);
> >      s->dvq = virtio_add_queue(&s->vdev, 128, virtio_balloon_handle_output);
> >      s->svq = virtio_add_queue(&s->vdev, 128, virtio_balloon_receive_stats);
> > 
> > -    reset_stats(s);
> > -
> >      s->qdev = dev;
> >      register_savevm(dev, "virtio-balloon", -1, 1,
> >                      virtio_balloon_save, virtio_balloon_load, s);
> > diff --git a/monitor.c b/monitor.c
> > index aadbdcb..868f2a0 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -484,6 +484,8 @@ void monitor_protocol_event(MonitorEvent event, QObject 
> > *data)
> >              break;
> >          case QEVENT_BLOCK_JOB_CANCELLED:
> >              event_name = "BLOCK_JOB_CANCELLED";
> > +        case QEVENT_BALLOON_STATS:
> > +            event_name = "BALLOON_MEMORY_STATS";
> >              break;
> >          default:
> >              abort();
> > diff --git a/monitor.h b/monitor.h
> > index b72ea07..76e26c7 100644
> > --- a/monitor.h
> > +++ b/monitor.h
> > @@ -38,6 +38,7 @@ typedef enum MonitorEvent {
> >      QEVENT_SPICE_DISCONNECTED,
> >      QEVENT_BLOCK_JOB_COMPLETED,
> >      QEVENT_BLOCK_JOB_CANCELLED,
> > +    QEVENT_BALLOON_STATS,
> >      QEVENT_MAX,
> >  } MonitorEvent;
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 24a42e3..c4d8d0c 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1563,3 +1563,24 @@
> >  { 'command': 'qom-list-types',
> >    'data': { '*implements': 'str', '*abstract': 'bool' },
> >    'returns': [ 'ObjectTypeInfo' ] }
> > +
> > +##
> > +# @balloon-get-memory-stats
> > +#
> > +# Ask the guest's balloon driver for guest memory statistics.
> > +#
> > +# This command will only get the process started and will return 
> > immediately.
> > +# The BALLOON_MEMORY_STATS event will be emitted when the statistics
> > +# information is returned by the guest.
> > +#
> > +# Returns: nothing on success
> > +#          If the balloon driver is enabled but not functional because the 
> > KVM
> > +#          kernel module cannot support it, KvmMissingCap
> > +#          If no balloon device is present, DeviceNotActive
> > +#
> > +# Notes: There's no guarantees the guest will ever respond, thus the
> > +#        BALLOON_MEMORY_STATS event may never be emitted.
> > +#
> > +# Since: 1.1
> > +##
> > +{ 'command': 'balloon-get-memory-stats' }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index b5e2ab8..52c1fc3 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -2047,3 +2047,8 @@ EQMP
> >          .args_type  = "implements:s?,abstract:b?",
> >          .mhandler.cmd_new = qmp_marshal_input_qom_list_types,
> >      },
> > +    {
> > +        .name       = "balloon-get-memory-stats",
> > +        .args_type  = "",
> > +        .mhandler.cmd_new = qmp_marshal_input_balloon_get_memory_stats,
> > +    },
> > -- 
> > 1.7.9.111.gf3fb0.dirty
> > 
>