On 21 Sep 2026, at 18:01, 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.
Thanks Mike for following up on this patch. I have one nit/style below,
please take a look and let me know what you think. I can apply it
at merge time.
//Eelco
> Signed-off-by: Mike Pattrick <[email protected]>
> ---
> v2:
> - Renamed variables
> - Changed redundant legs in check_pkt_len tests
> ---
> ofproto/ofproto-dpif-xlate.c | 26 ++++++++++++++++++++++----
> tests/ofproto-dpif.at | 18 +++++++++++++-----
> tests/system-offloads-traffic.at | 11 ++++++-----
> 3 files changed, 41 insertions(+), 14 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 4e7d6fb40..38da914e8 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -6872,7 +6872,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);
> @@ -6883,7 +6883,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);
> @@ -6897,7 +6897,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 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);
> @@ -6908,9 +6908,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);
> + size_t lte_len = ctx->odp_actions->size - offset_lte_attr - NLA_HDRLEN;
> + size_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) {
> + if (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) {
What about (saves one indent level):
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,
> + lte_len);
nit: gt_len here, as this is what we copy from.
> + ofpbuf_truncate(ctx->odp_actions, lte_len + offset);
> + }
> + }
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
