[ovs-dev] [ovs-dev, v16, 1/5] Change encaps_run to work incrementally

[Ryan Moats]

Ben Pfaff <blp at ovn.org> wrote on 05/19/2016 10:32:53 AM:

> From: Ben Pfaff <blp at ovn.org>
> To: Ryan Moats/Omaha/IBM at IBMUS
> Cc: dev at openvswitch.org
> Date: 05/19/2016 10:33 AM
> Subject: Re: [ovs-dev,v16,1/5] Change encaps_run to work incrementally
>
> On Thu, May 19, 2016 at 10:02:08AM -0500, Ryan Moats wrote:
> > Ben Pfaff <blp at ovn.org> wrote on 05/17/2016 10:13:19 PM:
> >
> > > From: Ben Pfaff <blp at ovn.org>
> > > To: Ryan Moats/Omaha/IBM at IBMUS
> > > Cc: dev at openvswitch.org
> > > Date: 05/17/2016 10:14 PM
> > > Subject: Re: [ovs-dev,v16,1/5] Change encaps_run to work
incrementally
> > >
> > > On Mon, May 02, 2016 at 10:26:54AM -0500, Ryan Moats wrote:
> > > > As a side effect, tunnel context is persisted.
> > > >
> > > > Signed-off-by: Ryan Moats <rmoats at us.ibm.com>
> > >
> > > Thanks for updating this.
> >
> > and thanks for looking - sorry for the delayed reply (I've been
> > doing OVN training the past couple of days)
> >
> > > In a couple of places, this uses hmap_first_with_hash() to find an
> > > element in a hash table.  ovn-controller uses this method in some
> > > special cases where the hash value is known to be unique; for
example, I
> > > think that it's used for a hash table where the "hash" is the
assigned
> > > logical datapath ID, which is a unique 32-bit (maybe shorter? I don't
> > > recall at the moment) number.  But that trick doesn't work when the
hash
> > > value is really a hash.  For example, it can't be used in this code
> > > where the hash is taken from a UUID, because there might be multiple
> > > UUIDs with the same hash value.  It's necessary, instead, to iterate
> > > through the items that have the desired hash value, with
> > > HMAP_FOR_EACH_WITH_HASH, and then compare the item's full key instead
of
> > > just the hash.
> >
> > Ugh - I thought I had changed that, but when I couple this with your
> > comments below, I'm thinking I've confused myself as to what patches
> > I have and haven't pushed
> >
> > > In the process_full_encaps case, I don't see what removes tunnels
that
> > > are no longer needed.
> > >
> > > This has some TODOs and commented-out code in it, so I suspect that
it's
> > > not really ready for full review?
> >
> > As I implied above, those shouldn't be there, so now I'm suspicious if
I've
> > lost track of a ball that I've been juggling... Since I've got to
rebase
> > the rest of the patches anyway, adding this one to the list won't add
> > that much additional effort...
>
> Yeah, it did seem weird, since going into it I expected that this patch
> was ready.  I'll look forward to the next version.
>

I've found the right code for encaps.c and re-looking at the tunnel
update path, I'm really dubious about it being correct - I'm out this
afternoon (CDT) for a HS graduation, but if I post just the first patch
as an RFC how soon would it capture eyeballs?  I ask because if it will
be a while, then I'll just work through the rest of the patches first.

Ryan