1:33 p.m.
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(a)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;
> }
>