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

[Erik Skultety]

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.

> +    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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: squash.patch
Type: text/x-diff
Size: 4246 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170724/eaa6729b/attachment-0001.bin>