[ovs-dev] [PATCH v2] ofproto-dpif-xlate: Do not execute resubmit again after recirculation.

[Ben Pfaff]

Thanks, I appreciate that.  I tried to untie this knot this morning and
ended up just getting more tangled.

I applied this to master and branch-2.5.

On Wed, Jan 27, 2016 at 04:16:00PM -0800, Jarno Rajahalme wrote:
> I’ll propose more patches to make recirculation handling in
> do_xlate_actions() more explicit.
> 
> Acked-by: Jarno Rajahalme <jarno at ovn.org>
> 
> > On Jan 27, 2016, at 9:16 AM, Ben Pfaff <blp at ovn.org> wrote:
> > 
> > Consider the following flow table:
> > 
> >    table=0 actions=resubmit(,1),2
> >    table=1 actions=debug_recirc
> > 
> > When debug_recirc triggers recirculation and we later resume processing,
> > only the output to port 2 should be executed, because the effects of
> > "resubmit" have already taken place.  However, until now, the "resubmit"
> > was added to the actions to execute post-recirculation, resulting in an
> > infinite loop.
> > 
> > Now consider this flow table (as seen in the "MPLS handling" test in
> > ofproto-dpif.at):
> > 
> >    table=0 actions=pop_mpls(0x0806),resubmit(,1)
> >    table=1 ip,nw_dst=1.2.3.4 actions=controller
> > 
> > Here, we do want to add the "resubmit" to the actions to execute
> > post-recirculation, since the "resubmit" cannot be processed until after
> > recirculation makes the nw_dst field available.
> > 
> > This commit fixes the problem in both cases.
> > 
> > Found when testing a feature based on recirculation.
> > 
> > Signed-off-by: Ben Pfaff <blp at ovn.org>
> > ---
> > v1->v2: Rewrite after realizing that I broke the "MPLS handling" test.
> > 
> > ofproto/ofproto-dpif-xlate.c | 24 +++++++++++++++++++++++-
> > tests/ofproto-dpif.at        | 24 ++++++++++++++++++++++++
> > 2 files changed, 47 insertions(+), 1 deletion(-)
> > 
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 4b25bf4..6e928da 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -4575,8 +4575,30 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
> >             break;
> > 
> >         case OFPACT_RESUBMIT:
> > +            /* Recirculation complicates resubmit.  There are two cases:
> > +             *
> > +             *     - If mpls_pop has been executed, then the flow table lookup
> > +             *       as part of resubmit might depend on fields that can only
> > +             *       be obtained via recirculation, so the resubmit itself
> > +             *       triggers recirculation and we need to make sure that the
> > +             *       resubmit is executed again after recirculation.
> > +             *       Therefore, in this case we trigger recirculation and let
> > +             *       the code following this "switch" append the resubmit to
> > +             *       the post-recirculation actions.
> > +             *
> > +             *     - Otherwise, some action in the flow entry found by resubmit
> > +             *       might trigger recirculation.  If that happens, then we do
> > +             *       not want to execute the resubmit again after
> > +             *       recirculation, so we want to skip back to the head of the
> > +             *       loop to avoid that, only adding any actions that follow
> > +             *       the resubmit to the post-recirculation actions.
> > +             */
> > +            if (ctx->was_mpls) {
> > +                ctx_trigger_recirculation(ctx);
> > +                break;
> > +            }
> >             xlate_ofpact_resubmit(ctx, ofpact_get_RESUBMIT(a));
> > -            break;
> > +            continue;
> > 
> >         case OFPACT_SET_TUNNEL:
> >             flow->tunnel.tun_id = htonll(ofpact_get_SET_TUNNEL(a)->tun_id);
> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> > index bfb1b56..aa2f1bb 100644
> > --- a/tests/ofproto-dpif.at
> > +++ b/tests/ofproto-dpif.at
> > @@ -4203,6 +4203,30 @@ AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: 3
> > OVS_VSWITCHD_STOP
> > AT_CLEANUP
> > 
> > +# This test verifies that "resubmit", when it triggers recirculation
> > +# indirectly through the flow that it recursively invokes, is not
> > +# re-executed when execution continues later post-recirculation.
> > +AT_SETUP([ofproto-dpif - recirculation after resubmit])
> > +OVS_VSWITCHD_START
> > +ADD_OF_PORTS([br0], [1], [2])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +table=0 in_port=1 actions=resubmit(,1),2
> > +table=1 in_port=1 actions=debug_recirc
> > +])
> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> > +
> > +flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow" -generate], [0], [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: recirc(0x1)
> > +])
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow,recirc_id(1)" -generate], [0], [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: 2
> > +])
> > +
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> > # Two testcases below are for the ofproto/trace command
> > # The first one tests all correct syntax:
> > # ofproto/trace [dp_name] odp_flow [-generate|packet]
> > -- 
> > 2.1.3
> > 
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>