Current code tracks is_last_action through many translation paths, but not through the NORMAL pipeline. This results in unnecessary clone() actions emitted in the end of the pipeline. For example, this is happening for a common case of hash() + recirc() after a tunnel push. Can be avoided by propagating the flag along the way.
Mirrors are left as-is with the last action always treated as 'false' for now. The benefits for mirrors are not clear, as normally the output for them is a simple port and not something like a patch port that can recurse and produce all sorts of actions. Updating the code for mirrors to track the last action is also a bit cumbersome given the mirrors bitmask and the potential multiple outputs per mirror. Port addition in the patch port test is split into multiple transactions to ensure that flood order doesn't depend on the order of ports within the database update. Signed-off-by: Ilya Maximets <[email protected]> --- ofproto/ofproto-dpif-xlate.c | 57 +++++++++++++++++++++-------------- tests/ofproto-dpif.at | 51 +++++++++++++++++++++---------- tests/tunnel-push-pop-ipv6.at | 4 +-- tests/tunnel-push-pop.at | 4 +-- 4 files changed, 73 insertions(+), 43 deletions(-) diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 3fdd66216..116ef03ed 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -638,9 +638,10 @@ static void do_xlate_actions(const struct ofpact *, size_t ofpacts_len, struct xlate_ctx *, bool, bool); static void clone_xlate_actions(const struct ofpact *, size_t ofpacts_len, struct xlate_ctx *, bool, bool); -static void xlate_normal(struct xlate_ctx *); +static void xlate_normal(struct xlate_ctx *, bool is_last_action); static void xlate_normal_flood(struct xlate_ctx *ct, - struct xbundle *in_xbundle, struct xvlan *); + struct xbundle *in_xbundle, struct xvlan *, + bool is_last_action); static void xlate_table_action(struct xlate_ctx *, ofp_port_t in_port, uint8_t table_id, bool may_packet_in, bool honor_table_miss, bool with_ct_orig, @@ -661,7 +662,7 @@ static void xvlan_output_translate(const struct xbundle *, const struct xvlan *xvlan, struct xvlan *out); static void output_normal(struct xlate_ctx *, const struct xbundle *, - const struct xvlan *); + const struct xvlan *, bool is_last_action); /* Optional bond recirculation parameter to compose_output_action(). */ struct xlate_bond_recirc { @@ -2355,7 +2356,7 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle, struct xbundle *out_xbundle = xbundle_lookup(ctx->xcfg, mc.out_bundle); if (out_xbundle) { - output_normal(ctx, out_xbundle, &xvlan); + output_normal(ctx, out_xbundle, &xvlan, false); } } else if (xvlan.v[0].vid != mc.out_vlan && !eth_addr_is_reserved(ctx->xin->flow.dl_dst)) { @@ -2366,7 +2367,7 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle, LIST_FOR_EACH (xb, list_node, &xbridge->xbundles) { if (xbundle_includes_vlan(xb, &xvlan) && !xbundle_mirror_out(xbridge, xb)) { - output_normal(ctx, xb, &xvlan); + output_normal(ctx, xb, &xvlan, false); } } xvlan.v[0].vid = old_vid; @@ -2614,7 +2615,7 @@ check_and_set_cvlan_mask(struct flow_wildcards *wc, static void output_normal(struct xlate_ctx *ctx, const struct xbundle *out_xbundle, - const struct xvlan *xvlan) + const struct xvlan *xvlan, bool is_last_action) { uint16_t vid; union flow_vlan_hdr old_vlans[FLOW_MAX_VLAN_HEADERS]; @@ -2699,7 +2700,7 @@ output_normal(struct xlate_ctx *ctx, const struct xbundle *out_xbundle, xvlan_put(&ctx->xin->flow, &out_xvlan, out_xbundle->use_priority_tags); compose_output_action(ctx, xport->ofp_port, use_recirc ? &xr : NULL, - false, false); + is_last_action, false); memcpy(&ctx->xin->flow.vlans, &old_vlans, sizeof(old_vlans)); } @@ -3006,13 +3007,15 @@ mcast_output_add(struct mcast_output *out, struct xbundle *mcast_xbundle) * bundle 'in_xbundle' and the current 'xvlan'. */ static void mcast_output_finish(struct xlate_ctx *ctx, struct mcast_output *out, - struct xbundle *in_xbundle, struct xvlan *xvlan) + struct xbundle *in_xbundle, struct xvlan *xvlan, + bool is_last_action) { if (out->flood) { - xlate_normal_flood(ctx, in_xbundle, xvlan); + xlate_normal_flood(ctx, in_xbundle, xvlan, is_last_action); } else { for (size_t i = 0; i < out->n; i++) { - output_normal(ctx, out->xbundles[i], xvlan); + output_normal(ctx, out->xbundles[i], xvlan, + is_last_action && i == out->n - 1); } } @@ -3134,9 +3137,9 @@ xlate_normal_mcast_send_rports(struct xlate_ctx *ctx, static void xlate_normal_flood(struct xlate_ctx *ctx, struct xbundle *in_xbundle, - struct xvlan *xvlan) + struct xvlan *xvlan, bool is_last_action) { - struct xbundle *xbundle; + struct xbundle *xbundle, *last = NULL; LIST_FOR_EACH (xbundle, list_node, &ctx->xbridge->xbundles) { if (xbundle != in_xbundle @@ -3144,9 +3147,15 @@ xlate_normal_flood(struct xlate_ctx *ctx, struct xbundle *in_xbundle, && xbundle_includes_vlan(xbundle, xvlan) && xbundle->floodable && !xbundle_mirror_out(ctx->xbridge, xbundle)) { - output_normal(ctx, xbundle, xvlan); + if (last) { + output_normal(ctx, last, xvlan, false); + } + last = xbundle; } } + if (last) { + output_normal(ctx, last, xvlan, is_last_action); + } ctx->nf_output_iface = NF_OUT_FLOOD; } @@ -3165,7 +3174,7 @@ is_ip_local_multicast(const struct flow *flow, struct flow_wildcards *wc) } static void -xlate_normal(struct xlate_ctx *ctx) +xlate_normal(struct xlate_ctx *ctx, bool is_last_action) { struct flow_wildcards *wc = ctx->wc; struct flow *flow = &ctx->xin->flow; @@ -3290,10 +3299,11 @@ xlate_normal(struct xlate_ctx *ctx) xlate_normal_mcast_send_rports(ctx, ms, in_xbundle, &out); ovs_rwlock_unlock(&ms->rwlock); - mcast_output_finish(ctx, &out, in_xbundle, &xvlan); + mcast_output_finish(ctx, &out, in_xbundle, &xvlan, + is_last_action); } else { xlate_report(ctx, OFT_DETAIL, "multicast traffic, flooding"); - xlate_normal_flood(ctx, in_xbundle, &xvlan); + xlate_normal_flood(ctx, in_xbundle, &xvlan, is_last_action); } return; } else if (is_mld(flow, wc)) { @@ -3311,10 +3321,11 @@ xlate_normal(struct xlate_ctx *ctx) xlate_normal_mcast_send_rports(ctx, ms, in_xbundle, &out); ovs_rwlock_unlock(&ms->rwlock); - mcast_output_finish(ctx, &out, in_xbundle, &xvlan); + mcast_output_finish(ctx, &out, in_xbundle, &xvlan, + is_last_action); } else { xlate_report(ctx, OFT_DETAIL, "MLD query, flooding"); - xlate_normal_flood(ctx, in_xbundle, &xvlan); + xlate_normal_flood(ctx, in_xbundle, &xvlan, is_last_action); } return; } else { @@ -3324,7 +3335,7 @@ xlate_normal(struct xlate_ctx *ctx) * be forwarded on all ports */ xlate_report(ctx, OFT_DETAIL, "RFC4541: section 2.1.2, item 2, flooding"); - xlate_normal_flood(ctx, in_xbundle, &xvlan); + xlate_normal_flood(ctx, in_xbundle, &xvlan, is_last_action); return; } } @@ -3356,7 +3367,7 @@ xlate_normal(struct xlate_ctx *ctx) } ovs_rwlock_unlock(&ms->rwlock); - mcast_output_finish(ctx, &out, in_xbundle, &xvlan); + mcast_output_finish(ctx, &out, in_xbundle, &xvlan, is_last_action); } else { ovs_rwlock_rdlock(&ctx->xbridge->ml->rwlock); mac = mac_learning_lookup(ctx->xbridge->ml, flow->dl_dst, vlan); @@ -3376,7 +3387,7 @@ xlate_normal(struct xlate_ctx *ctx) && mac_xbundle != in_xbundle && mac_xbundle->ofbundle != in_xbundle->ofbundle) { xlate_report(ctx, OFT_DETAIL, "forwarding to learned port"); - output_normal(ctx, mac_xbundle, &xvlan); + output_normal(ctx, mac_xbundle, &xvlan, is_last_action); } else if (!mac_xbundle) { xlate_report(ctx, OFT_WARN, "learned port is unknown, dropping"); @@ -3387,7 +3398,7 @@ xlate_normal(struct xlate_ctx *ctx) } else { xlate_report(ctx, OFT_DETAIL, "no learned MAC for destination, flooding"); - xlate_normal_flood(ctx, in_xbundle, &xvlan); + xlate_normal_flood(ctx, in_xbundle, &xvlan, is_last_action); } } } @@ -5608,7 +5619,7 @@ xlate_output_action(struct xlate_ctx *ctx, ofp_port_t port, do_xlate_actions); break; case OFPP_NORMAL: - xlate_normal(ctx); + xlate_normal(ctx, is_last_action); break; case OFPP_FLOOD: flood_packets(ctx, false, is_last_action); diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index cb96972de..3fcc53381 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -10300,17 +10300,17 @@ AT_CLEANUP AT_SETUP([ofproto-dpif - patch ports - clone]) OVS_VSWITCHD_START( - [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 -- \ - add-port br0 p1 -- set Interface p1 type=patch \ - options:peer=p2 ofport_request=2 -- \ - add-port br0 p4 -- set Interface p4 type=dummy ofport_request=4 -- \ - add-br br1 -- \ + [add-br br1 -- \ set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \ fail-mode=secure -- \ - add-port br1 p2 -- set Interface p2 type=patch \ - options:peer=p1 -- \ - add-port br1 p3 -- set Interface p3 type=dummy ofport_request=3]) + add-port br0 pbr0 -- set Interface pbr0 type=patch \ + options:peer=pbr1 ofport_request=5 -- \ + add-port br1 pbr1 -- set Interface pbr1 type=patch \ + options:peer=pbr0 ofport_request=6]) + +add_of_ports br0 1 4 +add_of_ports br1 3 m4_define([TRACE_PKT], [m4_join([,], [in_port(1)], @@ -10330,10 +10330,10 @@ dnl Meter on peer bridge - wrapped. AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br1 \ 'meter=1 pktps stats bands=type=drop rate=2']) AT_DATA([flows-br0.txt], [dnl -in_port=p0,ip,actions=p1,output:p4 +in_port=p1,ip,actions=pbr0,output:p4 ]) AT_DATA([flows-br1.txt], [dnl -in_port=p2,ip,actions=meter:1,output:p3 +in_port=pbr1,ip,actions=meter:1,output:p3 ]) CHECK_CLONE_ACTION([flows-br0.txt], [flows-br1.txt], [TRACE_PKT], [dnl Datapath actions: clone(meter(0),3),4 @@ -10342,11 +10342,11 @@ Datapath actions: clone(meter(0),3),4 dnl CT on peer bridge as last action - not wrapped (trailing clone unwrapped). AT_DATA([flows-br0.txt], [dnl priority=10,ip,action=push:NXM_OF_IN_PORT[],resubmit(,65),pop:NXM_OF_IN_PORT[] -table=65,priority=10,ip,in_port=p0,action=p1 +table=65,priority=10,ip,in_port=p1,action=pbr0 ]) AT_DATA([flows-br1.txt], [dnl -priority=100,in_port=p2,ip,ct_state=-trk,action=ct(table=0,zone=1) -priority=100,in_port=p2,ip,ct_state=+trk+est,ct_zone=1,action=p3 +priority=100,in_port=pbr1,ip,ct_state=-trk,action=ct(table=0,zone=1) +priority=100,in_port=pbr1,ip,ct_state=+trk+est,ct_zone=1,action=p3 ]) CHECK_CLONE_ACTION([flows-br0.txt], [flows-br1.txt], [TRACE_PKT], [dnl Datapath actions: ct(zone=1),recirc(X) @@ -10355,10 +10355,10 @@ Datapath actions: 3 dnl Non-reversible through resubmit on peer - wrapped. AT_DATA([flows-br0.txt], [dnl -in_port=p0,ip,actions=p1,output:p4 +in_port=p1,ip,actions=pbr0,output:p4 ]) AT_DATA([flows-br1.txt], [dnl -in_port=p2,ip,actions=resubmit(,1) +in_port=pbr1,ip,actions=resubmit(,1) table=1,ip,actions=ct(commit),output:p3 ]) CHECK_CLONE_ACTION([flows-br0.txt], [flows-br1.txt], [TRACE_PKT], [dnl @@ -10367,12 +10367,31 @@ Datapath actions: clone(ct(commit),3),4 dnl Only reversible actions on peer - not wrapped. AT_DATA([flows-br1.txt], [dnl -in_port=p2,ip,actions=set_field:192.168.1.1->ip_dst,output:p3 +in_port=pbr1,ip,actions=set_field:192.168.1.1->ip_dst,output:p3 ]) CHECK_CLONE_ACTION([flows-br0.txt], [flows-br1.txt], [TRACE_PKT], [dnl Datapath actions: set(ipv4(dst=192.168.1.1)),3,set(ipv4(dst=10.10.10.1)),4 ]) +dnl NORMAL with ct on peer, more actions after patch port - wrapped. +AT_DATA([flows-br0.txt], [dnl +in_port=p1,ip,actions=pbr0,output:p4 +]) +AT_DATA([flows-br1.txt], [dnl +in_port=pbr1,ip,actions=ct(commit),normal +]) +CHECK_CLONE_ACTION([flows-br0.txt], [flows-br1.txt], [TRACE_PKT], [dnl +Datapath actions: clone(ct(commit),101,3),4 +]) + +dnl NORMAL with ct on peer, patch port is the last action - not wrapped. +AT_DATA([flows-br0.txt], [dnl +in_port=p1,ip,actions=pbr0 +]) +CHECK_CLONE_ACTION([flows-br0.txt], [flows-br1.txt], [TRACE_PKT], [dnl +Datapath actions: ct(commit),101,3 +]) + OVS_VSWITCHD_STOP AT_CLEANUP diff --git a/tests/tunnel-push-pop-ipv6.at b/tests/tunnel-push-pop-ipv6.at index 80c080710..df626e0a3 100644 --- a/tests/tunnel-push-pop-ipv6.at +++ b/tests/tunnel-push-pop-ipv6.at @@ -847,7 +847,7 @@ Datapath actions: tnl_push(tnl_port(6081),header(size=70,type=5,dnl eth(dst=f8:bc:12:44:34:b6,src=aa:55:aa:55:00:00,dl_type=0x86dd),dnl ipv6(src=2001:cafe::88,dst=2001:cafe::92,label=0,proto=17,tclass=0x0,hlimit=64),dnl udp(src=0,dst=6081,csum=0xffff),geneve(vni=0x7b)),out_port(100)),dnl -clone(hash(l4(0)),recirc(0x1)) +hash(l4(0)),recirc(0x1) ]) dnl Now check that the packet is actually encapsulated and delivered. @@ -879,7 +879,7 @@ actions:tnl_push(tnl_port(6081),header(size=70,type=5,dnl eth(dst=f8:bc:12:44:34:b6,src=aa:55:aa:55:00:00,dl_type=0x86dd),dnl ipv6(src=2001:cafe::88,dst=2001:cafe::92,label=0,proto=17,tclass=0x0,hlimit=64),dnl udp(src=0,dst=6081,csum=0xffff),geneve(vni=0x7b)),out_port(100)),dnl -clone(hash(l4(0)),recirc(0x2)) +hash(l4(0)),recirc(0x2) ]) OVS_VSWITCHD_STOP diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at index 3f709d2a1..50306ceff 100644 --- a/tests/tunnel-push-pop.at +++ b/tests/tunnel-push-pop.at @@ -1373,7 +1373,7 @@ Datapath actions: tnl_push(tnl_port(6081),header(size=50,type=5,dnl eth(dst=f8:bc:12:44:34:b6,src=aa:55:aa:55:00:00,dl_type=0x0800),dnl ipv4(src=1.1.2.88,dst=1.1.2.92,proto=17,tos=0,ttl=64,frag=0x4000),dnl udp(src=0,dst=6081,csum=0x0),geneve(vni=0x7b)),out_port(100)),dnl -clone(hash(l4(0)),recirc(0x1)) +hash(l4(0)),recirc(0x1) ]) dnl Now check that the packet is actually encapsulated and delivered. @@ -1406,7 +1406,7 @@ actions:tnl_push(tnl_port(6081),header(size=50,type=5,dnl eth(dst=f8:bc:12:44:34:b6,src=aa:55:aa:55:00:00,dl_type=0x0800),dnl ipv4(src=1.1.2.88,dst=1.1.2.92,proto=17,tos=0,ttl=64,frag=0x4000),dnl udp(src=0,dst=6081,csum=0x0),geneve(vni=0x7b)),out_port(100)),dnl -clone(hash(l4(0)),recirc(0x2)) +hash(l4(0)),recirc(0x2) ]) OVS_VSWITCHD_STOP -- 2.54.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
