[ovs-dev] [PATCH 3/3] ofproto-dpif-xlate: Avoid using sample action when nesting level is low

[Andy Zhou]

On Fri, Mar 10, 2017 at 5:10 PM, Joe Stringer <joe at ovn.org> wrote:
> On 10 March 2017 at 16:51, Joe Stringer <joe at ovn.org> wrote:
>> On 9 March 2017 at 14:50, Andy Zhou <azhou at 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 <nusiddiq at redhat.com>
>>> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-March/329586.html
>>> Signed-off-by: Andy Zhou <azhou at 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 <joe at ovn.org>

Thanks for the review.  Push all patches in this series.