On 07/12/2013 08:39 AM, Daniel P. Berrange wrote:
> 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;
> }
>
This is a recursive function - setting ballooninit in here means I have
to know when I'm back at the first call. That's why it's set in the
callers. Since ballooninit means I've either made the call and was
successful or I've made the call and wasn't successful.
I think an argument could be made that the checking of balloonpath
probably is extraneous.
Since the balloon driver stats are only supported in qemu 1.5 and
beyond, do we really want an error message?
>> +
>> + /* 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.
>
hmm... Do we really want to virReportError() in either case? They're
not necessarily specifically asking for the statistics here... Although
with your suggestion to only call SetMemoryStatsPeriod if the period is
defined, I do see the point. Although the first time someone does a
'virsh dommemstats domain' they'd see this message and perhaps wonder.
Also if the user didn't have the balloon driver configured, then this
would be spit out.
>> + 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
>
It's possible with 'earlier' versions of qemu (eg, pre 1.5) that we
won't find the property. Thus if we provide an error message here, then
couldn't that be a "false positive" to the user?
>> + 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);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
hence why ballooninit is set outside the context of this function.
>> + }
>> + }
>> +
>> +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.
>
Ah - sure. That's fine.
>> + 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.
>
Yep - I can do that.
These were done this way because the only way to get the path prior to
patch 6 is if we called the setting of the period. In patch 6 I added
the call to get the path in the GetStats logic to check for the path as
well; otherwise, the Reconnect'd guest wouldn't find its path and thus
wouldn't show the stats...
>> + 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list