On 07/11/2013 10:05 AM, Michal Privoznik wrote:
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;
yep...
+ 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.
Funny - I did have one at one time along with a boatload more to print each return along the way. It got lost while I was stripping out the extraneous debug code.
+ } + } + + /* 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/
balloonpath is the variable name...
+ * 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?
I struggled with this return as well when I first added the code - I think this was added before saving the path and before checking in the path code that we our path had a polling-interval property. So, considering the "current" design it's duplicitous to get the value and check it against the current setting before setting, so I've removed it.
+ } + + 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.
OK gone.
+ 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?
Before I had a way to dynamically set the period I only had the period in the XML file, thus this was the only way to "get" the stats to show up for a running guest when I installed the new code and restarted libvirtd. Tks, John
/* 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