[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



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


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