[libvirt] [PATCH RFC 8/8] add qemu cache mutex

[Lee Schermerhorn]

On Mon, 2012-03-12 at 10:16 -0400, Lee Schermerhorn wrote:
> On Mon, 2012-03-12 at 14:00 +0100, Michal Privoznik wrote:
> > On 11.03.2012 19:56, Lee Schermerhorn wrote:
> > > Add a mutex for access to the qemu emulator cache.  Not clear that
> > > this is actually needed -- driver should be locked across calls [?].
> > > The patch can be dropped if not needed.
> > > ---
> > >  src/qemu/qemu_capabilities.c |   18 +++++++++++++++++-
> > >  src/qemu/qemu_capabilities.h |    2 ++
> > >  src/qemu/qemu_driver.c       |    3 +++
> > >  3 files changed, 22 insertions(+), 1 deletion(-)
> > > 
> > > Index: libvirt-0.9.10/src/qemu/qemu_capabilities.c
> > > ===================================================================
> > > --- libvirt-0.9.10.orig/src/qemu/qemu_capabilities.c
> > > +++ libvirt-0.9.10/src/qemu/qemu_capabilities.c
> > > @@ -27,6 +27,7 @@
> > >  #include "memory.h"
> > >  #include "logging.h"
> > >  #include "virterror_internal.h"
> > > +#include "threads.h"
> > >  #include "util.h"
> > >  #include "virfile.h"
> > >  #include "nodeinfo.h"
> > > @@ -180,6 +181,11 @@ enum qemuCapsProbes {
> > >      QEMU_PROBE_SIZE
> > >  };
> > >  
> > > +/*
> > > + * Use static initializer for tests
> > > + */
> > > +static virMutex qemuEmulatorCacheMutex = { PTHREAD_MUTEX_INITIALIZER };
> > 
> > This is not allowed in our code as we build with win32 threads which
> > initialize mutexes completely different. Why do you want to initialize
> > it here anyway ...
> 
> 
> Thanks.  I didn't know that.
> 
> As the comment says, I added it for the internal tests.  It appeared to
> me that they don't go through the driver init code where I inserted the
> call to to the init function below.  The tests were hanging at the first
> attempt to acquire the mutex.  It could have been a defect in my patches
> at that time [that may still be there?].  When I inserted the static
> initializer, the tests passed.  That was back on the 0.8.8 ubuntu natty
> code base.  I'll pull it out and see if they pass w/o it, now that the
> tests all seem to pass otherwise.  They certainly pass w/o this patch
> applied, but they're all single threaded, right?

Update:  'make check' tests [qemuxml2argv] succeed with static
initializer removed.  Previous failures were apparently either an
artifact of the 0.8.8-1ubuntu6.x code base or a defect elsewhere in my
patches at the time.  I was still debugging then, trying to get tests to
pass.  Question whether the mutex is needed still stands, but since the
driver lock is RW, it probably is.  cache mutex could probably be made
RW as well, and only taken for W when filling/refreshing the cache.

I'm running more tests and will update the series when/if I hear that
it's worth doing so.

Regards,
Lee
> 
> Bigger question is:  is the mutex actually needed at all?  I.e., can I
> assume that the driver is always locked -- in practice, not necessarily
> for the tests -- when the probes are called?
> 
> Lee
> 
> > > +
> > >  typedef struct _qemuEmulatorCache qemuEmulatorCache;
> > >  typedef qemuEmulatorCache* qemuEmulatorCachePtr;
> > >  struct _qemuEmulatorCache {
> > > @@ -206,9 +212,17 @@ qemuEmulatorCachedInfoGet(enum qemuCapsP
> > >                            const char *binary,
> > >                            const char *arch);
> > >  
> > > +int
> > > +qemuCapsCacheInit(void)
> > > +{
> > > +    return virMutexInit(&qemuEmulatorCacheMutex);
> > > +}
> > > +
> > 
> > if you have created this function?
> > >  static void
> > >  qemuEmulatorCachedInfoRelease(qemuEmulatorCachePtr emulator ATTRIBUTE_UNUSED)
> > > -{ }
> > > +{
> > > +    virMutexUnlock(&qemuEmulatorCacheMutex);
> > > +}
> > >  
>