On Fri, Mar 10, 2017 at 5:10 PM, Joe Stringer <j...@ovn.org> wrote: > On 10 March 2017 at 16:51, Joe Stringer <j...@ovn.org> wrote: >> On 9 March 2017 at 14:50, Andy Zhou <az...@ovn.org> wrote: >>> When datapath sample action only allow a small number of nested actions >>> (i.e. less than 3), do not translate the OpenFlow's 'clone' action >>> into datapath 'sample' action, since such translation would cause >>> datapath to reject the flow, with 'EOVERFLOW', when OVS is used to >>> implement the OVN pipeline, or more generally, when deeper nested >>> clone are expected. >>> >>> Reported-by: Numan Siddique <nusid...@redhat.com> >>> Reported-at: >>> https://mail.openvswitch.org/pipermail/ovs-dev/2017-March/329586.html >>> Signed-off-by: Andy Zhou <az...@ovn.org> >>> --- >>> ofproto/ofproto-dpif-xlate.c | 13 ++++++++++--- >>> 1 file changed, 10 insertions(+), 3 deletions(-) >>> >>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >>> index 1998521..090e8d6 100644 >>> --- a/ofproto/ofproto-dpif-xlate.c >>> +++ b/ofproto/ofproto-dpif-xlate.c >>> @@ -4794,13 +4794,20 @@ xlate_clone(struct xlate_ctx *ctx, const struct >>> ofpact_nest *oc) >>> /* Datapath clone action will make sure the pre clone packets >>> * are used for actions after clone. Save and restore >>> * ctx->base_flow to reflect this for the openflow pipeline. */ >>> - struct flow old_base_flow = ctx->base_flow; >>> if (ctx->xbridge->support.clone) { >>> + struct flow old_base_flow = ctx->base_flow; >>> compose_clone_action(ctx, oc); >>> - } else { >>> + ctx->base_flow = old_base_flow; >>> + } else if (ctx->xbridge->support.sample_nesting > 3) { >>> + /* Avoid generate sample action if datapath >>> + * only allow small number of nesting. Deeper nesting >>> + * can cause the datapath to reject the generated flow. */ >>> + struct flow old_base_flow = ctx->base_flow; >>> compose_clone_action_using_sample(ctx, oc); >>> + ctx->base_flow = old_base_flow; >>> + } else { >>> + do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx); >>> } >>> - ctx->base_flow = old_base_flow; >> >> I think that we should always store the old base flow before and >> restore afterwards, regardless of how the clone gets actually >> implemented. > > Never mind, the do_xlate_actions() wasn't wrapped in the baseflow > save/restore before commit 456024cb1c5e ("xlate: Translate openflow > clone into odp sample action."). > > Basically when we serialize clone using the `A,clone(x,y,z),B --> > A,x,y,z,z',y',x',B` method of putting actions then putting the > inverse, the changes must only be composed down to the datapath > actions list after x,y,z, then so long as z',y',x' changes are applied > back to the flow, then they will be correct the next time that the > actions are composed down to the datapath actions list. Thanks for the > explanation. > > LGTM, > > Acked-by: Joe Stringer <j...@ovn.org>
Thanks for the review. Push all patches in this series. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev