[libvirt] [PATCH v4 3/4] nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable
John Ferlan
jferlan at redhat.com
Fri Feb 2 19:33:15 UTC 2018
On 01/31/2018 10:10 PM, Stefan Berger wrote:
> On 12/08/2017 09:01 AM, John Ferlan wrote:
>> 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 at redhat.com>
>> ---
>> src/conf/virnwfilterobj.c | 402
>> ++++++++++++++++++++++++++++-------------
>> src/conf/virnwfilterobj.h | 2 +-
>> src/nwfilter/nwfilter_driver.c | 5 +-
>> 3 files changed, 282 insertions(+), 127 deletions(-)
>>
>> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
>> index 6b4758656..a4e6a03d2 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))) {
[1] As part of the magic of hash tables, when an element is removed from
the hash table the virObjectFreeHashData is called which does a
virObjectUnref...
>> + 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);
> Unref here after successful removal from hash bucket?
>> + virHashRemoveEntry(nwfilters->objsName, def->name);
> Again unref here after successful removal ?
[1] So when we RemoveEntry, the data->tableFree (from our Create) is
called. So, yes, that Unref happens automagically. Once the last ref is
removed the object's disposal API (virNWFilterObjDispose) is called
before being completely reaped.
>> + 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;
>> }
>>
>>
>> @@ -391,8 +449,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)) {
>> @@ -406,10 +467,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);
>> @@ -424,11 +482,13 @@
>> 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))) {
>> - virNWFilterObjPromoteToWrite(obj);
>> + /* 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 it
>> + * while we adjust data within. */
>> + virObjectRWLockRead(nwfilters);
>> + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters,
>> def->name))) {
>> + virObjectRWUnlock(nwfilters);
>> + virObjectRWLockWrite(obj);
>>
>> objdef = obj->def;
>> if (virNWFilterDefEqual(def, objdef)) {
>
> I think you should have left the above PromoteToWrite(obj) rather than
> doing a virObjectRWLockWrite(obj) now and would then adapt the code here
> as mentioned in review of 2/4.
>
Hmm... true... I think that's because originally I had done the list
patch before the object patch... So it's probably one of those merge
things...
Good catch.
>> @@ -458,37 +518,112 @@
>> 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);
>
> good, here you take a reference because obj ended in the hash bucket.
> Therefore, further above, you have to unref when taking it out of the
> hash bucket.
>
>> +
>> + if (virHashAddEntry(nwfilters->objsName, def->name, obj) < 0) {
>> + virHashRemoveEntry(nwfilters->objs, uuidstr);
>> + goto error;
>> }
>> + virObjectRef(obj);
>> +
> Same comment here. Looks also better to me since taking the reference is
> done 'closer' to virHashAddEntry() call.
>
Right. That was the "end goal"...
>> obj->def = def;
>>
>> - return virObjectRef(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 };
>
> style: one of these per line ?
>
I've never seen a consistent style for these things, but I can move the
.conn to the same line as .filter which is done in other initializers
where there is only one line.
>>
>> - 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;
>> }
>>
>>
>> @@ -499,82 +634,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;
>> +
> nit: indentation error
>
Thanks!
>> + 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));
>
> can we ignore errors here and not return -1 in case an error occurs ?
>
Yes, the error would be we cannot shrink... This is common between
vir.*ObjList.*Export.* functions
Thanks again for the detailed review. After I push patch 1, I'll repost
the remaining ones. Maybe Laine will also have some luck with his
nwfilter testing. He's pursuing either a locking problem or a *severe*
performance problem with the libvirt-tck test.
John
>> + *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 b38740b15..b24f59379 100644
>> --- a/src/nwfilter/nwfilter_driver.c
>> +++ b/src/nwfilter/nwfilter_driver.c
>> @@ -369,11 +369,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;
>> }
>>
>
More information about the libvir-list
mailing list