[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 05/10] Determine whether to start balloon memory stats gathering.



On Thu, Jul 11, 2013 at 07:55:55PM -0400, John Ferlan wrote:
> At vm startup and attach attempt to set the balloon driver statistics
> collection period based on the value found in the domain xml file. This
> is not done at reconnect since it's possible that a collection period
> was set on the live guest and making the set period call would reset to
> whatever value is stored in the config file.
> 
> Setting the stats collection period has a side effect of searching through
> the qom-list output for the virtio balloon driver and making sure that it
> has the right properties in order to allow setting of a collection period
> and eventually fetching of statistics.
> 
> The walk through the qom-list is expensive and thus the balloonpath will
> be saved in the monitor private structure as well as a flag indicating
> that the initialization has already been attempted (in the event that a
> path is not found, no sense to keep checking).
> 
> This processing model conforms to the qom object model model which
> requires setting object properties after device startup. That is, it's
> not possible to pass the period along via the startup code as it won't
> be recognized.
> ---
>  src/qemu/qemu_monitor.c      | 130 +++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_monitor.h      |   2 +
>  src/qemu/qemu_monitor_json.c |  24 ++++++++
>  src/qemu/qemu_monitor_json.h |   3 +
>  src/qemu/qemu_process.c      |  14 ++++-
>  5 files changed, 171 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 6f9a8fc..a3e250c 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -83,6 +83,10 @@ struct _qemuMonitor {
>  
>      /* cache of query-command-line-options results */
>      virJSONValuePtr options;
> +
> +    /* If found, path to the virtio memballoon driver */
> +    char *balloonpath;
> +    bool ballooninit;
>  };
>  
>  static virClassPtr qemuMonitorClass;
> @@ -248,6 +252,7 @@ static void qemuMonitorDispose(void *obj)
>      virCondDestroy(&mon->notify);
>      VIR_FREE(mon->buffer);
>      virJSONValueFree(mon->options);
> +    VIR_FREE(mon->balloonpath);
>  }
>  
>  
> @@ -925,6 +930,105 @@ qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options)
>      mon->options = options;
>  }
>  
> +/* Search the qom objects for the balloon driver object by it's known name
> + * of "virtio-balloon-pci".  The entry for the driver will be found in the
> + * returned 'type' field using the syntax "child<virtio-balloon-pci>".
> + *
> + * Once found, check the entry to ensure it has the correct property listed.
> + * If it does not, then obtaining statistics from qemu will not be possible.
> + * This feature was added to qemu 1.5.
> + *
> + * This procedure will be call recursively until found or the qom-list is
> + * exhausted.
> + *
> + * Returns:
> + *
> + *   1  - Found
> + *   0  - Not found still looking
> + *  -1  - Error bail out
> + *
> + * NOTE: This assumes we have already called qemuDomainObjEnterMonitor()
> + */
> +static int
> +qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon,
> +                                 virDomainObjPtr vm,
> +                                 const char *curpath)
> +{
> +    size_t i, j, npaths = 0, nprops = 0;
> +    int ret = 0;
> +    char *nextpath = NULL;
> +    qemuMonitorJSONListPathPtr *paths = NULL;
> +    qemuMonitorJSONListPathPtr *bprops = NULL;
> +
> +    /* Already set and won't change or we already search and failed to find */
> +    if (mon->balloonpath || mon->ballooninit)
> +        return 1;

This isn't correct logic. You need

   if (mon->balloonpath) {
     return 1;
   } else if (mon->ballooninit) {
      virReportError(VIR_ERR_INTERNAL_ERROR, "%s"
                     _("Cannot determine balloon device path"));
      return -1;
   }

> +
> +    /* Not supported */
> +    if (!vm->def->memballoon ||
> +        vm->def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
> +        VIR_DEBUG("Model must be virtio to get memballoon path");

You need to use virReportError here, so the caller sees an error
messages.

> +        return -1;
> +    }
> +
> +    VIR_DEBUG("Searching for Balloon Object Path starting at %s", curpath);
> +
> +    npaths = qemuMonitorJSONGetObjectListPaths(mon, curpath, &paths);
> +
> +    for (i = 0; i < npaths && ret == 0; i++) {
> +
> +        if (STREQ_NULLABLE(paths[i]->type, "link<virtio-balloon-pci>")) {
> +            VIR_DEBUG("Path to <virtio-balloon-pci> is '%s/%s'",
> +                      curpath, paths[i]->name);
> +            if (virAsprintf(&nextpath, "%s/%s", curpath, paths[i]->name) < 0) {
> +                ret = -1;
> +                goto cleanup;
> +            }
> +
> +            /* Now look at the each of the property entries to determine
> +             * whether "guest-stats-polling-interval" exists.  If not,
> +             * then this version of qemu/kvm does not support the feature.
> +             */
> +            nprops = qemuMonitorJSONGetObjectListPaths(mon, nextpath, &bprops);
> +            for (j = 0; j < nprops; j++) {
> +                if (STREQ(bprops[j]->name, "guest-stats-polling-interval")) {
> +                    VIR_DEBUG("Found Balloon Object Path %s", nextpath);
> +                    mon->balloonpath = nextpath;
> +                    nextpath = NULL;
> +                    ret = 1;
> +                    goto cleanup;
> +                }
> +            }
> +
> +            /* If we get here, we found the path, but not the property */
> +            VIR_DEBUG("Property 'guest-stats-polling-interval' not found");
> +            ret = -1;

And virReportERror here too

> +            goto cleanup;
> +        }
> +
> +        /* Type entries that begin with "child<" are a branch that can be
> +         * traversed looking for more entries
> +         */
> +        if (paths[i]->type && STRPREFIX(paths[i]->type, "child<")) {
> +            if (virAsprintf(&nextpath, "%s/%s", curpath, paths[i]->name) < 0) {
> +                ret = -1;
> +                goto cleanup;
> +            }
> +            ret = qemuMonitorFindBalloonObjectPath(mon, vm, nextpath);
> +        }
> +    }
> +
> +cleanup:
> +    for (i = 0; i < npaths; i++)
> +        qemuMonitorJSONListPathFree(paths[i]);
> +    VIR_FREE(paths);
> +    for (j = 0; j < nprops; j++)
> +        qemuMonitorJSONListPathFree(bprops[j]);
> +    VIR_FREE(bprops);
> +    VIR_FREE(nextpath);
> +    return ret;
> +}
> +
>  int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon,
>                                  const char *cmd,
>                                  int scm_fd,
> @@ -1386,6 +1490,32 @@ int qemuMonitorGetMemoryStats(qemuMonitorPtr mon,
>      return ret;
>  }
>  
> +int qemuMonitorSetMemoryStatsPeriod(qemuMonitorPtr mon,
> +                                    int period)
> +{
> +    int ret = -1;
> +    VIR_DEBUG("mon=%p period=%d", mon, period);
> +
> +    if (!mon) {
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
> +                       _("monitor must not be NULL"));
> +        return -1;
> +    }
> +
> +    if (!mon->json) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("JSON monitor is required"));
> +        return -1;
> +    }
> +
> +    if (qemuMonitorFindBalloonObjectPath(mon, mon->vm, "/") == 1) {
> +        ret = qemuMonitorJSONSetMemoryStatsPeriod(mon, mon->balloonpath,
> +                                                  period);
> +    }
> +    mon->ballooninit = true;
> +    return ret;
> +}
> +
>  int
>  qemuMonitorBlockIOStatusToError(const char *status)
>  {
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 78011ee..12b730a 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -265,6 +265,8 @@ int qemuMonitorGetBalloonInfo(qemuMonitorPtr mon,
>  int qemuMonitorGetMemoryStats(qemuMonitorPtr mon,
>                                virDomainMemoryStatPtr stats,
>                                unsigned int nr_stats);
> +int qemuMonitorSetMemoryStatsPeriod(qemuMonitorPtr mon,
> +                                    int period);
>  
>  int qemuMonitorBlockIOStatusToError(const char *status);
>  virHashTablePtr qemuMonitorGetBlockInfo(qemuMonitorPtr mon);
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 92d458c..a9e8723 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1488,6 +1488,30 @@ cleanup:
>  }
>  
>  
> +/*
> + * Using the provided balloonpath, determine if we need to set the
> + * collection interval property to enable statistics gathering.
> + */
> +int
> +qemuMonitorJSONSetMemoryStatsPeriod(qemuMonitorPtr mon,
> +                                    char *balloonpath,
> +                                    int period)
> +{
> +    qemuMonitorJSONObjectProperty prop;
> +
> +    /* Set to the value in memballoon (could enable or disable) */
> +    memset(&prop, 0, sizeof(qemuMonitorJSONObjectProperty));
> +    prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT;
> +    prop.val.iv = period;
> +    if (qemuMonitorJSONSetObjectProperty(mon, balloonpath,
> +                                         "guest-stats-polling-interval",
> +                                         &prop) < 0) {
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +
>  int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon,
>                                  virHashTablePtr table)
>  {
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index a857d86..e2324f1 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -61,6 +61,9 @@ int qemuMonitorJSONGetBalloonInfo(qemuMonitorPtr mon,
>  int qemuMonitorJSONGetMemoryStats(qemuMonitorPtr mon,
>                                    virDomainMemoryStatPtr stats,
>                                    unsigned int nr_stats);
> +int qemuMonitorJSONSetMemoryStatsPeriod(qemuMonitorPtr mon,
> +                                        char *balloonpath,
> +                                        int period);
>  int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon,
>                                  virHashTablePtr table);
>  int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 574abf2..239c65f 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3883,6 +3883,9 @@ int qemuProcessStart(virConnectPtr conn,
>          goto cleanup;
>      }
>      qemuDomainObjEnterMonitor(driver, vm);
> +    if (vm->def->memballoon)

Should that be   vm->def->memballoon && vm->def->memballoon->period.

eg we don't want to call this if no balloon stats period was set
in the XML.

> +        qemuMonitorSetMemoryStatsPeriod(priv->mon,
> +                                        vm->def->memballoon->period);
>      if (qemuMonitorSetBalloon(priv->mon, cur_balloon) < 0) {
>          qemuDomainObjExitMonitor(driver, vm);
>          goto cleanup;
> @@ -4409,11 +4412,18 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
>      if (!virDomainObjIsActive(vm))
>          goto cleanup;
>  
> -    if (running)
> +    if (running) {
>          virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
>                               VIR_DOMAIN_RUNNING_UNPAUSED);
> -    else
> +        if (vm->def->memballoon) {

Same here.

> +            qemuDomainObjEnterMonitor(driver, vm);
> +            qemuMonitorSetMemoryStatsPeriod(priv->mon,
> +                                            vm->def->memballoon->period);
> +            qemuDomainObjExitMonitor(driver, vm);
> +        }
> +    } else {
>          virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, reason);
> +    }
>  
>      VIR_DEBUG("Writing domain status to disk");
>      if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]