On 02/06/2018 08:20 PM, John Ferlan wrote: 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). What is the idea behind this if()? I don't understand it. There doesn't seem to be any fields in @objdef or Michal

On 02/08/2018 08:13 AM, Michal Privoznik wrote: I dunno, worked for me. The helper is being called from a thread that has taken out a READ lock at most one time and needs to get the WRITE lock in order to change things. If something else has the READ lock that thread waits until the other thread releases the READ lock as far as I understand things. Back when I first did this I had quite a lot of debug code and I have a vague recollection of seeing output from this particular path and seeing a couple of unlocks before the WRITE was taken while running the avocado tests. I have zero interest in spending more time on this. That ship sailed. If the decision is to drop the patches, then fine. I tried. I disagree that NWFilterObj is not a candidate for RWLockable - in fact it's quite the opposite *because* of the recursive locks. Additionally, because of the recursive locks we cannot use the virObjectLockable and quite frankly I see zero benefit from going down the path of just virObject. As they say, patches welcome. Somewhere along the line, I recall trying to post patches that were essentially virObjectRecursiveLockable and got NACK'd. Or you lost your train of thought? Happens with this code. The *DefEqual ensures that the new definition is the exact same as the old, but replaces the def for whatever reason - kind of the redefine type logic. If something is different, then we're stuck going through the FilterRebuild logic. That's about the extent of what I understand. John

[...] The reason I chose rwlocks and taking a read lock by default in the FindByUUID and FindByName (as opposed to other vir*FindBy* API's which return a write locked object) is because I know there's the potential for other code to be doing Reads and rwlocks allowed for that without the need to create any sort of recursive lockable object or any sort of API to perform "trylock"'s (all things I tried until the epiphany to use rwlocks was reached when you added them for other drivers and their list protection). In the long/short run I figured that having a common way to get the lock and then deciding to change it from Read to Write as necessary would work just fine for the purpose that was needed. If we had a real lock manager available this wouldn't be a problem ;-) [sorry just my OpenVMS roots and bias showing]. Still if you read up a bit on "dlm" you'll get an idea of what I mean. There's so many locks in the nwfilter code it's a wonder it all works. It's truly unfortunate that the wrlock mechanism is undefined for promoting a read lock to write leaving the consumer with few options. Since we can limit the "updating" of the fields in @obj and getting a write lock to very specific code paths, hopefully we can limit damage and/or deadlock type and/or TOCTOU issues Yes, reduction of duplication of vir*ObjectList is/was a goal. How to get there is where opinions start to vary - my code was essentially NACK'd, but getting to a point where someone else could try something different I figured would at least be helpful. This nwfilter code is the black sheep of the libvirt family in more ways than one. BTW: We're still not at a point where common List mgmt code could be written because the domain list mgmt varies greatly between the various hypervisor's. I started writing some patches to work through those issues, but have been side tracked by other higher priority items. BTW, see: https://www.redhat.com/archives/libvir-list/2017-May/msg01183.html for my pass at virObjectLockableRecursiveNew Right, pre-existing is what I'm going with 0-) John

