[ovs-dev] ovsdb-monitor: Refactor ovsdb monitor implementation.

[Han Zhou]

On Mon, Feb 25, 2019 at 8:45 AM Ben Pfaff <blp at ovn.org> wrote:
>
> On Fri, Feb 22, 2019 at 10:59:00PM -0800, Han Zhou wrote:
> > On Fri, Feb 22, 2019 at 8:36 PM Ben Pfaff <blp at ovn.org> wrote:
> > >
> > > On Fri, Feb 22, 2019 at 07:31:59PM -0800, Han Zhou wrote:
> > > > On Fri, Feb 22, 2019 at 5:27 PM Ben Pfaff <blp at ovn.org> wrote:
> > > > >
> > > > > On Fri, Feb 22, 2019 at 05:21:51PM -0800, Han Zhou wrote:
> > > > > > On Fri, Feb 22, 2019 at 3:04 PM Ben Pfaff <blp at ovn.org> wrote:
> > > > > > >
> > > > > > > Using HMAP_FOR_EACH_SAFE_WITH_HASH tends to imply that an hmap can have
> > > > > > > more than one item with a given key.  Nothing prevents that, and it will
> > > > > > > otherwise work, but it is unusual and it normally makes sense to use an
> > > > > > > hindex instead of an hmap for efficiency's sake if it is going to happen
> > > > > > > very often.  Does the data structure in question often have duplicates?
> > > > > > > Should we change it to an hindex?
> > > > > >
> > > > > > Hi Ben, I think you are asking if there can be more than one item with
> > > > > > a given "hash value". Yes it is possible, but it should not be often
> > > > > > for the data structure in question here - the json cache. The key is
> > > > > > uuid of the change set combined with monitor version, and the hash is
> > > > > > the uuid_hash. So conflicts of uuid_hash should be rare. When there
> > > > > > are different versions (e.g. monitor v1, v2, v3) of clients referring
> > > > > > same change sets we do get more items with same hash value, but there
> > > > > > were at most 2 versions and I am adding 1 more in next patches in the
> > > > > > series, and I guess it is not likely we add too many versions of
> > > > > > monitors in the future, and even that is possible, it is unlikely that
> > > > > > many different versions of clients co-exists in same environment. So I
> > > > > > don't think it has a real impact to performance.
> > > > > >
> > > > > > The reason for using HMAP_FOR_EACH_SAFE_WITH_HASH instead of
> > > > > > HMAP_FOR_EACH_WITH_HASH is just because the node is going to be
> > > > > > removed and freed in the loop. It needs to be safe regardless of the
> > > > > > probability of hash conflicts.
> > > > >
> > > > > It's important to be clear about the difference between hashes and keys.
> > > > > The hash is what's stored in the hmap_node (that is, the uuid_hash()
> > > > > return value in this case), the key in this case is what uuid_equals()
> > > > > compares.  I *think* you are saying there can be a small number of
> > > > > duplicate keys in the hmap in this case.  Is that correct?
> > > >
> > > > I thought you were talking about hashes instead of keys (I thought it
> > > > was a typo), because I assume keys are always unique.
> > > > Let me clarify a little more. In the json_cache hmap, the key is the
> > > > combination of <change_set_uuid, version> of the struct
> > > > ovsdb_monitor_json_cache_node. This key uniquely identifies each
> > > > element in the hmap. The hash function doesn't take version into the
> > > > hash calculation, so if there are multiple versions of clients exist
> > > > at the same time, there will be different elements with same hash.
> > > > However, since the number of versions should be very small (worse case
> > > > 3, most cases 1), this is not going to cause any performance problem.
> > > > There do exist probability of hash conflict even if there is only one
> > > > version, but the chances are very low (ensured by uuid_hash).
> > > >
> > > > Now for the function:
> > > > +/* Free all versions of json cache for a given change_set.*/
> > > > +static void
> > > > +ovsdb_monitor_json_cache_destroy(struct ovsdb_monitor *dbmon,
> > > > +                                 struct ovsdb_monitor_change_set *change_set)
> > > >
> > > > It is different from a general function that destroys an element in a
> > > > hmap with the unique key. Instead, it destroys *all versions* of the
> > > > json cache for a given change_set uuid. So it uses only part of the
> > > > key to do the search, which may have caused some confusion.
> > >
> > > Ah.  This is an unusual combination of factors.  Very unusual in fact.
> > > If it is something we really want, then I think that it should be
> > > carefully documented in a comment on the data structure.
> >
> > I agree. This trick introduces confusion without obvious benefit, and
> > the HMAP_FOR_EACH_SAFE_WITH_HASH is not really necessary. Instead of
> > playing tricks with the keys, I can simply use the search function to
> > find the nodes for all versions and destroy them if exists. Please see
> > below change on top of the current one. I will submit it as v3 if it
> > looks better.
>
> Thanks.  I like this better.  The repeated searches might make it
> slightly slower, but it is much clearer.

Thanks Ben, I just sent v3:
https://patchwork.ozlabs.org/project/openvswitch/list/?series=94085