[libvirt] [PATCH v5 1/3] nwfilter: Convert _virNWFilterObj to use virObjectRWLockable

[Daniel P. Berrangé]

On Thu, Feb 08, 2018 at 02:13:58PM +0100, Michal Privoznik wrote:
> On 02/06/2018 08:20 PM, John Ferlan wrote:
> > Unlike it's counterparts, the nwfilter object code needs to be able
> > to support recursive read locks while processing and checking new
> > filters against the existing environment. Thus instead of using a
> > virObjectLockable which uses pure mutexes, use the virObjectRWLockable
> > and primarily use RWLockRead when obtaining the object lock since
> > RWLockRead locks can be recursively obtained (up to a limit) as long
> > as there's a corresponding unlock.
> > 
> > Since all the object management is within the virnwfilterobj code, we
> > can limit the number of Write locks on the object to very small areas
> > of code to ensure we don't run into deadlock's with other threads and
> > domain code that will check/use the filters (it's a very delicate
> > balance). This limits the write locks to AssignDef and Remove processing.
> > 
> > This patch will introduce a new API virNWFilterObjEndAPI to unlock
> > and deref the object.
> > 
> > This patch introduces two helpers to promote/demote the read/write lock.
> > 
> > Signed-off-by: John Ferlan <jferlan at redhat.com>
> > ---
> >  src/conf/virnwfilterobj.c              | 193 +++++++++++++++++++++++----------
> >  src/conf/virnwfilterobj.h              |   9 +-
> >  src/libvirt_private.syms               |   3 +-
> >  src/nwfilter/nwfilter_driver.c         |  15 +--
> >  src/nwfilter/nwfilter_gentech_driver.c |  11 +-
> >  5 files changed, 153 insertions(+), 78 deletions(-)
> > 
> > diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> > index 87d7e7270..cd52706ec 100644
> > --- a/src/conf/virnwfilterobj.c
> > +++ b/src/conf/virnwfilterobj.c
> > @@ -34,7 +34,7 @@
> >  VIR_LOG_INIT("conf.virnwfilterobj");
> >  
> >  struct _virNWFilterObj {
> > -    virMutex lock;
> > +    virObjectRWLockable parent;
> >  
> >      bool wantRemoved;
> >  
> > @@ -47,27 +47,69 @@ struct _virNWFilterObjList {
> >      virNWFilterObjPtr *objs;
> >  };
> >  
> > +static virClassPtr virNWFilterObjClass;
> > +static void virNWFilterObjDispose(void *opaque);
> > +
> > +
> > +static int
> > +virNWFilterObjOnceInit(void)
> > +{
> > +    if (!(virNWFilterObjClass = virClassNew(virClassForObjectRWLockable(),
> > +                                            "virNWFilterObj",
> > +                                            sizeof(virNWFilterObj),
> > +                                            virNWFilterObjDispose)))
> > +        return -1;
> > +
> > +    return 0;
> > +}
> > +
> > +VIR_ONCE_GLOBAL_INIT(virNWFilterObj)
> > +
> >  
> >  static virNWFilterObjPtr
> >  virNWFilterObjNew(void)
> >  {
> >      virNWFilterObjPtr obj;
> >  
> > -    if (VIR_ALLOC(obj) < 0)
> > +    if (virNWFilterObjInitialize() < 0)
> >          return NULL;
> >  
> > -    if (virMutexInitRecursive(&obj->lock) < 0) {
> > -        virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                       "%s", _("cannot initialize mutex"));
> > -        VIR_FREE(obj);
> > +    if (!(obj = virObjectRWLockableNew(virNWFilterObjClass)))
> >          return NULL;
> > -    }
> >  
> > -    virNWFilterObjLock(obj);
> > +    virObjectRWLockWrite(obj);
> >      return obj;
> >  }
> >  
> >  
> > +static void
> > +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
> > +{
> > +    virObjectRWUnlock(obj);
> > +    virObjectRWLockWrite(obj);
> > +}
> 
> How can this not deadlock? This will work only if @obj is locked exactly
> once. But since we allow recursive locks any lock() that happens in the
> 2nd level must deadlock with this. On the other hand, there's no such
> case in the code currently. But that doesn't stop anybody from calling
> PromoteWrite() without understanding its limitations.
> 
> Maybe the fact that we need recursive lock for NWFilterObj means it's
> not a good candidate for virObjectRWLocable? It is still a good
> candidate for virObject though.
> 
> Or if you want to spend extra time on this, maybe introducing
> virObjectRecursiveLockable may be worth it (terrible name, I know).

I can't remember exactly call trace scenarios that meant we required
recursive locking. I vaguely recall is was related to fact that we
have an alternative entry point into the nwfilter code that's triggered
by the virt drivers. I'm thus vaguely hoping that when we split nwfilter
off into a separate daemon from virt driver, the need for recursive
lockingn would magically disappear. I could be completely wrong though :-)


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|