On 27 Aug 2026, at 17:05, Mike Pattrick via dev wrote:
> 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]>
Thanks Mike for looking into this. I have some comments below.
//Eelco
> ---
> ofproto/ofproto-dpif-xlate.c | 25 +++++++++++++++++++++----
> tests/ofproto-dpif.at | 6 +++---
> tests/system-offloads-traffic.at | 6 +++---
> 3 files changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 764dbd662..410bcd7cd 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -6853,7 +6853,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 gt_offset_attr = nl_msg_start_nested(
Maybe offset_gt_attr? This reads easier to me.
> 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);
> @@ -6864,7 +6864,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, gt_offset_attr);
>
> xretain_base_flow_restore(ctx, retained_state);
> xretain_flow_restore(ctx, retained_state);
> @@ -6878,7 +6878,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx,
> bool old_exit = ctx->exit;
> ctx->exit = false;
>
> - offset_attr = nl_msg_start_nested(
> + size_t lte_offset_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);
> @@ -6889,9 +6889,26 @@ 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, lte_offset_attr);
> + size_t act_len = ctx->odp_actions->size - lte_offset_attr - NLA_HDRLEN;
lte_len?
> 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 (lte_offset_attr - gt_offset_attr - NLA_HDRLEN == act_len) {
Maybe to make it more clear what you're doing?
size_t gt_len = lte_offset_attr - gt_offset_attr - NLA_HDRLEN;
size_t lte_len = ctx->odp_actions->size - lte_offset_attr - NLA_HDRLEN;
if (gt_len == lte_len) {
> + if (memcmp(ofpbuf_at(ctx->odp_actions, gt_offset_attr + NLA_HDRLEN,
> + act_len),
> + ofpbuf_at(ctx->odp_actions, lte_offset_attr + NLA_HDRLEN,
> + act_len),
> + act_len) == 0) {
> + memmove((uint8_t *) ctx->odp_actions->data + offset,
> + (uint8_t *) ctx->odp_actions->data + gt_offset_attr
> + + NLA_HDRLEN,
> + act_len);
> + ofpbuf_truncate(ctx->odp_actions, act_len + offset);
> + }
> + }
> +
> ctx->was_mpls = old_was_mpls;
> ctx->conntracked = old_conntracked;
> ctx->exit = old_exit;
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index ee6ac873d..569900f8c 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -13568,7 +13568,7 @@ table=0,in_port=1
> actions=check_pkt_larger(200)->NXM_NX_REG0[[0]]
> 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: drop
> ])
>
> ovs-ofctl del-flows br0
> @@ -13614,7 +13614,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
> @@ -13655,7 +13655,7 @@ 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: 3,2,4
> ])
>
> OVS_VSWITCHD_STOP
> diff --git a/tests/system-offloads-traffic.at
> b/tests/system-offloads-traffic.at
> index a51bba5f9..05e8409f4 100644
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -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:output
> in_port(3),eth(),eth_type(0x0800),ipv4(frag=no), packets:19, bytes:11614,
> used:0.001s, actions:output
> ])
Guess the test needs to be rewritten, as it no longer does what it was
supposed to do, i.e., correlate counters from two different branches.
Same for the tests below we need to keep the branches.
> @@ -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([1,3])
>
>
> AT_CHECK([ovs-appctl revalidator/wait], [0])
> @@ -680,7 +680,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(drop),le(drop)),3])
> +OVS_CHECK_ACTIONS([3])
>
>
> AT_CHECK([ovs-appctl revalidator/wait], [0])
> --
> 2.55.0
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
