> 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. > >> While investigating these issues, I noticed some significant >> differences between the way flow and other context information >> is saved and restored when using openflow patch ports, versus >> the way it is done with clone. At least one such difference, with >> regard to conntrack, is causing failures. > > What's the difference that is causing failures? I do not know about it, > so it is a high priority to fix it. Is it already fixed, or do we need > to do something about it? > >> I would classify the differences (those for which there is no current >> workaround by specifying nested actions within the clone) into two >> categories: >> 1. State that is kept outside of flow, which is currently saved, >> cleared, and restored when using openflow patch ports. >> This includes: >> ctx->wc->masks.tunnel > > I think that clone should not save and restore this. The situation is > different from a patch port because clone does not change the ingress > port. The patch port code has the following comment: > > /* Since this packet came in on a patch port (from the perspective of > * the peer bridge), it cannot have useful tunnel information. As a > * result, any wildcards generated on that tunnel also cannot be valid. > * The tunnel wildcards must be restored to their original version > since > * the peer bridge uses a separate tunnel metadata table and therefore > * any generated wildcards will be garbage in the context of our > * metadata table. */ > ctx->wc->masks.tunnel = old_flow_tnl_wc; > >> ctx->conntracked > > 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. > >> 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. It would be good to add these changes to the documentation as well. > >> ctx->xbridge (not an issue if bridge does not change) >> ctx->mirrors (not an issue if bridge does not change) > > clone doesn't change the bridge, so these shouldn't matter. > >> 2. State that resides inside the flow, but for which there is no >> workaround to clear or reset the fields. This includes: >> flow->tunnel ? > > Like the tunnel mask, this doesn't matter for clone, because clone > doesn't change the ingress port. > >> flow->tunnel.metadata.tab (not an issue if bridge does not change) > > clone doesn't change the bridge, so this shouldn't matter. > >> flow->ct_state (read only) >> flow->ct_mark (restricted so can only be set in ct action) >> flow->ct_label (restricted so can only be set in ct action) > > It's not obvious to me that clone should always clear these. However, > if it saves and restores them, then we can support clearing them by > adding a ct_clear action. I've written up a patch. > > I sent out a series that addresses these issues. Probably, we also need > to modify OVN to insert a "ct_clear" action into its use of "clone", but > my series doesn't do that. > > The series starts here: > https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/327204.html > _______________________________________________ > 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
