| Message ID | 1453917089-30631-2-git-send-email-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Jan 27, 2016 at 09:51:20AM -0800, Ben Pfaff 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@ovn.org> This is now committed since I had posted it separately a bit earlier.
This seems the same that I already reviewed by itself: Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Jan 27, 2016, at 9:51 AM, Ben Pfaff <blp@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@ovn.org> > --- > 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@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
Yes, it's the same, sorry for posting it twice. On Thu, Jan 28, 2016 at 12:29:32PM -0800, Jarno Rajahalme wrote: > This seems the same that I already reviewed by itself: > > Acked-by: Jarno Rajahalme <jarno@ovn.org> > > > On Jan 27, 2016, at 9:51 AM, Ben Pfaff <blp@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@ovn.org> > > --- > > 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@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev >
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]
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@ovn.org> --- ofproto/ofproto-dpif-xlate.c | 24 +++++++++++++++++++++++- tests/ofproto-dpif.at | 24 ++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-)