Implement the self locking object list for nwfilter object lists that uses two hash tables to store the nwfilter object by UUID or by Name. As part of this alter the uuid argument to virNWFilterObjLookupByUUID to expect an already formatted uuidstr. Alter the existing list traversal code to implement the hash table find/lookup/search functionality. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/virnwfilterobj.c | 405 ++++++++++++++++++++++++++++------------- src/conf/virnwfilterobj.h | 2 +- src/nwfilter/nwfilter_driver.c | 5 +- 3 files changed, 283 insertions(+), 129 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index cd52706ec..75a6584a2 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -43,12 +43,21 @@ struct _virNWFilterObj { }; struct _virNWFilterObjList { - size_t count; - virNWFilterObjPtr *objs; + virObjectRWLockable 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 virNWFilterObjClass; +static virClassPtr virNWFilterObjListClass; static void virNWFilterObjDispose(void *opaque); +static void virNWFilterObjListDispose(void *opaque); static int @@ -60,6 +69,12 @@ virNWFilterObjOnceInit(void) virNWFilterObjDispose))) return -1; + if (!(virNWFilterObjListClass = virClassNew(virClassForObjectRWLockable(), + "virNWFilterObjList", + sizeof(virNWFilterObjList), + virNWFilterObjListDispose))) + return -1; + return 0; } @@ -144,14 +159,20 @@ virNWFilterObjDispose(void *opaque) } +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++) - virObjectUnref(nwfilters->objs[i]); - VIR_FREE(nwfilters->objs); - VIR_FREE(nwfilters); + virObjectUnref(nwfilters); } @@ -160,8 +181,23 @@ virNWFilterObjListNew(void) { virNWFilterObjListPtr nwfilters; - if (VIR_ALLOC(nwfilters) < 0) + if (virNWFilterObjInitialize() < 0) + return NULL; + + if (!(nwfilters = virObjectRWLockableNew(virNWFilterObjListClass))) + return NULL; + + if (!(nwfilters->objs = virHashCreate(10, virObjectFreeHashData))) { + virObjectUnref(nwfilters); + return NULL; + } + + if (!(nwfilters->objsName = virHashCreate(10, virObjectFreeHashData))) { + virHashFree(nwfilters->objs); + virObjectUnref(nwfilters); return NULL; + } + return nwfilters; } @@ -170,83 +206,105 @@ void virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjPtr obj) { - size_t i; - - virObjectRWUnlock(obj); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virNWFilterDefPtr def; - for (i = 0; i < nwfilters->count; i++) { - virObjectRWLockWrite(nwfilters->objs[i]); - if (nwfilters->objs[i] == obj) { - virObjectRWUnlock(nwfilters->objs[i]); - virObjectUnref(nwfilters->objs[i]); + if (!obj) + return; + def = obj->def; - VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count); - break; - } - virObjectRWUnlock(nwfilters->objs[i]); - } + virUUIDFormat(def->uuid, uuidstr); + virObjectRef(obj); + virObjectRWUnlock(obj); + virObjectRWLockWrite(nwfilters); + virObjectRWLockWrite(obj); + virHashRemoveEntry(nwfilters->objs, uuidstr); + virHashRemoveEntry(nwfilters->objsName, def->name); + virObjectRWUnlock(obj); + virObjectUnref(obj); + virObjectRWUnlock(nwfilters); } /** - * virNWFilterObjListFindByUUID + * virNWFilterObjListFindByUUID[Locked] * @nwfilters: Pointer to filter list - * @uuid: UUID to use to lookup the object + * @uuidstr: UUID to use to lookup the object + * + * The static [Locked] version would only be used when the Object List is + * already locked, such as is the case during virNWFilterObjListAssignDef. + * The caller is thus responsible for locking the object. * * Search for the object by uuidstr in the hash table and return a read * locked copy of the object. * + * Returns: A reffed object or NULL on error + */ +static virNWFilterObjPtr +virNWFilterObjListFindByUUIDLocked(virNWFilterObjListPtr nwfilters, + const char *uuidstr) +{ + return virObjectRef(virHashLookup(nwfilters->objs, uuidstr)); +} + + +/* * Returns: A reffed and read locked object or NULL on error */ virNWFilterObjPtr virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, - const unsigned char *uuid) + const char *uuidstr) { - size_t i; virNWFilterObjPtr obj; - virNWFilterDefPtr def; - for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; + virObjectRWLockRead(nwfilters); + obj = virNWFilterObjListFindByUUIDLocked(nwfilters, uuidstr); + virObjectRWUnlock(nwfilters); + if (obj) virObjectRWLockRead(obj); - def = obj->def; - if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN)) - return virObjectRef(obj); - virObjectRWUnlock(obj); - } - return NULL; + return obj; } /** - * virNWFilterObjListFindByName + * virNWFilterObjListFindByName[Locked] * @nwfilters: Pointer to filter list * @name: filter name to use to lookup the object * + * The static [Locked] version would only be used when the Object List is + * already locked, such as is the case during virNWFilterObjListAssignDef. + * The caller is thus responsible for locking the object. + * * Search for the object by name in the hash table and return a read * locked copy of the object. * + * Returns: A reffed object or NULL on error + */ +static virNWFilterObjPtr +virNWFilterObjListFindByNameLocked(virNWFilterObjListPtr nwfilters, + const char *name) +{ + return virObjectRef(virHashLookup(nwfilters->objsName, name)); +} + + +/* * Returns: A reffed and read locked object or NULL on error */ 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]; + virObjectRWLockRead(nwfilters); + obj = virNWFilterObjListFindByNameLocked(nwfilters, name); + virObjectRWUnlock(nwfilters); + if (obj) virObjectRWLockRead(obj); - def = obj->def; - if (STREQ_NULLABLE(def->name, name)) - return virObjectRef(obj); - virObjectRWUnlock(obj); - } - return NULL; + return obj; } @@ -392,8 +450,11 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, { virNWFilterObjPtr obj; virNWFilterDefPtr objdef; + char uuidstr[VIR_UUID_STRING_BUFLEN]; - if ((obj = virNWFilterObjListFindByUUID(nwfilters, def->uuid))) { + virUUIDFormat(def->uuid, uuidstr); + + if ((obj = virNWFilterObjListFindByUUID(nwfilters, uuidstr))) { objdef = obj->def; if (STRNEQ(def->name, objdef->name)) { @@ -407,10 +468,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterObjEndAPI(&obj); } else { if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - objdef = obj->def; - virUUIDFormat(objdef->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("filter '%s' already exists with uuid %s"), def->name, uuidstr); @@ -425,24 +483,26 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, return NULL; } - - /* Get a READ lock and immediately promote to WRITE while we adjust - * data within. */ - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { + /* We're about to make some changes to objects on the list - so get the + * list READ lock in order to Find the object and WRITE lock the object + * since both paths would immediately promote it anyway */ + virObjectRWLockRead(nwfilters); + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) { + virObjectRWLockWrite(obj); + virObjectRWUnlock(nwfilters); 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; + + /* Demote while the trigger runs since it may need to grab a read + * lock on this object and promote before returning. */ virNWFilterObjDemoteFromWrite(obj); /* trigger the update on VMs referencing the filter */ @@ -462,39 +522,113 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, return obj; } + /* Promote the nwfilters to add a new object */ + virObjectRWUnlock(nwfilters); + virObjectRWLockWrite(nwfilters); if (!(obj = virNWFilterObjNew())) - return NULL; + goto cleanup; - if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, - nwfilters->count, obj) < 0) { - virNWFilterObjEndAPI(&obj); - return NULL; + if (virHashAddEntry(nwfilters->objs, uuidstr, obj) < 0) + goto error; + virObjectRef(obj); + + if (virHashAddEntry(nwfilters->objsName, def->name, obj) < 0) { + virHashRemoveEntry(nwfilters->objs, uuidstr); + goto error; } virObjectRef(obj); + obj->def = def; virNWFilterObjDemoteFromWrite(obj); + cleanup: + virObjectRWUnlock(nwfilters); return obj; + + error: + virObjectRWUnlock(obj); + virObjectUnref(obj); + virObjectRWUnlock(nwfilters); + return NULL; } +struct virNWFilterCountData { + virConnectPtr conn; + virNWFilterObjListFilter filter; + int nelems; +}; + +static int +virNWFilterObjListNumOfNWFiltersCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNWFilterObjPtr obj = payload; + struct virNWFilterCountData *data = opaque; + + virObjectRWLockRead(obj); + if (!data->filter || data->filter(data->conn, obj->def)) + data->nelems++; + virObjectRWUnlock(obj); + return 0; +} + int virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters, virConnectPtr conn, virNWFilterObjListFilter filter) { - size_t i; - int nfilters = 0; + struct virNWFilterCountData data = { + .conn = conn, .filter = filter, .nelems = 0 }; - for (i = 0; i < nwfilters->count; i++) { - virNWFilterObjPtr obj = nwfilters->objs[i]; - virObjectRWLockRead(obj); - if (!filter || filter(conn, obj->def)) - nfilters++; - virObjectRWUnlock(obj); + virObjectRWLockRead(nwfilters); + virHashForEach(nwfilters->objs, virNWFilterObjListNumOfNWFiltersCallback, + &data); + virObjectRWUnlock(nwfilters); + + return data.nelems; +} + + +struct virNWFilterListData { + virConnectPtr conn; + virNWFilterObjListFilter filter; + 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; + + virObjectRWLockRead(obj); + def = obj->def; + + if (!data->filter || data->filter(data->conn, def)) { + if (VIR_STRDUP(data->elems[data->nelems], def->name) < 0) { + data->error = true; + goto cleanup; + } + data->nelems++; } - return nfilters; + cleanup: + virObjectRWUnlock(obj); + return 0; } @@ -505,82 +639,103 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr nwfilters, char **const names, int maxnames) { - int nnames = 0; - size_t i; - virNWFilterDefPtr def; + struct virNWFilterListData data = { .conn = conn, .filter = filter, + .nelems = 0, .elems = names, .maxelems = maxnames, .error = false }; - for (i = 0; i < nwfilters->count && nnames < maxnames; i++) { - virNWFilterObjPtr obj = nwfilters->objs[i]; - virObjectRWLockRead(obj); - def = obj->def; - if (!filter || filter(conn, def)) { - if (VIR_STRDUP(names[nnames], def->name) < 0) { - virObjectRWUnlock(obj); - goto failure; - } - nnames++; - } - virObjectRWUnlock(obj); - } + virObjectRWLockRead(nwfilters); + virHashForEach(nwfilters->objs, virNWFilterObjListGetNamesCallback, &data); + virObjectRWUnlock(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; } +struct virNWFilterExportData { + virConnectPtr conn; + virNWFilterObjListFilter filter; + virNWFilterPtr *filters; + int nfilters; + bool error; +}; + +static int +virNWFilterObjListExportCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNWFilterObjPtr obj = payload; + virNWFilterDefPtr def; + + struct virNWFilterExportData *data = opaque; + virNWFilterPtr nwfilter; + + if (data->error) + return 0; + + virObjectRWLockRead(obj); + def = obj->def; + + if (data->filter && !data->filter(data->conn, def)) + goto cleanup; + + if (!data->filters) { + data->nfilters++; + goto cleanup; + } + + if (!(nwfilter = virGetNWFilter(data->conn, def->name, def->uuid))) { + data->error = true; + goto cleanup; + } + data->filters[data->nfilters++] = nwfilter; + + cleanup: + virObjectRWUnlock(obj); + return 0; +} + + int virNWFilterObjListExport(virConnectPtr conn, virNWFilterObjListPtr nwfilters, virNWFilterPtr **filters, virNWFilterObjListFilter filter) { - virNWFilterPtr *tmp_filters = NULL; - int nfilters = 0; - virNWFilterPtr nwfilter = NULL; - virNWFilterObjPtr obj = NULL; - virNWFilterDefPtr def; - size_t i; - int ret = -1; + struct virNWFilterExportData data = { .conn = conn, .filter = filter, + .filters = NULL, .nfilters = 0, .error = false }; - if (!filters) { - ret = nwfilters->count; - goto cleanup; + virObjectRWLockRead(nwfilters); + if (filters && + VIR_ALLOC_N(data.filters, virHashSize(nwfilters->objs) + 1) < 0) { + virObjectRWUnlock(nwfilters); + return -1; } - if (VIR_ALLOC_N(tmp_filters, nwfilters->count + 1) < 0) - goto cleanup; + virHashForEach(nwfilters->objs, virNWFilterObjListExportCallback, &data); + virObjectRWUnlock(nwfilters); - for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; - virObjectRWLockRead(obj); - def = obj->def; - if (!filter || filter(conn, def)) { - if (!(nwfilter = virGetNWFilter(conn, def->name, def->uuid))) { - virObjectRWUnlock(obj); - goto cleanup; - } - tmp_filters[nfilters++] = nwfilter; - } - virObjectRWUnlock(obj); + 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; } - *filters = tmp_filters; - tmp_filters = NULL; - ret = nfilters; + return data.nfilters; cleanup: - if (tmp_filters) { - for (i = 0; i < nfilters; i ++) - virObjectUnref(tmp_filters[i]); - } - VIR_FREE(tmp_filters); - - return ret; + virObjectListFree(data.filters); + return -1; } diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 0281bc5f5..cabb42a71 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -65,7 +65,7 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjPtr virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, - const unsigned char *uuid); + const char *uuidstr); virNWFilterObjPtr virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 3a0447627..a01c41b5c 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -435,11 +435,10 @@ nwfilterObjFromNWFilter(const unsigned char *uuid) virNWFilterObjPtr obj; char uuidstr[VIR_UUID_STRING_BUFLEN]; - if (!(obj = virNWFilterObjListFindByUUID(driver->nwfilters, uuid))) { - virUUIDFormat(uuid, uuidstr); + virUUIDFormat(uuid, uuidstr); + if (!(obj = virNWFilterObjListFindByUUID(driver->nwfilters, uuidstr))) virReportError(VIR_ERR_NO_NWFILTER, _("no nwfilter with matching uuid '%s'"), uuidstr); - } return obj; } -- 2.13.6

On 02/08/2018 08:13 AM, Michal Privoznik wrote: I dunno - works in the tests I've run (libvirt-tck and avocado). John

On 02/08/2018 08:13 AM, Michal Privoznik wrote: Could we keep the read lock and grab a 2nd lock as a write-lock? It wouldn't be a virObject call, though. Stefan

On 02/09/2018 01:48 AM, Michal Privoznik wrote: We can grab lock A and then lock B. Lock B would either be a read/write lock locked as a write lock or would be an exclusive lock. Why would that not work? Stefan

On 02/12/2018 04:38 AM, Michal Privoznik wrote: We could use the lock for recursive locking as we do it now. For 'promoting' to write lock we would grab a 2nd lock that's exclusive. Stefan