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