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

On 08.07.2013 21:20, John Ferlan wrote: > At vm startup, reconnect, and attach - check for the presence of the balloon > driver and save the path in the private area of the driver. This path will > remain constant throughout the life of the domain and can then be used rather > than attempting to find the path each time balloon driver statistics are > fetched or the collection period changes. The qom object model model 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. > If a balloon driver path is found a check of the existing collection period > will be made against the saved domain value in order to determine if an > adjustment needs to be made to the period to start or stop collecting stats > --- > src/qemu/qemu_monitor.c | 130 +++++++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_monitor.h | 2 + > src/qemu/qemu_monitor_json.c | 42 ++++++++++++++ > src/qemu/qemu_monitor_json.h | 3 + > src/qemu/qemu_process.c | 21 ++++++- > 5 files changed, 196 insertions(+), 2 deletions(-) > > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index fe5ab0a..038c9e8 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -83,6 +83,9 @@ struct _qemuMonitor { > > /* cache of query-command-line-options results */ > virJSONValuePtr options; > + > + /* If found, path to the virtio memballoon driver */ > + char *balloonpath; > }; > > static virClassPtr qemuMonitorClass; > @@ -248,6 +251,7 @@ static void qemuMonitorDispose(void *obj) > virCondDestroy(&mon->notify); > VIR_FREE(mon->buffer); > virJSONValueFree(mon->options); > + VIR_FREE(mon->balloonpath); > } > > > @@ -929,6 +933,107 @@ 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) > +{ > + int i,j; size_t i,j, npaths = 0, nprops = 0;

> + int npaths = 0; > + int nprops = 0; > + int ret = 0; > + char *nextpath = NULL; > + qemuMonitorJSONListPathPtr *paths = NULL; > + qemuMonitorJSONListPathPtr *bprops = NULL; > + > + /* 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"); > + return -1; > + } > + > + /* Already set and won't change */ > + if (mon->balloonpath) > + 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")) { > + mon->balloonpath = nextpath; > + nextpath = NULL; > + ret = 1; > + goto cleanup; Maybe I'd add VIR_DEBUG() here to log the path.

> + } > + } > + > + /* If we get here, we found the path, but not the property */ > + VIR_DEBUG("Property 'guest-stats-polling-interval' not found"); > + ret = -1; > + 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) { > + virReportOOMError(); > + 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, > @@ -1390,6 +1495,31 @@ 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); > + } > + 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 9487955..2d7f9b6 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -1495,6 +1495,48 @@ cleanup: > } > > > +/* > + * Using the provided balloonpath, determine if we need to set the s/balloonpath/balloon path/

> + * collection interval property to enable statistics gathering. > + */ > +int > +qemuMonitorJSONSetMemoryStatsPeriod(qemuMonitorPtr mon, > + char *balloonpath, > + int period) > +{ > + qemuMonitorJSONObjectProperty prop; > + > + /* Get the current value of the stats polling interval */ > + memset(&prop, 0, sizeof(qemuMonitorJSONObjectProperty)); > + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; > + if (qemuMonitorJSONGetObjectProperty(mon, balloonpath, > + "guest-stats-polling-interval", > + &prop) < 0) { > + VIR_DEBUG("Failed to get polling interval for balloon driver"); > + return 0; Shouldn't we return -1 here? I mean, if caller requires us to set an interval, we should error out if we fail. Why are we querying for the interval prior setting it anyway?

> + } > + > + VIR_DEBUG("memballoon period=%d 'guest-stats-polling-interval'=%d", > + period, prop.val.i); > + > + /* Same value - no need to set */ > + if (period == prop.val.i) > + return 0; > + > + /* Set to the value in memballoon (could enable or disable) */ > + memset(&prop, 0, sizeof(qemuMonitorJSONObjectProperty)); > + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; > + prop.val.i = period; > + if (qemuMonitorJSONSetObjectProperty(mon, balloonpath, > + "guest-stats-polling-interval", > + &prop) < 0) { > + VIR_DEBUG("Failed to set polling interval for balloon driver"); This is unnecessary as long as qemuMonitorJSONSetObjectProperty reports error on retval < 0, which it does.

> + 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 fd0dedd..b0068ff 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 f8c652f..fd3b7a8 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -3091,6 +3091,13 @@ qemuProcessReconnect(void *opaque) > if (qemuProcessRecoverJob(driver, obj, conn, &oldjob) < 0) > goto error; > > + if (obj->def->memballoon) { > + qemuDomainObjEnterMonitor(driver, obj); > + qemuMonitorSetMemoryStatsPeriod(priv->mon, > + obj->def->memballoon->period); > + qemuDomainObjExitMonitor(driver, obj); > + } > + Why do we want to enable this on ProcessReconnect?

> /* update domain state XML with possibly updated state in virDomainObj */ > if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, obj) < 0) > goto error; > @@ -3910,6 +3917,9 @@ int qemuProcessStart(virConnectPtr conn, > goto cleanup; > } > qemuDomainObjEnterMonitor(driver, vm); > + if (vm->def->memballoon) > + qemuMonitorSetMemoryStatsPeriod(priv->mon, > + vm->def->memballoon->period); It makes sense here. > if (qemuMonitorSetBalloon(priv->mon, cur_balloon) < 0) { > qemuDomainObjExitMonitor(driver, vm); > goto cleanup; > @@ -4439,11 +4449,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) { > + 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) > Michal