[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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;
>    }
> 
>> +
>> +    /* 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
> 

I've squashed the following in:


diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 424af38..82d5959 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -960,14 +960,20 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon,
     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)
+    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");
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("Memory balloon model must be virtio to "
+                         "get memballoon path"));
         return -1;
     }
 
@@ -1001,7 +1007,9 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon,
             }
 
             /* If we get here, we found the path, but not the property */
-            VIR_DEBUG("Property 'guest-stats-polling-interval' not found");
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("Property 'guest-stats-polling-interval' "
+                             "not found on memory balloon driver."));
             ret = -1;
             goto cleanup;
         }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 239c65f..3d5e8f6 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3883,9 +3883,8 @@ int qemuProcessStart(virConnectPtr conn,
         goto cleanup;
     }
     qemuDomainObjEnterMonitor(driver, vm);
-    if (vm->def->memballoon)
-        qemuMonitorSetMemoryStatsPeriod(priv->mon,
-                                        vm->def->memballoon->period);
+    if (vm->def->memballoon && vm->def->memballoon->period)
+        qemuMonitorSetMemoryStatsPeriod(priv->mon, vm->def->memballoon->period)
     if (qemuMonitorSetBalloon(priv->mon, cur_balloon) < 0) {
         qemuDomainObjExitMonitor(driver, vm);
         goto cleanup;
@@ -4415,7 +4414,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
     if (running) {
         virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
                              VIR_DOMAIN_RUNNING_UNPAUSED);
-        if (vm->def->memballoon) {
+        if (vm->def->memballoon && vm->def->memballoon->period) {
             qemuDomainObjEnterMonitor(driver, vm);
             qemuMonitorSetMemoryStatsPeriod(priv->mon,
                                             vm->def->memballoon->period);


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]