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