[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 Fri, Jul 12, 2013 at 09:15:40AM -0400, John Ferlan wrote:
> 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.

The scenario is that an application invokes virDomainSetMemoryStatsPeriod().
This calls qemuMonitorFindBalloonObjectPath(). If that returns -1, then
virDomainSetMemoryStatsPeriod will return -1. For any function that returns
a -1 error condition it is mandatory to have called 'virReportError',
otherwise the caller will just see "unknown error occurred".

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


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