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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
