On Thu, Jul 20, 2017 at 04:32:46PM -0400, John Ferlan wrote:
>
>
> 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 ;-)
Just don't forget to change it when pushing ;).
>
> 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.
Thanks for summary and pointing me to the commit, it makes sense now.
...
> >> + 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
Yep, those are the ones I meant, see my reply to v6.
Erik
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
