On Tue, Mar 18, 2014 at 04:58:29PM -0700, Andy Zhou wrote: > On Tue, Mar 18, 2014 at 4:25 PM, Ben Pfaff <b...@nicira.com> wrote: > > On Tue, Mar 11, 2014 at 04:56:21PM -0700, Andy Zhou wrote: > > > Infrastructure to enable megaflow support for bond ports using > > > recirculation. This patch adds the following features: > > > * Generate RECIRC action when bond can benefit from recirculation. > > > * Populate post recirculation rules in table 254. > > > * Uses post recirculation rules for bond rebalancing > > > * Logic to detect whether data path supports recirculation. > > > > > > Bond port using recirculation is currently turned off by always > > > detect the data path as not supporting recirculation. > > > > > > Signed-off-by: Andy Zhou <az...@nicira.com>
... > > In rule_dpif_lookup(), it's unsafe to use a pointer to 'recirc_flow' > > outside the block that declares it; it needs to be declared at > > function level. But I'm not sure I agree with what's going on there > > at a higher level, or maybe I just don't understand it. It looks to > > me like this always replaces the metadata by the recirc_id as part of > > the lookup. Why not just initialize the metadata from the recirc_id > > as part of receiving the packet, instead of doing it as part of the > > lookup? Not sure of the rationale, in any case. > > > I tried to keep flow as it was received from the datapath, so it won't be > confusing when we dump it out. Hmm, OK, but it seems like there's a trade-off between confusion there and confusion of "Why did this flow match that rule, there's no metadata in there?!" > > It makes me nervous to see bond.c retaining a pointer to the rules > > that it creates, especially when the table is no longer being marked > > read-only (so that "ovs-ofctl del-flows br0 table=254" could blow > > these flows away). Can we take a reference to the rules, or mark the > > table read-only again (and add some kind of override for use by > > internal flow_mods), or otherwise make this safer? > > > How about make it controller read-only? ovs-ofctl is a debugging > tool. I think that's my second suggestion, so yeah that's fine. > > Does update_recirc_rules() properly update rules following a > > rebalancing? It looks to me like add_pr_rule() never updates the > > output port if it finds a matching rule, so I'm not sure whether the > > new port assignments would properly take effect. > > > It seems there is still a bug there. Will debug it. I think add_pr_rule() just needs to update the other fields in the rule instead of just changing the op to ADD. > > I see dpif-netdev makes recirculation optional via ->enable_recirc, > > but that variable is initially true and I see no way to change it to > > false. Is that a feature for later? > > > I plan to add a unixctl command to change it so I can write test cases > against it. > I have been manually setting it during development. OK, any reason not to add it here? > > OFTABLE_READONLY is now unused. I don't know whether it is worth > > keeping it. Maybe it is useful to some hardware implementation, > > though. > > > Keep it and interpret it as controller read only? I like that solution, thanks! _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
