Currently it's possible for both legs of a check_pkt_len to result in the same actions. In the most extreme example of this, multiple check_pkt_len actions could be chained together, all resulting in a drop. Packet's transiting a check_pkt_len action can get cloned even if the legs don't need, resulting in potentially unneeded memory activity.
This patch checks if both legs of the action are identical, and replaces the entire action with one of the legs if they are. Signed-off-by: Mike Pattrick <[email protected]> --- ofproto/ofproto-dpif-xlate.c | 48 ++++++++++++++++++++++---------- tests/ofproto-dpif.at | 18 ++++++++---- tests/system-offloads-traffic.at | 11 ++++---- 3 files changed, 52 insertions(+), 25 deletions(-) diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 509f539fb..7077b2d05 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -6855,7 +6855,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, OVS_ACTION_ATTR_CHECK_PKT_LEN); nl_msg_put_u16(ctx->odp_actions, OVS_CHECK_PKT_LEN_ATTR_PKT_LEN, check_pkt_larger->pkt_len); - size_t offset_attr = nl_msg_start_nested( + size_t offset_gt_attr = nl_msg_start_nested( ctx->odp_actions, OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER); value->u8_val = 1; mf_write_subfield_flow(&check_pkt_larger->dst, value, &ctx->xin->flow); @@ -6866,7 +6866,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, if (ctx->freezing) { finish_freezing(ctx); } - nl_msg_end_nested(ctx->odp_actions, offset_attr); + nl_msg_end_nested(ctx->odp_actions, offset_gt_attr); xretain_base_flow_restore(ctx, retained_state); xretain_flow_restore(ctx, retained_state); @@ -6880,7 +6880,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, bool old_exit = ctx->exit; ctx->exit = false; - offset_attr = nl_msg_start_nested( + uint32_t offset_lte_attr = nl_msg_start_nested( ctx->odp_actions, OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL); value->u8_val = 0; mf_write_subfield_flow(&check_pkt_larger->dst, value, &ctx->xin->flow); @@ -6891,9 +6891,27 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, if (ctx->freezing) { finish_freezing(ctx); } - nl_msg_end_nested(ctx->odp_actions, offset_attr); + nl_msg_end_nested(ctx->odp_actions, offset_lte_attr); + uint32_t lte_len = ctx->odp_actions->size - offset_lte_attr - NLA_HDRLEN; + uint32_t gt_len = offset_lte_attr - offset_gt_attr - NLA_HDRLEN; nl_msg_end_nested(ctx->odp_actions, offset); + /* If the two legs are the identical length and content, replace this + * check_pkt_len action with one of the legs. */ + if (gt_len == lte_len + && memcmp(ofpbuf_at(ctx->odp_actions, offset_gt_attr + NLA_HDRLEN, + gt_len), + ofpbuf_at(ctx->odp_actions, offset_lte_attr + NLA_HDRLEN, + lte_len), + lte_len) == 0) { + memmove((uint8_t *) ctx->odp_actions->data + offset, + (uint8_t *) ctx->odp_actions->data + offset_gt_attr + + NLA_HDRLEN, + gt_len); + + ofpbuf_truncate(ctx->odp_actions, lte_len + offset); + } + ctx->was_mpls = old_was_mpls; ctx->conntracked = old_conntracked; ctx->exit = old_exit; @@ -8231,20 +8249,24 @@ act_is_observe(struct nlattr *action) size_t userdata_len; switch (nl_attr_type(action)) { + case OVS_ACTION_ATTR_METER: + case OVS_ACTION_ATTR_SAMPLE: + case OVS_ACTION_ATTR_PSAMPLE: + return true; case OVS_ACTION_ATTR_USERSPACE: { if (!nl_parse_nested(action, ovs_userspace_policy, attr, ARRAY_SIZE(attr))) { - break; + return false; } userdata_attr = attr[OVS_USERSPACE_ATTR_USERDATA]; if (!userdata_attr) { - break; + return false; } userdata_len = nl_attr_get_size(userdata_attr); if (userdata_len != sizeof *cookie) { - break; + return false; } cookie = nl_attr_get(userdata_attr); @@ -8253,17 +8275,13 @@ act_is_observe(struct nlattr *action) case USER_ACTION_COOKIE_FLOW_SAMPLE: case USER_ACTION_COOKIE_IPFIX: return true; + default: + return false; } - - return false; } - case OVS_ACTION_ATTR_METER: - case OVS_ACTION_ATTR_SAMPLE: - case OVS_ACTION_ATTR_PSAMPLE: - return true; + default: + return false; } - - return false; } /* This will tweak the odp actions generated. For now, it will: diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index efb36e058..6fc0b10d6 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -13597,13 +13597,21 @@ Datapath actions: check_pkt_len(size=200,gt(2),le(drop)) ovs-ofctl del-flows br0 AT_DATA([flows.txt], [dnl -table=0,in_port=1 actions=check_pkt_larger(200)->NXM_NX_REG0[[0]] +table=0,in_port=1 actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,1) +table=1,icmp,reg0=0x1/0x1 actions=4 ]) AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt]) AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout]) AT_CHECK([tail -1 stdout], [0], [dnl -Datapath actions: check_pkt_len(size=200,gt(drop),le(drop)) +Datapath actions: check_pkt_len(size=200,gt(4),le(drop)) +]) + +dnl chk_pkt_len optimization. +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt]) +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=17,tos=1,ttl=128,frag=no),udp()'], [0], [stdout]) +AT_CHECK([tail -1 stdout], [0], [dnl +Datapath actions: drop ]) ovs-ofctl del-flows br0 @@ -13649,7 +13657,7 @@ ovs-ofctl dump-flows br0 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout]) AT_CHECK([tail -1 stdout], [0], [dnl -Datapath actions: check_pkt_len(size=200,gt(set(ipv4(src=192.168.3.3)),check_pkt_len(size=200,gt(3),le(3))),le(set(ipv4(src=192.168.3.4)),check_pkt_len(size=200,gt(4),le(4)))) +Datapath actions: check_pkt_len(size=200,gt(set(ipv4(src=192.168.3.3)),3),le(set(ipv4(src=192.168.3.4)),4)) ]) ovs-ofctl del-flows br0 @@ -13683,14 +13691,14 @@ table=0,in_port=1 actions=load:0x1->NXM_NX_REG1[[]],resubmit(,1),load:0x2->NXM_N table=1,in_port=1,reg1=0x1 actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,4) table=1,in_port=1,reg1=0x2 actions=output:2 table=1,in_port=1,reg1=0x3 actions=output:4 -table=4,in_port=1 actions=output:3 +table=4,in_port=1,reg0=0x1 actions=output:3 ]) AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt]) AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout]) AT_CHECK([cat stdout | grep Datapath -B1], [0], [dnl Megaflow: recirc_id=0,eth,ip,in_port=1,nw_frag=no -Datapath actions: check_pkt_len(size=200,gt(3),le(3)),2,4 +Datapath actions: check_pkt_len(size=200,gt(3),le(drop)),2,4 ]) OVS_VSWITCHD_STOP diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at index cfcc5bd6c..c75b056d8 100644 --- a/tests/system-offloads-traffic.at +++ b/tests/system-offloads-traffic.at @@ -449,7 +449,7 @@ table=0,in_port=2 actions=output:1 table=0,in_port=1 actions=load:0x1->NXM_NX_REG1[[]],resubmit(,1),load:0x2->NXM_NX_REG1[[]],resubmit(,1) table=1,in_port=1,reg1=0x1 actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,4) table=4,in_port=1,reg0=0x1 actions=output:2 -table=4,in_port=1,reg0=0x0 actions=output:2 +table=4,in_port=1,reg0=0x0 actions=output:2,3 ]) AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt]) @@ -463,7 +463,7 @@ NS_CHECK_EXEC([at_ns1], [ping -q -c 10 -i 0.1 -W 2 -s 1024 10.1.1.2 | FORMAT_PIN ], [], [ovs-appctl dpctl/dump-flows; ovs-ofctl dump-flows br0]) AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | DUMP_CLEAN_SORTED | sed 's/bytes:11348/bytes:11614/'], [0], [dnl -in_port(2),eth(),eth_type(0x0800),ipv4(frag=no), packets:19, bytes:11614, used:0.001s, actions:check_pkt_len(size=200,gt(3),le(3)) +in_port(2),eth(),eth_type(0x0800),ipv4(frag=no), packets:19, bytes:11614, used:0.001s, actions:check_pkt_len(size=200,gt(3),le(3,4)) in_port(3),eth(),eth_type(0x0800),ipv4(frag=no), packets:19, bytes:11614, used:0.001s, actions:output ]) @@ -621,7 +621,7 @@ table=0,in_port=2 actions=output:1 table=0,in_port=1 actions=load:0x1->NXM_NX_REG1[[]],resubmit(,1),load:0x2->NXM_NX_REG1[[]],resubmit(,1) table=1,in_port=1,reg1=0x1 actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,4) table=1,in_port=1,reg1=0x2 actions=output:2 -table=4,in_port=1,reg0=0x1 actions=output:br0 +table=4,in_port=1,reg0=0x1 actions=output:br0,3 table=4,in_port=1,reg0=0x0 actions=output:br0 ]) AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt]) @@ -629,7 +629,7 @@ sleep 1 NS_CHECK_EXEC([at_ns1], [ping -q -c 10 -i 0.1 -W 2 -s 64 10.1.1.2 | FORMAT_PING], [0], [dnl 10 packets transmitted, 10 received, 0% packet loss, time 0ms ], [], [ovs-appctl dpctl/dump-flows; ovs-ofctl dump-flows br0]) -OVS_CHECK_ACTIONS([check_pkt_len(size=200,gt(1),le(1)),3]) +OVS_CHECK_ACTIONS([check_pkt_len(size=200,gt(1,4),le(1)),3]) AT_CHECK([ovs-appctl revalidator/wait], [0]) @@ -674,13 +674,14 @@ table=0,in_port=2 actions=output:1 table=0,in_port=1 actions=load:0x1->NXM_NX_REG1[[]],resubmit(,1),load:0x2->NXM_NX_REG1[[]],resubmit(,1) table=1,in_port=1,reg1=0x1 actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,4) table=1,in_port=1,reg1=0x2 actions=output:2 +table=4,in_port=1,reg0=0x1 actions=output:br0 ]) AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt]) sleep 1 NS_CHECK_EXEC([at_ns1], [ping -q -c 10 -i 0.1 -W 2 -s 64 10.1.1.2 | FORMAT_PING], [0], [dnl 10 packets transmitted, 10 received, 0% packet loss, time 0ms ], [], [ovs-appctl dpctl/dump-flows; ovs-ofctl dump-flows br0]) -OVS_CHECK_ACTIONS([check_pkt_len(size=200,gt(drop),le(drop)),3]) +OVS_CHECK_ACTIONS([check_pkt_len(size=200,gt(1),le(drop)),3]) AT_CHECK([ovs-appctl revalidator/wait], [0]) -- 2.55.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
