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

On 07/20/2017 10:48 AM, Erik Skultety wrote: >> @@ -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. > hrmph... this showed up after I posted v6.... I'll provide my comments here... Sure I meant "by-name"... The comment was originally sourced from _virDomainObjList... I've always just copied it around ;-)

The comment in/for domain objs list was added in commit id 'a3adcce79' when the code was still part of domain_conf.c I think the "decoding" is that prior to that one would have 0(n) mutex locks taken for each domain obj in the list as the list was being traversed. With hash tables one as 0(1) mutex locks taken to lock the list before get the entry out of the hash table.

>> + 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 > Oh - I've been down that fork in the road before - create what I think is a "simple" helper combining things only to get shot down during review because it was deemed unnecessary or that each of these should have their own separate pair of API's Each one of these is a unique way to search the objs list for a piece of data by some value that is not a key... FindBySysfsPath FindByWWNs FindByFabricWWN FindByCap FindSCSIHostByWWNs