[libvirt] [PATCH v2 4/6] nwfilter: Remove recursive locking for nwfilter instantiation

The current algorithm required usage of recursive locks because there was no other mechanism available to ensure that the object wasn't deleted whilst the instantiation was taking place. Unfortunately common objects (virObjectLockable) do not allow for recursive locks; however, since this is a very specific use case, let's roll our own version by using pthread_mutex_trylock and handle the return status based on what the API tells us. In order to do this, introduce a virMutexTryLock call and modify the virNWFilterObjListFindInstantiateFilter in order to consume and check the returned status. For starters let's only ever worry about the good status (e.g. we were the first to get the lock) and the pseudo recursive view of the world that our current thread was the thread that tried to get the lock when EBUSY is returned. An EBUSY is returned when either this thread or some thread has the lock. This involves keeping track of which thread was able to take out the first lock and if it's the same as the current thread, then allow the lock to succeed; otherwise, we'll need to retry as some other thread is currently holding the lock. Still we don't want that retry to last forever, so in order to avoid potential deadlock if two threads are doing a define at the same time, add retry lock that lasts for about 2 seconds which should be plenty of time.For any other errors, cause failure for the calling thread. Modify the callers to use a new virNWFilterObjEndInstAPI which handles the comparison of the number of try locks that were successful for the thread and only truly unlocks the object resource when the last lock is removed. Additionally to avoid the possibility that we do something untoward to ourselves and because we are not taking out the higher level lock, introduce an instLock to ensure that changes to the saved depth and tid are single threaded. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/virnwfilterobj.c | 156 ++++++++++++++++++++++++++++++++- src/conf/virnwfilterobj.h | 3 + src/libvirt_private.syms | 2 + src/nwfilter/nwfilter_gentech_driver.c | 65 ++++++++++---- src/util/virthread.c | 5 ++ src/util/virthread.h | 1 + 6 files changed, 211 insertions(+), 21 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 5d34851..d4fa98b 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -48,6 +48,12 @@ struct _virNWFilterObj { virNWFilterDefPtr def; virNWFilterDefPtr newDef; + + /* Instantiation locking */ + virMutex instLock; + int instDepth; + int instRetry; + pthread_t instTID; }; struct _virNWFilterObjList { @@ -88,7 +94,7 @@ virNWFilterObjNew(void) if (VIR_ALLOC(obj) < 0) return NULL; - if (virMutexInitRecursive(&obj->lock) < 0) { + if (virMutexInit(&obj->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize mutex")); VIR_FREE(obj); @@ -98,6 +104,12 @@ virNWFilterObjNew(void) virNWFilterObjLock(obj); virAtomicIntSet(&obj->refs, 1); + if (virMutexInit(&obj->instLock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot initialize instantiation mutex")); + virNWFilterObjEndAPI(&obj); + } + return obj; } @@ -114,6 +126,96 @@ virNWFilterObjEndAPI(virNWFilterObjPtr *obj) } +/** + * virNWFilterObjTryLock(obj) + * virNWFilterObjEndInstAPI(&obj) + * @obj: nwfilter object + * + * Rather than use recursive locks, nwfilter instantiation uses a + * modified version that will use NORMAL mutex locks except for the + * locking mechanism in virNWFilterObjListFindInstantiateFilter which + * uses virMutexTryLock processing to lock the object "recursively" + * for this thread only. + * + * In order to do this "safely", the processing of the TryLock and EndInstAPI + * will be protected by an "instLock" so that we can track which thread by + * pthread id has the lock and how many levels of depth have been taken when + * the lock is taken. + * + * This way when we go to release the lock (EndInstAPI) we can check our level + * and once the last lock has been released, call virNWFilterObjUnlock to + * release the lock. If we called it too soon, then we'd potentially release + * a lock we still need. + * + * Returns 0 on success and an errno value on failure + */ +static int +virNWFilterObjTryLock(virNWFilterObjPtr obj) +{ + int err; + pthread_t thisTID = pthread_self(); + + virMutexLock(&obj->instLock); + + do { + err = virMutexTryLock(&obj->lock); + if (err == 0) { + /* We are the first, then just like virMutexLock and we + * set our markers, instDepth = 1 and thisTID */ + obj->instDepth = 1; + obj->instRetry = 0; + obj->instTID = thisTID; + goto cleanup; + } else if (err == EBUSY) { + /* EBUSY indicates this thread or some other thread owns the lock + * If it's us, then excellent, similar to recursion. + * Else it's some other thread, let's retry for a bit until + * it is us. If we cannot get it, then avoid deadlock */ + if (obj->instTID == thisTID) { + obj->instDepth++; + obj->instRetry = 0; + err = 0; + goto cleanup; + } + if (++obj->instRetry > 20) + goto cleanup; + usleep(100 * 1000); /* Give the owner a chance */ + } else { + /* Don't handle other errors - return failure */ + goto cleanup; + } + } while (1); + + cleanup: + virMutexUnlock(&obj->instLock); + return err; +} + + +void +virNWFilterObjEndInstAPI(virNWFilterObjPtr *obj) +{ + if (!*obj) + return; + + virMutexLock(&(*obj)->instLock); + + /* Once the last locker has called this EndInstAPI function, then + * we can clear the instTID and really release the Lock; otherwise, + * we'll just decrement the Refcnt for the object and Unlock our + * instLock which protects this code from ourselves. */ + if (--(*obj)->instDepth == 0) { + (*obj)->instTID = 0; + virNWFilterObjUnlock(*obj); + } + + virNWFilterObjUnref(*obj); + virMutexUnlock(&(*obj)->instLock); + + *obj = NULL; +} + + virNWFilterDefPtr virNWFilterObjGetDef(virNWFilterObjPtr obj) { @@ -298,13 +400,30 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, } +/** + * To avoid the need to have recursive locks as a result of how the + * virNWFilterInstantiateFilter processing works, this API uses the + * virNWFilterObjTryLock processing in order to perform the pseudo + * recursive locking operation. + * + * NB: Use virNWFilterObjListFindByNameLocked since when called via + * virNWFilterObjListAssignDef path, the lock will already be held. + * For other callers, the single threaded driver level locking via + * virNWFilterWriteLockFilterUpdates ensure that the AssignDef, + * Undefine, and Reload will provide protection that the list cannot + * be adjusted whilst we're processing. Additionally the driver has + * a virNWFilterCallbackDriversLock which gets set to ensure other + * consumers of the NWFilter data cannot be called whilst the + * AssignDef, Undefine, or Reload occurs. + */ virNWFilterObjPtr virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, const char *filtername) { virNWFilterObjPtr obj; + int err; - if (!(obj = virNWFilterObjListFindByName(nwfilters, filtername))) { + if (!(obj = virNWFilterObjListFindByNameLocked(nwfilters, filtername))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("referenced filter '%s' is missing"), filtername); return NULL; @@ -313,7 +432,19 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); - virNWFilterObjEndAPI(&obj); + virNWFilterObjUnref(obj); + return NULL; + } + + if ((err = virNWFilterObjTryLock(obj)) != 0) { + virReportSystemError(err, + _("Unable to get mutex for '%s' depth=%d " + "tid=%llu mytid=%llu"), + filtername, obj->instDepth, + (unsigned long long)obj->instTID, + (unsigned long long)pthread_self()); + virNWFilterObjUnref(obj); + return NULL; } return obj; @@ -424,6 +555,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterObjPtr obj; virNWFilterDefPtr objdef; char uuidstr[VIR_UUID_STRING_BUFLEN]; + int rebuild_ret; virObjectLock(nwfilters); @@ -474,8 +606,24 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, } obj->newDef = def; + + /* Since we have the obj lock, update the instTID + * and instDepth because the Rebuild will trigger + * the driver to start calling FindInstantiateFilter, + * which uses the TryLock processing in order to + * acquire and release locks. */ + obj->instTID = pthread_self(); + obj->instDepth = 1; + /* trigger the update on VMs referencing the filter */ - if (virNWFilterTriggerVMFilterRebuild() < 0) { + rebuild_ret = virNWFilterTriggerVMFilterRebuild(); + + /* We're done, we still hold the original lock, let's reset + * the instTID and instDepth for the next consumer */ + obj->instTID = 0; + obj->instDepth = 0; + + if (rebuild_ret < 0) { obj->newDef = NULL; virNWFilterObjEndAPI(&obj); virObjectUnlock(nwfilters); diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index a78d8cd..9f738fe 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -44,6 +44,9 @@ struct _virNWFilterDriverState { void virNWFilterObjEndAPI(virNWFilterObjPtr *obj); +void +virNWFilterObjEndInstAPI(virNWFilterObjPtr *obj); + virNWFilterDefPtr virNWFilterObjGetDef(virNWFilterObjPtr obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 41531b5..3b9640d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -981,6 +981,7 @@ virNodeDeviceObjListRemove; # conf/virnwfilterobj.h virNWFilterObjEndAPI; +virNWFilterObjEndInstAPI; virNWFilterObjGetDef; virNWFilterObjGetNewDef; virNWFilterObjListAssignDef; @@ -2725,6 +2726,7 @@ virMutexDestroy; virMutexInit; virMutexInitRecursive; virMutexLock; +virMutexTryLock; virMutexUnlock; virOnce; virRWLockDestroy; diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 608cfbc..0f7f026 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -57,19 +57,50 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = { NULL }; -/* Serializes instantiation of filters. This is necessary - * to avoid lock ordering deadlocks. eg virNWFilterInstantiateFilterUpdate - * will hold a lock on a virNWFilterObjPtr. This in turn invokes - * virNWFilterDoInstantiate which invokes virNWFilterDetermineMissingVarsRec - * which invokes virNWFilterObjListFindInstantiateFilter. This iterates over - * every single virNWFilterObjPtr in the list. So if 2 threads try to - * instantiate a filter in parallel, they'll both hold 1 lock at the top level - * in virNWFilterInstantiateFilterUpdate which will cause the other thread - * to deadlock in virNWFilterObjListFindInstantiateFilter. +/* NB: Upon return from virNWFilterObjListFindInstantiateFilter the nwfilter + * object will be locked via an NWFilter only call to virObjectTryLock as a + * way to implement recursive locking, but without requiring the usage of + * recursive lock for the object. This mechanism allows the nwfilter object + * to use the common virLockableObject API's rather than having to have + * recursive mutexes and lock/ref API's for the majority of calls while + * leaving the very special case of instantiation to be handled via its + * own recursive methodolgy (all handled in the nwfilter object code). * - * XXX better long term solution is to make virNWFilterObjList use a - * hash table as is done for virDomainObjList. You can then get - * lockless lookup of objects by name. + * The virNWFilterObjListFindInstantiateFilter consumers are a complicated + * set of API's that are serialized via the updateMutex. For some consumers, + * a simple/shared read lock will suffice, while others will require the + * write lock. Serialization is also handled via a pair of driver mutexes + * virNWFilterWriteLockFilterUpdates and virNWFilterCallbackDriversLock. + * + * Processing of the instantiation code can be triggered via a direct + * nwfilterInstantiateFilter call or it may be triggered via a driver + * callback mechanism. The implementation is an interesting compilation of + * recursively called API's in order to handle the filter objects @def + * elements <filterref> and <rule> which define the ordering and usage + * of the filters. + * + * Since virNWFilterObjListFindInstantiateFilter provides a single entry + * reference point, it was modified to use the TryLock processing to + * check if the attempt to get the lock was being done by the same thread + * that originally obtained the lock. The corollary for the instantiation + * lock processing is usage of virNWFilterObjEndInstAPI when done with + * the object instead of virNWFilterObjEndAPI. The virNWFilterObjEndInstAPI + * must be used for the nwfilter lock obtained as a result of the call to + * virNWFilterObjListFindInstantiateFilter. + * + * XXX + * Some day perhaps the "matrix" of recursive callers and ways to get oneself + * very lost in this code will be "fixed" to be more orderly. For example, + * during virNWFilterDefToInst processing an object is fetched and placed + * into the inst->filters lookaside list during virNWFilterIncludeDefToRuleInst + * processing which then recursively calls virNWFilterDefToInst. The object + * is removed during virNWFilterInstReset, but in the mean time it's also + * possible to call virNWFilterDoInstantiate that has a completely separate + * path to calling virNWFilterObjListFindInstantiateFilter via the call to + * virNWFilterDetermineMissingVarsRec. This tangled web of interconnected + * callers also inludes virNWFilterInstantiateFilterUpdate as well. Each + * of these callers will use virNWFilterObjEndInstAPI in order to undo the + * lock and reference taken. */ static virMutex updateMutex; @@ -316,7 +347,7 @@ virNWFilterInstReset(virNWFilterInstPtr inst) size_t i; for (i = 0; i < inst->nfilters; i++) - virNWFilterObjEndAPI(&inst->filters[i]); + virNWFilterObjEndInstAPI(&inst->filters[i]); VIR_FREE(inst->filters); inst->nfilters = 0; @@ -426,7 +457,7 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, if (ret < 0) virNWFilterInstReset(inst); virNWFilterHashTableFree(tmpvars); - virNWFilterObjEndAPI(&obj); + virNWFilterObjEndInstAPI(&obj); return ret; } @@ -544,7 +575,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, /* create a temporary hashmap for depth-first tree traversal */ if (!(tmpvars = virNWFilterCreateVarsFrom(inc->params, vars))) { rc = -1; - virNWFilterObjEndAPI(&obj); + virNWFilterObjEndInstAPI(&obj); break; } @@ -568,7 +599,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, virNWFilterHashTableFree(tmpvars); - virNWFilterObjEndAPI(&obj); + virNWFilterObjEndInstAPI(&obj); if (rc < 0) break; } @@ -842,7 +873,7 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, virNWFilterHashTableFree(vars1); err_exit: - virNWFilterObjEndAPI(&obj); + virNWFilterObjEndInstAPI(&obj); VIR_FREE(str_ipaddr); VIR_FREE(str_macaddr); diff --git a/src/util/virthread.c b/src/util/virthread.c index 6c49515..1da6606 100644 --- a/src/util/virthread.c +++ b/src/util/virthread.c @@ -84,6 +84,11 @@ void virMutexDestroy(virMutexPtr m) pthread_mutex_destroy(&m->lock); } +int virMutexTryLock(virMutexPtr m) +{ + return pthread_mutex_trylock(&m->lock); +} + void virMutexLock(virMutexPtr m) { pthread_mutex_lock(&m->lock); diff --git a/src/util/virthread.h b/src/util/virthread.h index e466d9b..03c9fd6 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -132,6 +132,7 @@ int virMutexInitRecursive(virMutexPtr m) ATTRIBUTE_RETURN_CHECK; void virMutexDestroy(virMutexPtr m); void virMutexLock(virMutexPtr m); +int virMutexTryLock(virMutexPtr m); void virMutexUnlock(virMutexPtr m); -- 2.9.4