On Fri, Jan 6, 2017 at 9:52 AM, Ben Pfaff <b...@ovn.org> wrote: > On Thu, Jan 05, 2017 at 05:54:46PM -0800, Jarno Rajahalme wrote: > > > > > On Jan 5, 2017, at 4:28 PM, Ben Pfaff <b...@ovn.org> wrote: > > > > > > On Tue, Jan 03, 2017 at 02:55:19AM -0800, Mickey Spiegel wrote: > > >> One of the motivations for clone is to use it as a lightweight way to > > >> resubmit to an earlier table at the beginning of the pipeline, without > > >> incurring all of the overhead associated with openflow patch ports. > > >> One such usage is in OVN, where a recent patch set replaced the > > >> use of openflow patch ports with clone, for OVN patch ports within > > >> the same bridge (br-int). > > >> > > >> Over the holidays, some issues arose related to this usage of clone > > >> (see the thread ovn ping from VM to external gateway IP failed). > > > > > > Thanks for bringing this up. I had overlooked these questions and this > > > issue. > > > > > > I guess that we should save and restore this since we're saving and > > > restoring the conntrack metadata. I've written up a patch. > > > > > >> ctx->was_mpls > > > > > > I do not think that that this is a correctness issue, but it's a nice > > > optimization to save and restore this. I've written up a patch. > > > > I think the rationale is that ‘was_mpls’ is used to track the validity > > of the flow key across an MPLS POP operation. Since the flow key is > > saved and restored, we should save and restore ‘was_mpls’ as well. > > OK. I did send a patch that adds that. Do you want to make a > suggestion for the commit message? I may not fully understand the issue > yet, so I don't think the commit message is very good. > > Here's the patch: > https://mail.openvswitch.org/pipermail/ovs-dev/2017- > January/327207.html > > > >> ctx->xin->tables_version (not an issue if bridge does not change) > > > > > > clone doesn't change the bridge, so this shouldn't matter. > > > > > >> ctx->stack > > >> ctx->action_set > > > > > > I think it's cleanest if a clone starts off with both of these empty > and > > > saves and restores them. I've written up a patch. > > > > I think saving and restoring is needed, but I’m not so sure of > > clearing these. However, it seems that there is no way for the action > > set to be executed within the clone, so I guess it does not matter. > > I guess that it would also be a clean design, and consistent with my new > ct_clear action, to not clear them but instead to start from a copy and > allow for clearing them explicitly within the clone. > > There is already an instruction to clear the action set, so we wouldn't > need to add anything. I think that the action set can only affect what > happens inside the clone in terms of matches or actions based on the > actset_output field, though. > > I'm not sure of the value of an action to clear the stack, so I'd be > inclined to hold off on that until we think of one. > > I'll revise my patch to work this way. > > > It would be good to add these changes to the documentation as well. > > My patch does update the documentation on this point. >
Thanks Ben for all the fixes. We are in middle of performance testing with the version of ovn-controller which creates patch ports for router ports. Once this is done, we will be able to test with the patches you have proposed. Dong Jun- May be if you want to test these patches and I see if it resolves the issues which you had posted. Thanks Numan _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
