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

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(a)redhat.com&gt; --- 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); +}

@@ -347,26 +426,39 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, } + /* Get a READ lock and immediately promote to WRITE while we adjust + * data within. */ if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { objdef = obj->def; if (virNWFilterDefEqual(def, objdef)) { + virNWFilterObjPromoteToWrite(obj); virNWFilterDefFree(objdef); obj->def = def; + virNWFilterObjDemoteFromWrite(obj); return obj; }

+ /* Set newDef and run the trigger with a demoted lock since it may need + * to grab a read lock on this object and promote before returning. */ + virNWFilterObjPromoteToWrite(obj); obj->newDef = def; + virNWFilterObjDemoteFromWrite(obj); + /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild() < 0) { + virNWFilterObjPromoteToWrite(obj); obj->newDef = NULL; - virNWFilterObjUnlock(obj); + /* NB: We're done, no need to Demote and End, just End */ + virNWFilterObjEndAPI(&obj); return NULL; } + virNWFilterObjPromoteToWrite(obj); virNWFilterDefFree(objdef); obj->def = def; obj->newDef = NULL; + virNWFilterObjDemoteFromWrite(obj); return obj; }