Ben Pfaff <b...@ovn.org> wrote on 06/06/2016 10:50:43 AM:
> From: Ben Pfaff <b...@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 06/06/2016 10:50 AM > Subject: Re: [ovs-dev,v17,4/5] Persist ovn flow tables. > > On Mon, Jun 06, 2016 at 10:25:32AM -0500, Ryan Moats wrote: > > Ben Pfaff <b...@ovn.org> wrote on 06/03/2016 10:50:45 AM: > > > > > From: Ben Pfaff <b...@ovn.org> > > > To: Ryan Moats/Omaha/IBM@IBMUS > > > Cc: dev@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 <rmo...@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. Will do on documenting is_new both in the code base and in the commit message Ryan _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
