Re: [libvirt] [PATCH v6 3/4] nodedev: Convert virNodeDeviceObjListPtr to use hash tables

On Thu, Jul 20, 2017 at 10:08:14AM -0400, John Ferlan wrote: > Rather than use a forward linked list of elements, it'll be much more > efficient to use a hash table to reference the elements by unique name > and to perform hash searches. > > This patch does all the heavy lifting of converting the list object to > use a self locking list that contains the hash table. Each of the FindBy > functions that do not involve finding the object by it's key (name) is > converted to use virHashSearch in order to find the specific object. > When searching for the key (name), it's possible to use virHashLookup. > For any of the list perusal functions that are required to evaluate > each object, the virHashForEach function is used. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/conf/virnodedeviceobj.c | 575 ++++++++++++++++++++++++++++++-------------- > 1 file changed, 400 insertions(+), 175 deletions(-) > > diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c > index 035a56d..58481e7 100644 > --- a/src/conf/virnodedeviceobj.c > +++ b/src/conf/virnodedeviceobj.c > @@ -25,6 +25,7 @@ > #include "viralloc.h" > #include "virnodedeviceobj.h" > #include "virerror.h" > +#include "virhash.h" > #include "virlog.h" > #include "virstring.h" > > @@ -39,13 +40,19 @@ struct _virNodeDeviceObj { > }; > > struct _virNodeDeviceObjList { > - size_t count; > - virNodeDeviceObjPtr *objs; > + virObjectLockable parent; > + > + /* name string -> virNodeDeviceObj mapping > + * for O(1), lockless lookup-by-uuid */ > + virHashTable *objs; > + > }; > > > static virClassPtr virNodeDeviceObjClass; > +static virClassPtr virNodeDeviceObjListClass; > static void virNodeDeviceObjDispose(void *opaque); > +static void virNodeDeviceObjListDispose(void *opaque); > > static int > virNodeDeviceObjOnceInit(void) > @@ -56,6 +63,12 @@ virNodeDeviceObjOnceInit(void) > virNodeDeviceObjDispose))) > return -1; > > + if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectLockable(), > + "virNodeDeviceObjList", > + sizeof(virNodeDeviceObjList), > + virNodeDeviceObjListDispose))) > + return -1; > + > return 0; > } > > @@ -211,26 +224,49 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj) > } > > > +static int > +virNodeDeviceObjListFindBySysfsPathCallback(const void *payload, > + const void *name ATTRIBUTE_UNUSED, > + const void *opaque) > +{ > + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; > + virNodeDeviceDefPtr def; As we discussed in v5, I'd like you to drop ^these @def (and only @def...) declarations and usages across the whole patch for the reasons I already mentioned - it makes sense in general, but it's not very useful in this specific case.

> + const char *sysfs_path = opaque; > + int want = 0; > + > + virObjectLock(obj); > + def = obj->def; ... > virNodeDeviceObjPtr > virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, > const char *sysfs_path) > { > - size_t i; > + virNodeDeviceObjPtr obj; > > - for (i = 0; i < devs->count; i++) { > - virNodeDeviceObjPtr obj = devs->objs[i]; > - virNodeDeviceDefPtr def; > + virObjectLock(devs); > + obj = virHashSearch(devs->objs, virNodeDeviceObjListFindBySysfsPathCallback, > + (void *)sysfs_path, NULL); > + virObjectRef(obj); > + virObjectUnlock(devs); > > + if (obj) > virObjectLock(obj); > - def = obj->def; > - if ((def->sysfs_path != NULL) && > - (STREQ(def->sysfs_path, sysfs_path))) { > - return virObjectRef(obj); > - } > - virObjectUnlock(obj); > - } > > - return NULL; > + return obj; With reference to v5: The fact that creating a helper wasn't met with an agreement from the reviewer's side in one case doesn't mean it doesn't make sense to do in an other case, like this one. So, what I actually meant by creating a helper for: BySysfsPath ByWWNs ByFabricWWN ByCap ByCapSCSIByWWNs is just simply move the lock and search logic to a separate function, that's all, see my attached patch. And then, as you suggested in your v5 response to this patch, we can move from here (my patch included) and potentially do some more refactoring.

ACK if you move the lock-search-ref-unlock-return snippets to a separate function for the cases mentioned above. Erik