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

20 Jul 2017


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

    

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

Erik Skultety