On Fri, Jan 6, 2017 at 9:52 AM, Ben Pfaff <b...@ovn.org
<mailto: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
<mailto: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
<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 <mailto:d...@openvswitch.org>
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
<https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
