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

[John Ferlan]

On 07/24/2017 03:52 AM, Erik Skultety wrote:
> 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 at redhat.com>
>> ---
>>  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.
> 

<sigh> OK, I really do dislike the obj->def->X syntax "just" for a few
functions while others use the @def nomenclature.

>> +    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.

Replies don't include your patch; however, I will note if you jump to
patch 13:

https://www.redhat.com/archives/libvir-list/2017-June/msg00929.html

of the virObject series I posted last month:

https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html

that you'll see that is the direction this would be headed anyway. I can
do this sooner that's fine, although I prefer the name
virNodeDeviceObjListSearch as opposed to virNodeDeviceObjListFindHelper.

Ironically though you didn't like the @def usage, still you chose
virHashSearcher cb = $functionName which has one use to be used as an
argument to the function even though $functionName could be used
directly in the call. The only advantage of @cb is that it reduces the
line length of the call.

John

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