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

[Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx>]

On 02/08/2018 08:30 AM, Daniel P. Berrangé wrote:
> 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@xxxxxxxxxx>
> > > ---
> > >   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 :-)

The nwfilter code tries to detect circular references of filters, like 
a->b->c->a, and while testing for that needs to grab a read lock twice.

   Stefan

> Regards,
> Daniel


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list