[ovs-dev] [ovs-dev,v17,4/5] Persist ovn flow tables.

[Ben Pfaff]

On Mon, Jun 06, 2016 at 10:25:32AM -0500, Ryan Moats wrote:
> Ben Pfaff <blp at ovn.org> wrote on 06/03/2016 10:50:45 AM:
> 
> > From: Ben Pfaff <blp at ovn.org>
> > To: Ryan Moats/Omaha/IBM at IBMUS
> > Cc: dev at openvswitch.org
> > Date: 06/03/2016 10:50 AM
> > Subject: Re: [ovs-dev,v17,4/5] Persist ovn flow tables.
> >
> > On Sun, May 22, 2016 at 04:36:21PM -0500, Ryan Moats wrote:
> > > Ensure that ovn flow tables are persisted so that changes to
> > > them chan be applied incrementally - this is a prereq for
> > > making lflow_run and physical_run incremental.
> > >
> > > Signed-off-by: Ryan Moats <rmoats at us.ibm.com>
> >
> > I don't really understand the design here.
> >
> > One way that I find it difficult to understand is that ofctrl_add_flow()
> > has a new 'is_new' parameter that appears to be important, but every
> > call I see to ofctrl_add_flow() passes 'true' as the argument.
> 
> What's going on here is that is_new *will* be set to false in the
> _TRACKED branches of the next patch set.  However, given the size
> of that patch set, I pulled the is_new modifications into the patch
> set.  I'm open to suggestion on how to bundle all of this into more
> sane patch sets...

Without looking back at the patch again, I think that part of the issue
for me was that I had to guess what 'is_new' meant, because there wasn't
a comment that said or any user for is_new==false.  Can you add a
comment to ofctrl_add_flow() to explain its meaning?  That would help.

> > I don't understand the hashing scheme.  Partly this is due to the
> > comment here:
> >
> >  * Because it is possible for both actions and matches to change on a
> rule,
> >  * and because the hmap struct only supports a single hash, this method
> >  * uses two hash maps - one that uses table_id+priority+matches for its
> hash
> >  * and the other that uses table_id+priority+actions.
> >
> > I see the hash table based on table_id+priority+matches,
> > 'match_flow_table'.  I don't see a hash table that looks at
> > table_id+priority+actions, and I don't understand why that would be
> > useful.  I do see a hash table based on a uuid; is that the one you
> > mean?
> 
> Yes, that is my mistake.  I was originally hashing on actions, but that is
> too coarse a hash mechanism and so I later switched to uuid - I will update
> the comment when I respin the patch.

Thanks.

> > A hash table based on a uuid will not be unique, because multiple
> > OpenFlow flows can be generated from a single logical flow and all of
> > those will have the same UUID (the logical flow's UUID).  An hmap is not
> > a good hash table when duplicates are likely; instead, use an hindex.
> 
> I will switch this to an hindex in the next respin, but I'll ask, does the
> logic otherwise make sense?

I think so.  I thought that the UUID-based hash was sensible enough, I
just didn't have enough context to understand everything together.