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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list