[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v5 2/3] nodedev: Convert virNodeDeviceObjListPtr to use hash tables



> @@ -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 */

I think you meant lookup-by-name ^here. More importantly though, I don't
understand the comment at all, well I understand the words :), but the context
is all noise - why should be the lookup lockless? You always lock the objlist
prior to calling virHashLookup, followed by ref'ing and returning the result, I
know we have that in other conf/vir*obj* modules and those don't make any more
sense to me either.

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

I recall having discussed ^this with you already, but this is one-time use only
and I simply don't think it's necessary, I'd use helper variables for the sole
purpose of getting rid of multiple levels of dereference either in a multi-use
situation, or the number of levels dereferenced makes the line really long and
the usage unreadable. (this also occurs on multiple places...no need to repeat
this...)

> +    const char *sysfs_path = opaque;
> +    int want = 0;
> +
> +    virObjectLock(obj);
> +    def = obj->def;
> +    if (STREQ_NULLABLE(def->sysfs_path, sysfs_path))
> +        want = 1;
> +    virObjectUnlock(obj);
> +    return want;
> +}
> +
> +
>  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);
> +    virObjectRef(obj);
> +    virObjectUnlock(devs);

^This pattern occurs multiple times within the patch, I think it deserves a
simple helper

>
> +    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;
> +}
> +
> +
> +static virNodeDeviceObjPtr
> +virNodeDeviceObjListFindByNameLocked(virNodeDeviceObjListPtr devs,
> +                                     const char *name)
> +{
> +    return virObjectRef(virHashLookup(devs->objs, name));
>  }
>
>
> @@ -238,20 +274,42 @@ virNodeDeviceObjPtr
>  virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs,
>                                 const char *name)
>  {
> -    size_t i;
> -
> -    for (i = 0; i < devs->count; i++) {
> -        virNodeDeviceObjPtr obj = devs->objs[i];
> -        virNodeDeviceDefPtr def;
> +    virNodeDeviceObjPtr obj;
>
> +    virObjectLock(devs);
> +    obj = virNodeDeviceObjListFindByNameLocked(devs, name);
> +    virObjectUnlock(devs);
> +    if (obj)
>          virObjectLock(obj);
> -        def = obj->def;
> -        if (STREQ(def->name, name))
> -            return virObjectRef(obj);
> -        virObjectUnlock(obj);
> -    }
>
> -    return NULL;
> +    return obj;
> +}
> +
> +
> +struct virNodeDeviceObjListFindByWWNsData {
> +    const char *parent_wwnn;
> +    const char *parent_wwpn;
> +};
> +
> +static int
> +virNodeDeviceObjListFindByWWNsCallback(const void *payload,
> +                                       const void *name ATTRIBUTE_UNUSED,
> +                                       const void *opaque)
> +{
> +    virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
> +    struct virNodeDeviceObjListFindByWWNsData *data =
> +        (struct virNodeDeviceObjListFindByWWNsData *) opaque;
> +    virNodeDevCapsDefPtr cap;
> +    int want = 0;
> +
> +    virObjectLock(obj);
> +    if ((cap = virNodeDeviceFindFCCapDef(obj)) &&
> +        STREQ_NULLABLE(cap->data.scsi_host.wwnn, data->parent_wwnn) &&
> +        STREQ_NULLABLE(cap->data.scsi_host.wwpn, data->parent_wwpn) &&
> +        virNodeDeviceFindVPORTCapDef(obj))
> +        want = 1;
> +    virObjectUnlock(obj);
> +    return want;
>  }
>
>
> @@ -260,22 +318,40 @@ virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr devs,
>                                 const char *parent_wwnn,
>                                 const char *parent_wwpn)
>  {
> -    size_t i;
> +    virNodeDeviceObjPtr obj;
> +    struct virNodeDeviceObjListFindByWWNsData data = {
> +        .parent_wwnn = parent_wwnn, .parent_wwpn = parent_wwpn };
>
> -    for (i = 0; i < devs->count; i++) {
> -        virNodeDeviceObjPtr obj = devs->objs[i];
> -        virNodeDevCapsDefPtr cap;
> +    virObjectLock(devs);
> +    obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByWWNsCallback,
> +                        &data);
> +    virObjectRef(obj);
> +    virObjectUnlock(devs);
>
> +    if (obj)
>          virObjectLock(obj);
> -        if ((cap = virNodeDeviceFindFCCapDef(obj)) &&
> -            STREQ_NULLABLE(cap->data.scsi_host.wwnn, parent_wwnn) &&
> -            STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn) &&
> -            virNodeDeviceFindVPORTCapDef(obj))
> -            return virObjectRef(obj);
> -        virObjectUnlock(obj);
> -    }
>
> -    return NULL;
> +    return obj;
> +}
> +
> +
> +static int
> +virNodeDeviceObjListFindByFabricWWNCallback(const void *payload,
> +                                            const void *name ATTRIBUTE_UNUSED,
> +                                            const void *opaque)
> +{
> +    virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
> +    const char *matchstr = opaque;
> +    virNodeDevCapsDefPtr cap;
> +    int want = 0;
> +
> +    virObjectLock(obj);
> +    if ((cap = virNodeDeviceFindFCCapDef(obj)) &&
> +        STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, matchstr) &&
> +        virNodeDeviceFindVPORTCapDef(obj))
> +        want = 1;
> +    virObjectUnlock(obj);
> +    return want;
>  }
>
>
> @@ -283,21 +359,35 @@ static virNodeDeviceObjPtr
>  virNodeDeviceObjListFindByFabricWWN(virNodeDeviceObjListPtr devs,
>                                      const char *parent_fabric_wwn)
>  {
> -    size_t i;
> +    virNodeDeviceObjPtr obj;
>
> -    for (i = 0; i < devs->count; i++) {
> -        virNodeDeviceObjPtr obj = devs->objs[i];
> -        virNodeDevCapsDefPtr cap;
> +    virObjectLock(devs);
> +    obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByFabricWWNCallback,
> +                        (void *)parent_fabric_wwn);
> +    virObjectRef(obj);
> +    virObjectUnlock(devs);
>
> +    if (obj)
>          virObjectLock(obj);
> -        if ((cap = virNodeDeviceFindFCCapDef(obj)) &&
> -            STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn) &&
> -            virNodeDeviceFindVPORTCapDef(obj))
> -            return virObjectRef(obj);
> -        virObjectUnlock(obj);
> -    }
>
> -    return NULL;
> +    return obj;
> +}
> +
> +
> +static int
> +virNodeDeviceObjListFindByCapCallback(const void *payload,
> +                                      const void *name ATTRIBUTE_UNUSED,
> +                                      const void *opaque)
> +{
> +    virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
> +    const char *matchstr = opaque;
> +    int want = 0;
> +
> +    virObjectLock(obj);
> +    if (virNodeDeviceObjHasCap(obj, matchstr))
> +        want = 1;
> +    virObjectUnlock(obj);
> +    return want;
>  }
>
>
> @@ -305,18 +395,59 @@ static virNodeDeviceObjPtr
>  virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs,
>                                const char *cap)
>  {
> -    size_t i;
> +    virNodeDeviceObjPtr obj;
>
> -    for (i = 0; i < devs->count; i++) {
> -        virNodeDeviceObjPtr obj = devs->objs[i];
> +    virObjectLock(devs);
> +    obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByCapCallback,
> +                        (void *)cap);
> +    virObjectRef(obj);
> +    virObjectUnlock(devs);
>
> +    if (obj)
>          virObjectLock(obj);
> -        if (virNodeDeviceObjHasCap(obj, cap))
> -            return virObjectRef(obj);
> -        virObjectUnlock(obj);
> -    }
>
> -    return NULL;
> +    return obj;
> +}
> +
> +
> +struct virNodeDeviceObjListFindSCSIHostByWWNsData {
> +    const char *wwnn;
> +    const char *wwpn;
> +};
> +
> +static int
> +virNodeDeviceObjListFindSCSIHostByWWNsCallback(const void *payload,
> +                                               const void *name ATTRIBUTE_UNUSED,
> +                                               const void *opaque)
> +{
> +    virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
> +    virNodeDeviceDefPtr def;
> +    struct virNodeDeviceObjListFindSCSIHostByWWNsData *data =
> +        (struct virNodeDeviceObjListFindSCSIHostByWWNsData *) opaque;
> +    virNodeDevCapsDefPtr cap;
> +    int want = 0;
> +
> +    virObjectLock(obj);
> +    def = obj->def;
> +    cap = def->caps;
> +
> +    while (cap) {
> +        if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) {
> +            virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host);
> +            if (cap->data.scsi_host.flags &
> +                VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) {
> +                if (STREQ(cap->data.scsi_host.wwnn, data->wwnn) &&
> +                    STREQ(cap->data.scsi_host.wwpn, data->wwpn)) {
> +                    want = 1;
> +                    break;
> +                }
> +            }
> +        }
> +        cap = cap->next;
> +     }
> +
> +    virObjectUnlock(obj);
> +    return want;
>  }
>
>
> @@ -325,31 +456,30 @@ virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs,
>                                         const char *wwnn,
>                                         const char *wwpn)
>  {
> -    size_t i;
> +    virNodeDeviceObjPtr obj;
> +    struct virNodeDeviceObjListFindSCSIHostByWWNsData data = {
> +        .wwnn = wwnn, .wwpn = wwpn };
>
> -    for (i = 0; i < devs->count; i++) {
> -        virNodeDeviceObjPtr obj = devs->objs[i];
> -        virNodeDevCapsDefPtr cap;
> +    virObjectLock(devs);
> +    obj = virHashSearch(devs->objs,
> +                        virNodeDeviceObjListFindSCSIHostByWWNsCallback,
> +                        &data);
> +    virObjectRef(obj);
> +    virObjectUnlock(devs);
>
> +    if (obj)
>          virObjectLock(obj);
> -        cap = obj->def->caps;
> -
> -        while (cap) {
> -            if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) {
> -                virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host);
> -                if (cap->data.scsi_host.flags &
> -                    VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) {
> -                    if (STREQ(cap->data.scsi_host.wwnn, wwnn) &&
> -                        STREQ(cap->data.scsi_host.wwpn, wwpn))
> -                        return virObjectRef(obj);
> -                }
> -            }
> -            cap = cap->next;
> -        }
> -        virObjectUnlock(obj);
> -    }
>
> -    return NULL;
> +    return obj;
> +}
> +
> +
> +static void
> +virNodeDeviceObjListDispose(void *obj)
> +{
> +    virNodeDeviceObjListPtr devs = obj;
> +
> +    virHashFree(devs->objs);
>  }
>
>
> @@ -358,8 +488,17 @@ virNodeDeviceObjListNew(void)
>  {
>      virNodeDeviceObjListPtr devs;
>
> -    if (VIR_ALLOC(devs) < 0)
> +    if (virNodeDeviceObjInitialize() < 0)
> +        return NULL;
> +
> +    if (!(devs = virObjectLockableNew(virNodeDeviceObjListClass)))
>          return NULL;
> +
> +    if (!(devs->objs = virHashCreate(50, virObjectFreeHashData))) {
> +        virObjectUnref(devs);
> +        return NULL;
> +    }
> +
>      return devs;
>  }
>
> @@ -367,11 +506,7 @@ virNodeDeviceObjListNew(void)
>  void
>  virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs)
>  {
> -    size_t i;
> -    for (i = 0; i < devs->count; i++)
> -        virObjectUnref(devs->objs[i]);
> -    VIR_FREE(devs->objs);
> -    VIR_FREE(devs);
> +    virObjectUnref(devs);
>  }
>
>
> @@ -381,22 +516,28 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs,
>  {
>      virNodeDeviceObjPtr obj;
>
> -    if ((obj = virNodeDeviceObjListFindByName(devs, def->name))) {
> +    virObjectLock(devs);
> +
> +    if ((obj = virNodeDeviceObjListFindByNameLocked(devs, def->name))) {
> +        virObjectLock(obj);
>          virNodeDeviceDefFree(obj->def);
>          obj->def = def;
> -        return obj;
> -    }
> +    } else {
> +        if (!(obj = virNodeDeviceObjNew()))
> +            goto cleanup;
>
> -    if (!(obj = virNodeDeviceObjNew()))
> -        return NULL;
> +        if (virHashAddEntry(devs->objs, def->name, obj) < 0) {
> +            virNodeDeviceObjEndAPI(&obj);
> +            goto cleanup;
> +        }
>
> -    if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, obj) < 0) {
> -        virNodeDeviceObjEndAPI(&obj);
> -        return NULL;
> +        obj->def = def;
> +        virObjectRef(obj);
>      }
> -    obj->def = def;
>
> -    return virObjectRef(obj);
> + cleanup:
> +    virObjectUnlock(devs);
> +    return obj;
>  }
>
>
> @@ -404,21 +545,20 @@ void
>  virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs,
>                             virNodeDeviceObjPtr obj)
>  {
> -    size_t i;
> -
> -    virObjectUnlock(obj);
> +    virNodeDeviceDefPtr def;
>
> -    for (i = 0; i < devs->count; i++) {
> -        virObjectLock(devs->objs[i]);
> -        if (devs->objs[i] == obj) {
> -            virObjectUnlock(devs->objs[i]);
> -            virObjectUnref(devs->objs[i]);
> +    if (!obj)
> +        return;
> +    def = obj->def;

In this specific case, what's the benefit of having @def...

>
> -            VIR_DELETE_ELEMENT(devs->objs, i, devs->count);
> -            break;
> -        }
> -        virObjectUnlock(devs->objs[i]);
> -    }
> +    virObjectRef(obj);
> +    virObjectUnlock(obj);
> +    virObjectLock(devs);
> +    virObjectLock(obj);
> +    virHashRemoveEntry(devs->objs, def->name);

...and use it here instead of obj->def->name... I'd prefer if you just dropped
it here.

> +    virObjectUnlock(obj);
> +    virObjectUnref(obj);
> +    virObjectUnlock(devs);
>  }
>
>
> @@ -643,25 +783,89 @@ virNodeDeviceCapMatch(virNodeDeviceObjPtr obj,
>  }
>
>
> +struct virNodeDeviceCountData {
> +    virConnectPtr conn;
> +    virNodeDeviceObjListFilter aclfilter;
> +    const char *matchstr;
> +    int count;
> +};

These single purpose structures often have the same members across multiple
callbacks, I was wondering whether we could just make one generic one, call it
virNodeDeviceQueryData. But then, it would unnecessarily big, most fields would
be unused, I don't know, it might not be a "nicer" solution eventually, so I'm
ambivalent on this...

Erik


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]