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@xxxxxxxxxx>
> ---
> 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
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 58481e782..03cc20026 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -224,6 +224,25 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj)
}
+static virNodeDeviceObjPtr
+virNodeDeviceObjListFindHelper(virNodeDeviceObjListPtr devs,
+ virHashSearcher iterator,
+ const void *data)
+{
+ virNodeDeviceObjPtr obj;
+
+ virObjectLock(devs);
+ obj = virHashSearch(devs->objs, iterator, data, NULL);
+ virObjectRef(obj);
+ virObjectUnlock(devs);
+
+ if (obj)
+ virObjectLock(obj);
+
+ return obj;
+}
+
+
static int
virNodeDeviceObjListFindBySysfsPathCallback(const void *payload,
const void *name ATTRIBUTE_UNUSED,
@@ -247,18 +266,9 @@ virNodeDeviceObjPtr
virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
const char *sysfs_path)
{
- virNodeDeviceObjPtr obj;
+ virHashSearcher cb = virNodeDeviceObjListFindBySysfsPathCallback;
- virObjectLock(devs);
- obj = virHashSearch(devs->objs, virNodeDeviceObjListFindBySysfsPathCallback,
- (void *)sysfs_path, NULL);
- virObjectRef(obj);
- virObjectUnlock(devs);
-
- if (obj)
- virObjectLock(obj);
-
- return obj;
+ return virNodeDeviceObjListFindHelper(devs, cb, sysfs_path);
}
@@ -318,20 +328,11 @@ virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr devs,
const char *parent_wwnn,
const char *parent_wwpn)
{
- virNodeDeviceObjPtr obj;
+ virHashSearcher cb = virNodeDeviceObjListFindByWWNsCallback;
struct virNodeDeviceObjListFindByWWNsData data = {
.parent_wwnn = parent_wwnn, .parent_wwpn = parent_wwpn };
- virObjectLock(devs);
- obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByWWNsCallback,
- &data, NULL);
- virObjectRef(obj);
- virObjectUnlock(devs);
-
- if (obj)
- virObjectLock(obj);
-
- return obj;
+ return virNodeDeviceObjListFindHelper(devs, cb, &data);
}
@@ -359,18 +360,9 @@ static virNodeDeviceObjPtr
virNodeDeviceObjListFindByFabricWWN(virNodeDeviceObjListPtr devs,
const char *parent_fabric_wwn)
{
- virNodeDeviceObjPtr obj;
+ virHashSearcher cb = virNodeDeviceObjListFindByFabricWWNCallback;
- virObjectLock(devs);
- obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByFabricWWNCallback,
- (void *)parent_fabric_wwn, NULL);
- virObjectRef(obj);
- virObjectUnlock(devs);
-
- if (obj)
- virObjectLock(obj);
-
- return obj;
+ return virNodeDeviceObjListFindHelper(devs, cb, parent_fabric_wwn);
}
@@ -395,18 +387,9 @@ static virNodeDeviceObjPtr
virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs,
const char *cap)
{
- virNodeDeviceObjPtr obj;
+ virHashSearcher cb = virNodeDeviceObjListFindByCapCallback;
- virObjectLock(devs);
- obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByCapCallback,
- (void *)cap, NULL);
- virObjectRef(obj);
- virObjectUnlock(devs);
-
- if (obj)
- virObjectLock(obj);
-
- return obj;
+ return virNodeDeviceObjListFindHelper(devs, cb, cap);
}
@@ -456,21 +439,11 @@ virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs,
const char *wwnn,
const char *wwpn)
{
- virNodeDeviceObjPtr obj;
+ virHashSearcher cb = virNodeDeviceObjListFindSCSIHostByWWNsCallback;
struct virNodeDeviceObjListFindSCSIHostByWWNsData data = {
.wwnn = wwnn, .wwpn = wwpn };
- virObjectLock(devs);
- obj = virHashSearch(devs->objs,
- virNodeDeviceObjListFindSCSIHostByWWNsCallback,
- &data, NULL);
- virObjectRef(obj);
- virObjectUnlock(devs);
-
- if (obj)
- virObjectLock(obj);
-
- return obj;
+ return virNodeDeviceObjListFindHelper(devs, cb, &data);
}
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
