[libvirt] [PATCH v2 0/6] nwfilter adjustments for common object
v1: https://www.redhat.com/archives/libvir-list/2017-June/msg00079.html Changes since v1 (if I can recall all of them!): Patches 1, 4, 8-13 were pushed Patches 2, 3, 5-7 are dropped This this is a rework of patches 14-17 Patch 1 (former patch14): * Requested adjustments made to ACK'd patch, but since this and the remaining ones were related I didn't yet push it. Patch 2 (new): * From review though... As it turns out, virNWFilterDefEqual doesn't require the @cmpUUIDs patch. Patch 3 (fromer patch 15): * Fixed the line as requested. Patch was ACK'd by like Patch 1 I held onto it since it was related. Patch 4 (former patch 16): * Let's call it a complete rewrite. Rather than rely solely on the refcnt of the object, I've added/implemented a 'trylock' mechanism which essentially will allow the subsequent patch to use the virObjectLockable (e.g. a non recursive lock). Of course as I got further into the code - I think I've come to the conclusion that there isn't a way for a @def to disappear between threads with a refcnt only mechanism because there's a few other serialized locks which would need to be hurdled before hand. Still as I found out while running the Avocado test 'nwfilter_update_vm_running.update_arp_rule' the recursion would occur because the AssignDef code would call the Instantiation with the lock from the def being updated and that's where all the awful magic needs to occur. Additionally, I found that one wouldn't want to attempt to lock the nwfilters list inside the virNWFilterObjListFindInstantiateFilter because AssignDef already had that lock. I debated needing a recursive lock there until I came to the conclusion that the list couldn't change because the DefineXML is protected by a driver level lock (as is the Undefine and Reload paths). Patch 5 (former patch 17): * No changes, it was ACK'd, but without 1-4 is useless Patch 6 (NEW): * Remove the need for the driver level lock for a few API's since we have self locking nwfilters list. Also left comments in the 3 places where that lock remained to hopefully cause someone great anxiety if they decided to attempt to remove the lock without first consulting a specialist. NB: Ran all of the changes through the 'nwfilter' tests found in the Avocado test suite. John Ferlan (6): nwfilter: Add @refs logic to __virNWFilterObj nwfilter: Remove unnecessary UUID comparison bypass nwfilter: Convert _virNWFilterObjList to be a virObjectLockable nwfilter: Remove recursive locking for nwfilter instantiation nwfilter: Convert virNWFilterObj to use virObjectLockable nwfilter: Remove need for nwfilterDriverLock in some API's src/conf/virnwfilterobj.c | 635 ++++++++++++++++++++++++--------- src/conf/virnwfilterobj.h | 12 +- src/libvirt_private.syms | 6 +- src/nwfilter/nwfilter_driver.c | 66 ++-- src/nwfilter/nwfilter_gentech_driver.c | 66 +++- src/util/virobject.c | 24 ++ src/util/virobject.h | 4 + src/util/virthread.c | 5 + src/util/virthread.h | 1 + 9 files changed, 586 insertions(+), 233 deletions(-) -- 2.9.4
"Simple" conversion to the virObjectLockable object isn't quite possible yet because the nwfilter lock requires usage of recursive locking due to algorithms handing filter "<rule>"'s and "<filterref>"'s. The list search logic would also benefit from using hash tables for lookups. So this patch is step 1 in the process - add the @refs to _virNWFilterObj and modify the algorithms to use (a temporary) virNWFilterObj{Ref|Unref} in order to set things up for the list logic to utilize virObjectLockable hash tables. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 75 ++++++++++++++++++++++++++-------- src/conf/virnwfilterobj.h | 15 ++++--- src/libvirt_private.syms | 5 ++- src/nwfilter/nwfilter_driver.c | 15 +++---- src/nwfilter/nwfilter_gentech_driver.c | 11 +++-- 5 files changed, 83 insertions(+), 38 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 93072be..ecc7653 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -23,6 +23,7 @@ #include "datatypes.h" #include "viralloc.h" +#include "viratomic.h" #include "virerror.h" #include "virfile.h" #include "virlog.h" @@ -33,8 +34,15 @@ VIR_LOG_INIT("conf.virnwfilterobj"); +static void +virNWFilterObjLock(virNWFilterObjPtr obj); + +static void +virNWFilterObjUnlock(virNWFilterObjPtr obj); + struct _virNWFilterObj { virMutex lock; + int refs; bool wantRemoved; @@ -64,10 +72,24 @@ virNWFilterObjNew(void) } virNWFilterObjLock(obj); + virAtomicIntSet(&obj->refs, 1); + return obj; } +void +virNWFilterObjEndAPI(virNWFilterObjPtr *obj) +{ + if (!*obj) + return; + + virNWFilterObjUnlock(*obj); + virNWFilterObjUnref(*obj); + *obj = NULL; +} + + virNWFilterDefPtr virNWFilterObjGetDef(virNWFilterObjPtr obj) { @@ -104,12 +126,33 @@ virNWFilterObjFree(virNWFilterObjPtr obj) } +virNWFilterObjPtr +virNWFilterObjRef(virNWFilterObjPtr obj) +{ + if (obj) + virAtomicIntInc(&obj->refs); + return obj; +} + + +bool +virNWFilterObjUnref(virNWFilterObjPtr obj) +{ + bool lastRef; + if (!obj) + return false; + if ((lastRef = virAtomicIntDecAndTest(&obj->refs))) + virNWFilterObjFree(obj); + return !lastRef; +} + + void virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) { size_t i; for (i = 0; i < nwfilters->count; i++) - virNWFilterObjFree(nwfilters->objs[i]); + virNWFilterObjUnref(nwfilters->objs[i]); VIR_FREE(nwfilters->objs); VIR_FREE(nwfilters); } @@ -138,7 +181,7 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjLock(nwfilters->objs[i]); if (nwfilters->objs[i] == obj) { virNWFilterObjUnlock(nwfilters->objs[i]); - virNWFilterObjFree(nwfilters->objs[i]); + virNWFilterObjUnref(nwfilters->objs[i]); VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count); break; @@ -161,7 +204,7 @@ virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, virNWFilterObjLock(obj); def = obj->def; if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN)) - return obj; + return virNWFilterObjRef(obj); virNWFilterObjUnlock(obj); } @@ -182,7 +225,7 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, virNWFilterObjLock(obj); def = obj->def; if (STREQ_NULLABLE(def->name, name)) - return obj; + return virNWFilterObjRef(obj); virNWFilterObjUnlock(obj); } @@ -205,8 +248,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); - virNWFilterObjUnlock(obj); - return NULL; + virNWFilterObjEndAPI(&obj); } return obj; @@ -240,7 +282,7 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, if (obj) { rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def, filtername); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); if (rc < 0) break; } @@ -332,10 +374,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, _("filter with same UUID but different name " "('%s') already exists"), objdef->name); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); } else { if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -345,7 +387,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virReportError(VIR_ERR_OPERATION_FAILED, _("filter '%s' already exists with uuid %s"), def->name, uuidstr); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } } @@ -356,7 +398,6 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, return NULL; } - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { objdef = obj->def; @@ -370,7 +411,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild() < 0) { obj->newDef = NULL; - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } @@ -391,7 +432,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, } obj->def = def; - return obj; + return virNWFilterObjRef(obj); } @@ -561,8 +602,8 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, continue; obj = virNWFilterObjListLoadConfig(nwfilters, configDir, entry->d_name); - if (obj) - virNWFilterObjUnlock(obj); + + virNWFilterObjEndAPI(&obj); } VIR_DIR_CLOSE(dir); @@ -570,14 +611,14 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, } -void +static void virNWFilterObjLock(virNWFilterObjPtr obj) { virMutexLock(&obj->lock); } -void +static void virNWFilterObjUnlock(virNWFilterObjPtr obj) { virMutexUnlock(&obj->lock); diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 509e8dc..a78d8cd 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -41,6 +41,9 @@ struct _virNWFilterDriverState { bool watchingFirewallD; }; +void +virNWFilterObjEndAPI(virNWFilterObjPtr *obj); + virNWFilterDefPtr virNWFilterObjGetDef(virNWFilterObjPtr obj); @@ -50,6 +53,12 @@ virNWFilterObjGetNewDef(virNWFilterObjPtr obj); bool virNWFilterObjWantRemoved(virNWFilterObjPtr obj); +virNWFilterObjPtr +virNWFilterObjRef(virNWFilterObjPtr obj); + +bool +virNWFilterObjUnref(virNWFilterObjPtr obj); + virNWFilterObjListPtr virNWFilterObjListNew(void); @@ -105,10 +114,4 @@ int virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, const char *configDir); -void -virNWFilterObjLock(virNWFilterObjPtr obj); - -void -virNWFilterObjUnlock(virNWFilterObjPtr obj); - #endif /* VIRNWFILTEROBJ_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 187b12b..41531b5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -980,6 +980,7 @@ virNodeDeviceObjListRemove; # conf/virnwfilterobj.h +virNWFilterObjEndAPI; virNWFilterObjGetDef; virNWFilterObjGetNewDef; virNWFilterObjListAssignDef; @@ -993,9 +994,9 @@ virNWFilterObjListLoadAllConfigs; virNWFilterObjListNew; virNWFilterObjListNumOfNWFilters; virNWFilterObjListRemove; -virNWFilterObjLock; +virNWFilterObjRef; virNWFilterObjTestUnassignDef; -virNWFilterObjUnlock; +virNWFilterObjUnref; virNWFilterObjWantRemoved; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 2f9a51c..cb7b330 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -400,7 +400,7 @@ nwfilterLookupByUUID(virConnectPtr conn, nwfilter = virGetNWFilter(conn, def->name, def->uuid); cleanup: - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return nwfilter; } @@ -430,7 +430,7 @@ nwfilterLookupByName(virConnectPtr conn, nwfilter = virGetNWFilter(conn, def->name, def->uuid); cleanup: - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return nwfilter; } @@ -517,6 +517,8 @@ nwfilterDefineXML(virConnectPtr conn, if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) { virNWFilterObjListRemove(driver->nwfilters, obj); + virNWFilterObjUnref(obj); + obj = NULL; goto cleanup; } @@ -524,8 +526,7 @@ nwfilterDefineXML(virConnectPtr conn, cleanup: virNWFilterDefFree(def); - if (obj) - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); @@ -563,12 +564,12 @@ nwfilterUndefine(virNWFilterPtr nwfilter) goto cleanup; virNWFilterObjListRemove(driver->nwfilters, obj); + virNWFilterObjUnref(obj); obj = NULL; ret = 0; cleanup: - if (obj) - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); @@ -601,7 +602,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, ret = virNWFilterDefFormat(def); cleanup: - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return ret; } diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 6758200..608cfbc 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -316,7 +316,7 @@ virNWFilterInstReset(virNWFilterInstPtr inst) size_t i; for (i = 0; i < inst->nfilters; i++) - virNWFilterObjUnlock(inst->filters[i]); + virNWFilterObjEndAPI(&inst->filters[i]); VIR_FREE(inst->filters); inst->nfilters = 0; @@ -426,8 +426,7 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, if (ret < 0) virNWFilterInstReset(inst); virNWFilterHashTableFree(tmpvars); - if (obj) - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return ret; } @@ -545,7 +544,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, /* create a temporary hashmap for depth-first tree traversal */ if (!(tmpvars = virNWFilterCreateVarsFrom(inc->params, vars))) { rc = -1; - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); break; } @@ -569,7 +568,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, virNWFilterHashTableFree(tmpvars); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); if (rc < 0) break; } @@ -843,7 +842,7 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, virNWFilterHashTableFree(vars1); err_exit: - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); VIR_FREE(str_ipaddr); VIR_FREE(str_macaddr); -- 2.9.4
Commit id '46a811db07' added code to check if the filter by Name already existed with a different UUID, to go along with the existing found filter by UUID and compare the Names match thus making it impossible to reach this find by Name condition without both the Name and UUID already matching, so remove the need to "filter" out the UUID for the virNWFilterDefEqual. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index ecc7653..1e8fd36 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -329,18 +329,11 @@ virNWFilterObjTestUnassignDef(virNWFilterObjPtr obj) static bool virNWFilterDefEqual(const virNWFilterDef *def1, - virNWFilterDefPtr def2, - bool cmpUUIDs) + virNWFilterDefPtr def2) { bool ret = false; - unsigned char rem_uuid[VIR_UUID_BUFLEN]; - char *xml1, *xml2 = NULL; - - if (!cmpUUIDs) { - /* make sure the UUIDs are equal */ - memcpy(rem_uuid, def2->uuid, sizeof(rem_uuid)); - memcpy(def2->uuid, def1->uuid, sizeof(def2->uuid)); - } + char *xml1 = NULL; + char *xml2 = NULL; if (!(xml1 = virNWFilterDefFormat(def1)) || !(xml2 = virNWFilterDefFormat(def2))) @@ -349,9 +342,6 @@ virNWFilterDefEqual(const virNWFilterDef *def1, ret = STREQ(xml1, xml2); cleanup: - if (!cmpUUIDs) - memcpy(def2->uuid, rem_uuid, sizeof(rem_uuid)); - VIR_FREE(xml1); VIR_FREE(xml2); @@ -401,7 +391,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { objdef = obj->def; - if (virNWFilterDefEqual(def, objdef, false)) { + if (virNWFilterDefEqual(def, objdef)) { virNWFilterDefFree(objdef); obj->def = def; return obj; -- 2.9.4
Perhaps a bit out of order from the normal convert driver object to virObjectLockable, then convert the driver object list. However, as it turns out nwfilter objects have been using a recursive mutex for which the common virObject code does not want to use. So, if we convert the nwfilter object list to use virObjectLockable, then it will be more possible to make the necessary adjustments to the virNWFilterObjListFindInstantiateFilter algorithm in order to handle a recursive lock operation required as part of the <rule> and <filterref> (or "inc" list) processing. This patch takes the plunge, modifying the nwfilter object list handling code to utilize hash tables for both the UUID and Name for which an nwfilter def would utilize. This makes lookup by either "key" possible without needing to first lock the object in order to find a match as long as of course the object list itself is locked. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 404 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 287 insertions(+), 117 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 1e8fd36..5d34851 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -51,10 +51,34 @@ struct _virNWFilterObj { }; struct _virNWFilterObjList { - size_t count; - virNWFilterObjPtr *objs; + virObjectLockable parent; + + /* uuid string -> virNWFilterObj mapping + * for O(1), lockless lookup-by-uuid */ + virHashTable *objs; + + /* name -> virNWFilterObj mapping for O(1), + * lockless lookup-by-name */ + virHashTable *objsName; }; +static virClassPtr virNWFilterObjListClass; +static void virNWFilterObjListDispose(void *opaque); + +static int +virNWFilterObjOnceInit(void) +{ + if (!(virNWFilterObjListClass = virClassNew(virClassForObjectLockable(), + "virNWFilterObjList", + sizeof(virNWFilterObjList), + virNWFilterObjListDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virNWFilterObj) + static virNWFilterObjPtr virNWFilterObjNew(void) @@ -147,14 +171,30 @@ virNWFilterObjUnref(virNWFilterObjPtr obj) } +static void +virNWFilterObjListDispose(void *opaque) +{ + virNWFilterObjListPtr nwfilters = opaque; + + virHashFree(nwfilters->objs); + virHashFree(nwfilters->objsName); +} + + void virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) { - size_t i; - for (i = 0; i < nwfilters->count; i++) - virNWFilterObjUnref(nwfilters->objs[i]); - VIR_FREE(nwfilters->objs); - VIR_FREE(nwfilters); + virObjectUnref(nwfilters); +} + + +static void +virNWFilterObjListObjsFreeData(void *payload, + const void *name ATTRIBUTE_UNUSED) +{ + virNWFilterObjPtr obj = payload; + + virNWFilterObjUnref(obj); } @@ -163,8 +203,23 @@ virNWFilterObjListNew(void) { virNWFilterObjListPtr nwfilters; - if (VIR_ALLOC(nwfilters) < 0) + if (virNWFilterObjInitialize() < 0) + return NULL; + + if (!(nwfilters = virObjectLockableNew(virNWFilterObjListClass))) + return NULL; + + if (!(nwfilters->objs = virHashCreate(10, virNWFilterObjListObjsFreeData))) { + virObjectUnref(nwfilters); + return NULL; + } + + if (!(nwfilters->objsName = virHashCreate(10, virNWFilterObjListObjsFreeData))) { + virHashFree(nwfilters->objs); + virObjectUnref(nwfilters); return NULL; + } + return nwfilters; } @@ -173,21 +228,34 @@ void virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjPtr obj) { - size_t i; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virNWFilterDefPtr def; + if (!obj) + return; + def = obj->def; + + virUUIDFormat(def->uuid, uuidstr); + virNWFilterObjRef(obj); virNWFilterObjUnlock(obj); + virObjectLock(nwfilters); + virNWFilterObjLock(obj); + virHashRemoveEntry(nwfilters->objs, uuidstr); + virHashRemoveEntry(nwfilters->objsName, def->name); + virNWFilterObjUnlock(obj); + virNWFilterObjUnref(obj); + virObjectUnlock(nwfilters); +} - for (i = 0; i < nwfilters->count; i++) { - virNWFilterObjLock(nwfilters->objs[i]); - if (nwfilters->objs[i] == obj) { - virNWFilterObjUnlock(nwfilters->objs[i]); - virNWFilterObjUnref(nwfilters->objs[i]); - VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count); - break; - } - virNWFilterObjUnlock(nwfilters->objs[i]); - } +static virNWFilterObjPtr +virNWFilterObjListFindByUUIDLocked(virNWFilterObjListPtr nwfilters, + const unsigned char *uuid) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(uuid, uuidstr); + return virNWFilterObjRef(virHashLookup(nwfilters->objs, uuidstr)); } @@ -195,20 +263,22 @@ virNWFilterObjPtr virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, const unsigned char *uuid) { - size_t i; virNWFilterObjPtr obj; - virNWFilterDefPtr def; - for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; + virObjectLock(nwfilters); + obj = virNWFilterObjListFindByUUIDLocked(nwfilters, uuid); + virObjectUnlock(nwfilters); + if (obj) virNWFilterObjLock(obj); - def = obj->def; - if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN)) - return virNWFilterObjRef(obj); - virNWFilterObjUnlock(obj); - } + return obj; +} - return NULL; + +static virNWFilterObjPtr +virNWFilterObjListFindByNameLocked(virNWFilterObjListPtr nwfilters, + const char *name) +{ + return virNWFilterObjRef(virHashLookup(nwfilters->objsName, name)); } @@ -216,20 +286,15 @@ virNWFilterObjPtr virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, const char *name) { - size_t i; virNWFilterObjPtr obj; - virNWFilterDefPtr def; - for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; + virObjectLock(nwfilters); + obj = virNWFilterObjListFindByNameLocked(nwfilters, name); + virObjectUnlock(nwfilters); + if (obj) virNWFilterObjLock(obj); - def = obj->def; - if (STREQ_NULLABLE(def->name, name)) - return virNWFilterObjRef(obj); - virNWFilterObjUnlock(obj); - } - return NULL; + return obj; } @@ -277,9 +342,10 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, break; } - obj = virNWFilterObjListFindByName(nwfilters, - entry->include->filterref); + obj = virNWFilterObjListFindByNameLocked(nwfilters, + entry->include->filterref); if (obj) { + virObjectLock(obj); rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def, filtername); virNWFilterObjEndAPI(&obj); @@ -301,6 +367,8 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, * Detect a loop introduced through the filters being able to * reference each other. * + * NB: Enter with nwfilters locked + * * Returns 0 in case no loop was detected, -1 otherwise. */ static int @@ -355,8 +423,12 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, { virNWFilterObjPtr obj; virNWFilterDefPtr objdef; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virObjectLock(nwfilters); - if ((obj = virNWFilterObjListFindByUUID(nwfilters, def->uuid))) { + if ((obj = virNWFilterObjListFindByUUIDLocked(nwfilters, def->uuid))) { + virNWFilterObjLock(obj); objdef = obj->def; if (STRNEQ(def->name, objdef->name)) { @@ -365,19 +437,21 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, "('%s') already exists"), objdef->name); virNWFilterObjEndAPI(&obj); + virObjectUnlock(nwfilters); return NULL; } virNWFilterObjEndAPI(&obj); } else { - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) { + virNWFilterObjLock(obj); objdef = obj->def; virUUIDFormat(objdef->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("filter '%s' already exists with uuid %s"), def->name, uuidstr); virNWFilterObjEndAPI(&obj); + virObjectUnlock(nwfilters); return NULL; } } @@ -385,16 +459,18 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (virNWFilterObjListDefLoopDetect(nwfilters, def) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("filter would introduce a loop")); + virObjectUnlock(nwfilters); return NULL; } - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) { + virNWFilterObjLock(obj); objdef = obj->def; if (virNWFilterDefEqual(def, objdef)) { virNWFilterDefFree(objdef); obj->def = def; - return obj; + goto cleanup; } obj->newDef = def; @@ -402,27 +478,63 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (virNWFilterTriggerVMFilterRebuild() < 0) { obj->newDef = NULL; virNWFilterObjEndAPI(&obj); + virObjectUnlock(nwfilters); return NULL; } virNWFilterDefFree(objdef); obj->def = def; obj->newDef = NULL; - return obj; + goto cleanup; } if (!(obj = virNWFilterObjNew())) return NULL; - if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, - nwfilters->count, obj) < 0) { - virNWFilterObjUnlock(obj); - virNWFilterObjFree(obj); - return NULL; + virUUIDFormat(def->uuid, uuidstr); + if (virHashAddEntry(nwfilters->objs, uuidstr, obj) < 0) + goto error; + virNWFilterObjRef(obj); + + if (virHashAddEntry(nwfilters->objsName, def->name, obj) < 0) { + virHashRemoveEntry(nwfilters->objs, uuidstr); + goto error; } + obj->def = def; + virNWFilterObjRef(obj); + + cleanup: + virObjectUnlock(nwfilters); + return obj; - return virNWFilterObjRef(obj); + error: + virNWFilterObjUnlock(obj); + virNWFilterObjUnref(obj); + virObjectUnlock(nwfilters); + return NULL; +} + + +struct virNWFilterCountData { + virConnectPtr conn; + virNWFilterObjListFilter aclfilter; + int nelems; +}; + +static int +virNWFilterObjListNumOfNWFiltersCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNWFilterObjPtr obj = payload; + struct virNWFilterCountData *data = opaque; + + virObjectLock(obj); + if (!data->aclfilter || data->aclfilter(data->conn, obj->def)) + data->nelems++; + virObjectUnlock(obj); + return 0; } @@ -431,18 +543,56 @@ virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters, virConnectPtr conn, virNWFilterObjListFilter aclfilter) { - size_t i; - int nfilters = 0; + struct virNWFilterCountData data = { .conn = conn, + .aclfilter = aclfilter, .nelems = 0 }; - for (i = 0; i < nwfilters->count; i++) { - virNWFilterObjPtr obj = nwfilters->objs[i]; - virNWFilterObjLock(obj); - if (!aclfilter || aclfilter(conn, obj->def)) - nfilters++; - virNWFilterObjUnlock(obj); + virObjectLock(nwfilters); + virHashForEach(nwfilters->objs, virNWFilterObjListNumOfNWFiltersCallback, + &data); + virObjectUnlock(nwfilters); + + return data.nelems; +} + + +struct virNWFilterListData { + virConnectPtr conn; + virNWFilterObjListFilter aclfilter; + int nelems; + char **elems; + int maxelems; + bool error; +}; + +static int +virNWFilterObjListGetNamesCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNWFilterObjPtr obj = payload; + virNWFilterDefPtr def; + struct virNWFilterListData *data = opaque; + + if (data->error) + return 0; + + if (data->maxelems >= 0 && data->nelems == data->maxelems) + return 0; + + virObjectLock(obj); + def = obj->def; + + if (!data->aclfilter || data->aclfilter(data->conn, def)) { + if (VIR_STRDUP(data->elems[data->nelems], def->name) < 0) { + data->error = true; + goto cleanup; + } + data->nelems++; } - return nfilters; + cleanup: + virObjectUnlock(obj); + return 0; } @@ -453,82 +603,102 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr nwfilters, char **const names, int maxnames) { - int nnames = 0; - size_t i; - virNWFilterDefPtr def; + struct virNWFilterListData data = { .conn = conn, .aclfilter = aclfilter, + .nelems = 0, .elems = names, .maxelems = maxnames, .error = false }; - for (i = 0; i < nwfilters->count && nnames < maxnames; i++) { - virNWFilterObjPtr obj = nwfilters->objs[i]; - virNWFilterObjLock(obj); - def = obj->def; - if (!aclfilter || aclfilter(conn, def)) { - if (VIR_STRDUP(names[nnames], def->name) < 0) { - virNWFilterObjUnlock(obj); - goto failure; - } - nnames++; - } - virNWFilterObjUnlock(obj); - } + virObjectLock(nwfilters); + virHashForEach(nwfilters->objs, virNWFilterObjListGetNamesCallback, &data); + virObjectUnlock(nwfilters); - return nnames; + if (data.error) + goto error; - failure: - while (--nnames >= 0) - VIR_FREE(names[nnames]); + return data.nelems; + error: + while (--data.nelems >= 0) + VIR_FREE(data.elems[data.nelems]); return -1; } -int -virNWFilterObjListExport(virConnectPtr conn, - virNWFilterObjListPtr nwfilters, - virNWFilterPtr **filters, - virNWFilterObjListFilter aclfilter) +struct virNWFilterExportData { + virConnectPtr conn; + virNWFilterObjListFilter aclfilter; + virNWFilterPtr *filters; + int nfilters; + bool error; +}; + +static int +virNWFilterObjListExportCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) { - virNWFilterPtr *tmp_filters = NULL; - int nfilters = 0; - virNWFilterPtr filter = NULL; - virNWFilterObjPtr obj = NULL; + virNWFilterObjPtr obj = payload; virNWFilterDefPtr def; - size_t i; - int ret = -1; + struct virNWFilterExportData *data = opaque; + virNWFilterPtr nwfilter; + + if (data->error) + return 0; + + virObjectLock(obj); + def = obj->def; - if (!filters) { - ret = nwfilters->count; + if (data->aclfilter && !data->aclfilter(data->conn, def)) + goto cleanup; + + if (!data->filters) { + data->nfilters++; goto cleanup; } - if (VIR_ALLOC_N(tmp_filters, nwfilters->count + 1) < 0) + if (!(nwfilter = virGetNWFilter(data->conn, def->name, def->uuid))) { + data->error = true; goto cleanup; + } + data->filters[data->nfilters++] = nwfilter; - for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; - virNWFilterObjLock(obj); - def = obj->def; - if (!aclfilter || aclfilter(conn, def)) { - if (!(filter = virGetNWFilter(conn, def->name, def->uuid))) { - virNWFilterObjUnlock(obj); - goto cleanup; - } - tmp_filters[nfilters++] = filter; - } - virNWFilterObjUnlock(obj); + cleanup: + virObjectUnlock(obj); + return 0; +} + + +int +virNWFilterObjListExport(virConnectPtr conn, + virNWFilterObjListPtr nwfilters, + virNWFilterPtr **filters, + virNWFilterObjListFilter aclfilter) +{ + struct virNWFilterExportData data = { .conn = conn, .aclfilter = aclfilter, + .filters = NULL, .nfilters = 0, .error = false }; + + virObjectLock(nwfilters); + if (filters && + VIR_ALLOC_N(data.filters, virHashSize(nwfilters->objs) + 1) < 0) { + virObjectUnlock(nwfilters); + return -1; } - *filters = tmp_filters; - tmp_filters = NULL; - ret = nfilters; + virHashForEach(nwfilters->objs, virNWFilterObjListExportCallback, &data); + virObjectUnlock(nwfilters); - cleanup: - if (tmp_filters) { - for (i = 0; i < nfilters; i ++) - virObjectUnref(tmp_filters[i]); + if (data.error) + goto cleanup; + + if (data.filters) { + /* trim the array to the final size */ + ignore_value(VIR_REALLOC_N(data.filters, data.nfilters + 1)); + *filters = data.filters; } - VIR_FREE(tmp_filters); - return ret; + return data.nfilters; + + cleanup: + virObjectListFree(data.filters); + return -1; } -- 2.9.4
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@redhat.com> --- 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
Now that we have a bit more control, let's convert our object into a lockable object and let that magic handle the create, lock/unlock, and reference counting. Because we have the need for instantiation in a recursive manner, introduce the virObjectTryLock to handle the virNWFilterObjTryLock processing. It'll just be the shim into virMutexTryLock, but adds an extra error value EFAULT for when the incoming @obj is not determined to be a LockableObject. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 132 +++++++++++++---------------------------- src/conf/virnwfilterobj.h | 6 -- src/libvirt_private.syms | 3 +- src/nwfilter/nwfilter_driver.c | 4 +- src/util/virobject.c | 24 ++++++++ src/util/virobject.h | 4 ++ 6 files changed, 71 insertions(+), 102 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index d4fa98b..4792f9a 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -23,7 +23,6 @@ #include "datatypes.h" #include "viralloc.h" -#include "viratomic.h" #include "virerror.h" #include "virfile.h" #include "virlog.h" @@ -34,15 +33,8 @@ VIR_LOG_INIT("conf.virnwfilterobj"); -static void -virNWFilterObjLock(virNWFilterObjPtr obj); - -static void -virNWFilterObjUnlock(virNWFilterObjPtr obj); - struct _virNWFilterObj { - virMutex lock; - int refs; + virObjectLockable parent; bool wantRemoved; @@ -68,12 +60,20 @@ struct _virNWFilterObjList { virHashTable *objsName; }; +static virClassPtr virNWFilterObjClass; static virClassPtr virNWFilterObjListClass; +static void virNWFilterObjDispose(void *opaque); static void virNWFilterObjListDispose(void *opaque); static int virNWFilterObjOnceInit(void) { + if (!(virNWFilterObjClass = virClassNew(virClassForObjectLockable(), + "virNWFilterObj", + sizeof(virNWFilterObj), + virNWFilterObjDispose))) + return -1; + if (!(virNWFilterObjListClass = virClassNew(virClassForObjectLockable(), "virNWFilterObjList", sizeof(virNWFilterObjList), @@ -91,18 +91,13 @@ virNWFilterObjNew(void) { virNWFilterObjPtr obj; - if (VIR_ALLOC(obj) < 0) + if (virNWFilterObjInitialize() < 0) return NULL; - if (virMutexInit(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(obj); + if (!(obj = virObjectLockableNew(virNWFilterObjClass))) return NULL; - } - virNWFilterObjLock(obj); - virAtomicIntSet(&obj->refs, 1); + virObjectLock(obj); if (virMutexInit(&obj->instLock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -120,8 +115,8 @@ virNWFilterObjEndAPI(virNWFilterObjPtr *obj) if (!*obj) return; - virNWFilterObjUnlock(*obj); - virNWFilterObjUnref(*obj); + virObjectUnlock(*obj); + virObjectUnref(*obj); *obj = NULL; } @@ -158,7 +153,7 @@ virNWFilterObjTryLock(virNWFilterObjPtr obj) virMutexLock(&obj->instLock); do { - err = virMutexTryLock(&obj->lock); + err = virObjectTryLock(obj); if (err == 0) { /* We are the first, then just like virMutexLock and we * set our markers, instDepth = 1 and thisTID */ @@ -206,10 +201,10 @@ virNWFilterObjEndInstAPI(virNWFilterObjPtr *obj) * instLock which protects this code from ourselves. */ if (--(*obj)->instDepth == 0) { (*obj)->instTID = 0; - virNWFilterObjUnlock(*obj); + virObjectUnlock(*obj); } - virNWFilterObjUnref(*obj); + virObjectUnref(*obj); virMutexUnlock(&(*obj)->instLock); *obj = NULL; @@ -238,38 +233,15 @@ virNWFilterObjWantRemoved(virNWFilterObjPtr obj) static void -virNWFilterObjFree(virNWFilterObjPtr obj) +virNWFilterObjDispose(void *opaque) { + virNWFilterObjPtr obj = opaque; + if (!obj) return; virNWFilterDefFree(obj->def); virNWFilterDefFree(obj->newDef); - - virMutexDestroy(&obj->lock); - - VIR_FREE(obj); -} - - -virNWFilterObjPtr -virNWFilterObjRef(virNWFilterObjPtr obj) -{ - if (obj) - virAtomicIntInc(&obj->refs); - return obj; -} - - -bool -virNWFilterObjUnref(virNWFilterObjPtr obj) -{ - bool lastRef; - if (!obj) - return false; - if ((lastRef = virAtomicIntDecAndTest(&obj->refs))) - virNWFilterObjFree(obj); - return !lastRef; } @@ -290,16 +262,6 @@ virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) } -static void -virNWFilterObjListObjsFreeData(void *payload, - const void *name ATTRIBUTE_UNUSED) -{ - virNWFilterObjPtr obj = payload; - - virNWFilterObjUnref(obj); -} - - virNWFilterObjListPtr virNWFilterObjListNew(void) { @@ -311,12 +273,12 @@ virNWFilterObjListNew(void) if (!(nwfilters = virObjectLockableNew(virNWFilterObjListClass))) return NULL; - if (!(nwfilters->objs = virHashCreate(10, virNWFilterObjListObjsFreeData))) { + if (!(nwfilters->objs = virHashCreate(10, virObjectFreeHashData))) { virObjectUnref(nwfilters); return NULL; } - if (!(nwfilters->objsName = virHashCreate(10, virNWFilterObjListObjsFreeData))) { + if (!(nwfilters->objsName = virHashCreate(10, virObjectFreeHashData))) { virHashFree(nwfilters->objs); virObjectUnref(nwfilters); return NULL; @@ -338,14 +300,14 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, def = obj->def; virUUIDFormat(def->uuid, uuidstr); - virNWFilterObjRef(obj); - virNWFilterObjUnlock(obj); + virObjectRef(obj); + virObjectUnlock(obj); virObjectLock(nwfilters); - virNWFilterObjLock(obj); + virObjectLock(obj); virHashRemoveEntry(nwfilters->objs, uuidstr); virHashRemoveEntry(nwfilters->objsName, def->name); - virNWFilterObjUnlock(obj); - virNWFilterObjUnref(obj); + virObjectUnlock(obj); + virObjectUnref(obj); virObjectUnlock(nwfilters); } @@ -357,7 +319,7 @@ virNWFilterObjListFindByUUIDLocked(virNWFilterObjListPtr nwfilters, char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(uuid, uuidstr); - return virNWFilterObjRef(virHashLookup(nwfilters->objs, uuidstr)); + return virObjectRef(virHashLookup(nwfilters->objs, uuidstr)); } @@ -371,7 +333,7 @@ virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, obj = virNWFilterObjListFindByUUIDLocked(nwfilters, uuid); virObjectUnlock(nwfilters); if (obj) - virNWFilterObjLock(obj); + virObjectLock(obj); return obj; } @@ -380,7 +342,7 @@ static virNWFilterObjPtr virNWFilterObjListFindByNameLocked(virNWFilterObjListPtr nwfilters, const char *name) { - return virNWFilterObjRef(virHashLookup(nwfilters->objsName, name)); + return virObjectRef(virHashLookup(nwfilters->objsName, name)); } @@ -394,7 +356,7 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, obj = virNWFilterObjListFindByNameLocked(nwfilters, name); virObjectUnlock(nwfilters); if (obj) - virNWFilterObjLock(obj); + virObjectLock(obj); return obj; } @@ -432,7 +394,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); - virNWFilterObjUnref(obj); + virObjectUnref(obj); return NULL; } @@ -443,7 +405,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, filtername, obj->instDepth, (unsigned long long)obj->instTID, (unsigned long long)pthread_self()); - virNWFilterObjUnref(obj); + virObjectUnref(obj); return NULL; } @@ -560,7 +522,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virObjectLock(nwfilters); if ((obj = virNWFilterObjListFindByUUIDLocked(nwfilters, def->uuid))) { - virNWFilterObjLock(obj); + virObjectLock(obj); objdef = obj->def; if (STRNEQ(def->name, objdef->name)) { @@ -576,7 +538,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, } else { if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) { - virNWFilterObjLock(obj); + virObjectLock(obj); objdef = obj->def; virUUIDFormat(objdef->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, @@ -597,7 +559,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) { - virNWFilterObjLock(obj); + virObjectLock(obj); objdef = obj->def; if (virNWFilterDefEqual(def, objdef)) { virNWFilterDefFree(objdef); @@ -642,7 +604,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virUUIDFormat(def->uuid, uuidstr); if (virHashAddEntry(nwfilters->objs, uuidstr, obj) < 0) goto error; - virNWFilterObjRef(obj); + virObjectRef(obj); if (virHashAddEntry(nwfilters->objsName, def->name, obj) < 0) { virHashRemoveEntry(nwfilters->objs, uuidstr); @@ -650,15 +612,15 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, } obj->def = def; - virNWFilterObjRef(obj); + virObjectRef(obj); cleanup: virObjectUnlock(nwfilters); return obj; error: - virNWFilterObjUnlock(obj); - virNWFilterObjUnref(obj); + virObjectUnlock(obj); + virObjectUnref(obj); virObjectUnlock(nwfilters); return NULL; } @@ -917,17 +879,3 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, VIR_DIR_CLOSE(dir); return ret; } - - -static void -virNWFilterObjLock(virNWFilterObjPtr obj) -{ - virMutexLock(&obj->lock); -} - - -static void -virNWFilterObjUnlock(virNWFilterObjPtr obj) -{ - virMutexUnlock(&obj->lock); -} diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 9f738fe..73bb9b9 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -56,12 +56,6 @@ virNWFilterObjGetNewDef(virNWFilterObjPtr obj); bool virNWFilterObjWantRemoved(virNWFilterObjPtr obj); -virNWFilterObjPtr -virNWFilterObjRef(virNWFilterObjPtr obj); - -bool -virNWFilterObjUnref(virNWFilterObjPtr obj); - virNWFilterObjListPtr virNWFilterObjListNew(void); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3b9640d..c10960d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -995,9 +995,7 @@ virNWFilterObjListLoadAllConfigs; virNWFilterObjListNew; virNWFilterObjListNumOfNWFilters; virNWFilterObjListRemove; -virNWFilterObjRef; virNWFilterObjTestUnassignDef; -virNWFilterObjUnref; virNWFilterObjWantRemoved; @@ -2296,6 +2294,7 @@ virObjectLock; virObjectLockableNew; virObjectNew; virObjectRef; +virObjectTryLock; virObjectUnlock; virObjectUnref; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index cb7b330..a83f5cf 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -517,7 +517,7 @@ nwfilterDefineXML(virConnectPtr conn, if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) { virNWFilterObjListRemove(driver->nwfilters, obj); - virNWFilterObjUnref(obj); + virObjectUnref(obj); obj = NULL; goto cleanup; } @@ -564,7 +564,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter) goto cleanup; virNWFilterObjListRemove(driver->nwfilters, obj); - virNWFilterObjUnref(obj); + virObjectUnref(obj); obj = NULL; ret = 0; diff --git a/src/util/virobject.c b/src/util/virobject.c index 34805d3..3ced1b4 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -350,6 +350,30 @@ virObjectLock(void *anyobj) /** + * virObjectTryLock: + * @anyobj: any instance of virObjectLockablePtr + * + * Acquire a lock on @anyobj. The lock must be + * released by virObjectUnlock. + * + * The caller is expected to have acquired a reference + * on the object before locking it (eg virObjectRef). + * The object must be unlocked before releasing this + * reference. + */ +int +virObjectTryLock(void *anyobj) +{ + virObjectLockablePtr obj = virObjectGetLockableObj(anyobj); + + if (!obj) + return EFAULT; + + return virMutexTryLock(&obj->lock); +} + + +/** * virObjectUnlock: * @anyobj: any instance of virObjectLockablePtr * diff --git a/src/util/virobject.h b/src/util/virobject.h index f4c292b..452b6b2 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -112,6 +112,10 @@ void virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); +int +virObjectTryLock(void *lockableobj) + ATTRIBUTE_NONNULL(1); + void virObjectUnlock(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.9.4
Now that nwfilters object list is self locking, it's no longer necessary to hold the driver level lock for certain API's. However, for the DefineXML, Undefine, and Reload processing keeping that lock ensures for serialization required in order to process the filter Instantiation properly. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_driver.c | 51 +++++++++++++++++------------------------- 1 file changed, 20 insertions(+), 31 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index a83f5cf..fba2c79 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -295,6 +295,10 @@ nwfilterStateReload(void) /* shut down all threads -- they will be restarted if necessary */ virNWFilterLearnThreadsTerminate(true); + /* Serialization of virNWFilterObjListLoadAllConfigs is extremely + * important as it relates to virNWFilterObjListFindInstantiateFilter + * processing via virNWFilterTriggerVMFilterRebuild that occurs during + * virNWFilterObjListAssignDef */ nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); @@ -386,11 +390,7 @@ nwfilterLookupByUUID(virConnectPtr conn, virNWFilterDefPtr def; virNWFilterPtr nwfilter = NULL; - nwfilterDriverLock(); - obj = nwfilterObjFromNWFilter(uuid); - nwfilterDriverUnlock(); - - if (!obj) + if (!(obj = nwfilterObjFromNWFilter(uuid))) return NULL; def = virNWFilterObjGetDef(obj); @@ -413,11 +413,7 @@ nwfilterLookupByName(virConnectPtr conn, virNWFilterDefPtr def; virNWFilterPtr nwfilter = NULL; - nwfilterDriverLock(); - obj = virNWFilterObjListFindByName(driver->nwfilters, name); - nwfilterDriverUnlock(); - - if (!obj) { + if (!(obj = virNWFilterObjListFindByName(driver->nwfilters, name))) { virReportError(VIR_ERR_NO_NWFILTER, _("no nwfilter with matching name '%s'"), name); return NULL; @@ -451,17 +447,12 @@ nwfilterConnectListNWFilters(virConnectPtr conn, char **const names, int maxnames) { - int nnames; - if (virConnectListNWFiltersEnsureACL(conn) < 0) return -1; - nwfilterDriverLock(); - nnames = virNWFilterObjListGetNames(driver->nwfilters, conn, - virConnectListNWFiltersCheckACL, - names, maxnames); - nwfilterDriverUnlock(); - return nnames; + return virNWFilterObjListGetNames(driver->nwfilters, conn, + virConnectListNWFiltersCheckACL, + names, maxnames); } @@ -470,19 +461,13 @@ nwfilterConnectListAllNWFilters(virConnectPtr conn, virNWFilterPtr **nwfilters, unsigned int flags) { - int ret; - virCheckFlags(0, -1); if (virConnectListAllNWFiltersEnsureACL(conn) < 0) return -1; - nwfilterDriverLock(); - ret = virNWFilterObjListExport(conn, driver->nwfilters, nwfilters, - virConnectListAllNWFiltersCheckACL); - nwfilterDriverUnlock(); - - return ret; + return virNWFilterObjListExport(conn, driver->nwfilters, nwfilters, + virConnectListAllNWFiltersCheckACL); } static virNWFilterPtr @@ -500,6 +485,10 @@ nwfilterDefineXML(virConnectPtr conn, return NULL; } + /* Serialization of *one* DefineXML consumer is extremely important + * as it relates to virNWFilterObjListFindInstantiateFilter processing + * via virNWFilterTriggerVMFilterRebuild that occurs during + * virNWFilterObjListAssignDef */ nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); @@ -542,6 +531,10 @@ nwfilterUndefine(virNWFilterPtr nwfilter) virNWFilterDefPtr def; int ret = -1; + /* Serialization of *one* Undefine consumer is extremely important + * as it relates to virNWFilterObjListFindInstantiateFilter processing + * via virNWFilterTriggerVMFilterRebuild that occurs during + * virNWFilterObjTestUnassignDef */ nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); @@ -588,11 +581,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, virCheckFlags(0, NULL); - nwfilterDriverLock(); - obj = nwfilterObjFromNWFilter(nwfilter->uuid); - nwfilterDriverUnlock(); - - if (!obj) + if (!(obj = nwfilterObjFromNWFilter(nwfilter->uuid))) return NULL; def = virNWFilterObjGetDef(obj); -- 2.9.4
ping - perhaps at least the first 3... I'm now beginning to think/wonder if using the rwlock_rdlock would be the solution at least for nwfilter objs. It seems from a quick scan of the man page that they are designed to be recursive as long as the consumer guarantees that there is an Unlock for every LockRead. A lot better than rolling my own recursive lock algorithm that I tried in patch 4. Would require some other adjustments (and concessions) along the way, but seemingly possible. Tks - John On 07/18/2017 04:57 PM, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2017-June/msg00079.html
Changes since v1 (if I can recall all of them!):
Patches 1, 4, 8-13 were pushed Patches 2, 3, 5-7 are dropped
This this is a rework of patches 14-17
Patch 1 (former patch14): * Requested adjustments made to ACK'd patch, but since this and the remaining ones were related I didn't yet push it.
Patch 2 (new): * From review though... As it turns out, virNWFilterDefEqual doesn't require the @cmpUUIDs patch.
Patch 3 (fromer patch 15): * Fixed the line as requested. Patch was ACK'd by like Patch 1 I held onto it since it was related.
Patch 4 (former patch 16): * Let's call it a complete rewrite. Rather than rely solely on the refcnt of the object, I've added/implemented a 'trylock' mechanism which essentially will allow the subsequent patch to use the virObjectLockable (e.g. a non recursive lock). Of course as I got further into the code - I think I've come to the conclusion that there isn't a way for a @def to disappear between threads with a refcnt only mechanism because there's a few other serialized locks which would need to be hurdled before hand. Still as I found out while running the Avocado test 'nwfilter_update_vm_running.update_arp_rule' the recursion would occur because the AssignDef code would call the Instantiation with the lock from the def being updated and that's where all the awful magic needs to occur. Additionally, I found that one wouldn't want to attempt to lock the nwfilters list inside the virNWFilterObjListFindInstantiateFilter because AssignDef already had that lock. I debated needing a recursive lock there until I came to the conclusion that the list couldn't change because the DefineXML is protected by a driver level lock (as is the Undefine and Reload paths).
Patch 5 (former patch 17): * No changes, it was ACK'd, but without 1-4 is useless
Patch 6 (NEW): * Remove the need for the driver level lock for a few API's since we have self locking nwfilters list. Also left comments in the 3 places where that lock remained to hopefully cause someone great anxiety if they decided to attempt to remove the lock without first consulting a specialist.
NB: Ran all of the changes through the 'nwfilter' tests found in the Avocado test suite.
John Ferlan (6): nwfilter: Add @refs logic to __virNWFilterObj nwfilter: Remove unnecessary UUID comparison bypass nwfilter: Convert _virNWFilterObjList to be a virObjectLockable nwfilter: Remove recursive locking for nwfilter instantiation nwfilter: Convert virNWFilterObj to use virObjectLockable nwfilter: Remove need for nwfilterDriverLock in some API's
src/conf/virnwfilterobj.c | 635 ++++++++++++++++++++++++--------- src/conf/virnwfilterobj.h | 12 +- src/libvirt_private.syms | 6 +- src/nwfilter/nwfilter_driver.c | 66 ++-- src/nwfilter/nwfilter_gentech_driver.c | 66 +++- src/util/virobject.c | 24 ++ src/util/virobject.h | 4 + src/util/virthread.c | 5 + src/util/virthread.h | 1 + 9 files changed, 586 insertions(+), 233 deletions(-)
ping^^2 Again, don't bother with patches 4-6. Tks, John On 08/02/2017 07:27 AM, John Ferlan wrote:
ping -
perhaps at least the first 3...
I'm now beginning to think/wonder if using the rwlock_rdlock would be the solution at least for nwfilter objs. It seems from a quick scan of the man page that they are designed to be recursive as long as the consumer guarantees that there is an Unlock for every LockRead. A lot better than rolling my own recursive lock algorithm that I tried in patch 4. Would require some other adjustments (and concessions) along the way, but seemingly possible.
Tks -
John
On 07/18/2017 04:57 PM, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2017-June/msg00079.html
Changes since v1 (if I can recall all of them!):
Patches 1, 4, 8-13 were pushed Patches 2, 3, 5-7 are dropped
This this is a rework of patches 14-17
Patch 1 (former patch14): * Requested adjustments made to ACK'd patch, but since this and the remaining ones were related I didn't yet push it.
Patch 2 (new): * From review though... As it turns out, virNWFilterDefEqual doesn't require the @cmpUUIDs patch.
Patch 3 (fromer patch 15): * Fixed the line as requested. Patch was ACK'd by like Patch 1 I held onto it since it was related.
Patch 4 (former patch 16): * Let's call it a complete rewrite. Rather than rely solely on the refcnt of the object, I've added/implemented a 'trylock' mechanism which essentially will allow the subsequent patch to use the virObjectLockable (e.g. a non recursive lock). Of course as I got further into the code - I think I've come to the conclusion that there isn't a way for a @def to disappear between threads with a refcnt only mechanism because there's a few other serialized locks which would need to be hurdled before hand. Still as I found out while running the Avocado test 'nwfilter_update_vm_running.update_arp_rule' the recursion would occur because the AssignDef code would call the Instantiation with the lock from the def being updated and that's where all the awful magic needs to occur. Additionally, I found that one wouldn't want to attempt to lock the nwfilters list inside the virNWFilterObjListFindInstantiateFilter because AssignDef already had that lock. I debated needing a recursive lock there until I came to the conclusion that the list couldn't change because the DefineXML is protected by a driver level lock (as is the Undefine and Reload paths).
Patch 5 (former patch 17): * No changes, it was ACK'd, but without 1-4 is useless
Patch 6 (NEW): * Remove the need for the driver level lock for a few API's since we have self locking nwfilters list. Also left comments in the 3 places where that lock remained to hopefully cause someone great anxiety if they decided to attempt to remove the lock without first consulting a specialist.
NB: Ran all of the changes through the 'nwfilter' tests found in the Avocado test suite.
John Ferlan (6): nwfilter: Add @refs logic to __virNWFilterObj nwfilter: Remove unnecessary UUID comparison bypass nwfilter: Convert _virNWFilterObjList to be a virObjectLockable nwfilter: Remove recursive locking for nwfilter instantiation nwfilter: Convert virNWFilterObj to use virObjectLockable nwfilter: Remove need for nwfilterDriverLock in some API's
src/conf/virnwfilterobj.c | 635 ++++++++++++++++++++++++--------- src/conf/virnwfilterobj.h | 12 +- src/libvirt_private.syms | 6 +- src/nwfilter/nwfilter_driver.c | 66 ++-- src/nwfilter/nwfilter_gentech_driver.c | 66 +++- src/util/virobject.c | 24 ++ src/util/virobject.h | 4 + src/util/virthread.c | 5 + src/util/virthread.h | 1 + 9 files changed, 586 insertions(+), 233 deletions(-)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (1)
-
John Ferlan