On 2/1/26 12:52 AM, Jun Gu wrote:
> In the current implementation, mirror_packet() is invoked at the end of
> compose_output_action__(). This logic is flawed because it triggers
> mirroring even when the packet is not actually being output to a port,
> but is instead being recirculated (e.g., for bond member selection).
>
> This causes two major issues:
>
> 1. Redundant Mirrors: If a packet is mirrored during the recirculation
> phase and then mirrored again when it finally hits an actual output
> action, the mirror destination receives duplicate traffic.
>
> 2. False Mirroring on Dropped Packets: Mirroring should only occur on
> actual output. If for some reason the post-recurculation rules are
> not available, we should not be mirroring the packet. For example,
> if a bond is removed while we have the first datapath flow installed,
> but the packet after recirculation goes to userspace via MISS upcall
> and gets dropped, then we should not mirror that packet.
>
> This patch refactors compose_output_action__() to ensure mirror_packet()
> is only called within the actual output.
>
> Signed-off-by: Jun Gu <[email protected]>
> ---
Hi. Thanks for the new version! It mostly looks good to me.
See a few small comments below.
Best regards, Ilya Maximets.
> ofproto/ofproto-dpif-xlate.c | 21 ++++++++++++++-----
> tests/ofproto-dpif.at | 40 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 56 insertions(+), 5 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 32ee49e7e..335a7e68f 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4560,6 +4560,9 @@ compose_output_action__(struct xlate_ctx *ctx,
> ofp_port_t ofp_port,
> /* Commit accumulated flow updates before output. */
> xlate_commit_actions(ctx);
>
> + /* Flag to decide whether the output should be mirrored. */
> + bool mirrored = true;
The name is in the past tense, so it suggests that the packet was already
mirrored, which is not the case. Something like 'skip_mirrors' might be
better with a default value of 'false'. The comment for it here is also
not necessary, as it will be self-explanatory. And we'll need to move
the declaration to the top of the function, see the comment below.
> +
> if (xr && bond_use_lb_output_action(xport->xbundle->bond)) {
> /*
> * If bond mode is balance-tcp and optimize balance tcp is
> enabled
> @@ -4589,6 +4592,9 @@ compose_output_action__(struct xlate_ctx *ctx,
> ofp_port_t ofp_port,
> /* Recirc action. */
> nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_RECIRC,
> xr->recirc_id);
> +
> + /* Avoid mirroring on recirculation. */
> + mirrored = false;
> } else if (is_native_tunnel) {
> /* Output to native tunnel port. */
> native_tunnel_output(ctx, xport, flow, odp_port, truncate,
> @@ -4629,12 +4635,17 @@ compose_output_action__(struct xlate_ctx *ctx,
> ofp_port_t ofp_port,
> ctx->sflow_odp_port = odp_port;
> ctx->sflow_n_outputs++;
> ctx->nf_output_iface = ofp_port;
> - }
>
> - if (mbridge_has_mirrors(ctx->xbridge->mbridge) && xport->xbundle) {
> - mirror_packet(ctx, xport->xbundle,
> - xbundle_mirror_dst(xport->xbundle->xbridge,
> - xport->xbundle));
> + /* Mirror only when we actually perform an output action.
> + * Recirculation does not send the packet anywhere, so it
> + * should not trigger mirroring.
> + */
This comment doesn't make a lot of sense here. It's talking about
recirculation, but at this point in code we're far from the recirculation
handling. We don't a comment here, but part of it can be moved to the
place where the boolean value is set.
> + if (mirrored && mbridge_has_mirrors(ctx->xbridge->mbridge)
> + && xport->xbundle) {
> + mirror_packet(ctx, xport->xbundle,
> + xbundle_mirror_dst(xport->xbundle->xbridge,
> + xport->xbundle));
> + }
I'd suggest we keep this outside of the 'if (out_port != ODPP_NONE)'
check. The reason is that some virtual ports may not have an actual
datapath port number, but we may still want to mirror when traffic
goes through them. I don't think we have any such port today beside
the patch port, but it's better if we keep the logic more generic,
even if it's not a possible condition right now.
> }
>
> out:
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index b791bf4e4..86ade6066 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -6112,6 +6112,46 @@ AT_CHECK([tail -1 stdout | grep -E
> "trunc\(200\),2,trunc\(300\),3,100|trunc\(300
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([ofproto-dpif - mirroring on balance-tcp bonding with dp_hash])
> +AT_KEYWORDS([mirror mirrors mirroring])
> +OVS_VSWITCHD_START(
> + [add-bond br0 bond0 p1 p2 bond_mode=balance-tcp lacp=active \
> + other-config:lacp-time=fast other-config:bond-rebalance-interval=0 --
> \
> + set interface p1 type=dummy options:pstream=punix:$OVS_RUNDIR/p1.sock
> ofport_request=1 -- \
> + set interface p2 type=dummy options:pstream=punix:$OVS_RUNDIR/p2.sock
> ofport_request=2 -- \
> + 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=standalone -- \
> + add-bond br1 bond1 p3 p4 bond_mode=balance-tcp lacp=active \
> + other-config:lacp-time=fast other-config:bond-rebalance-interval=0 --
> \
> + set interface p3 type=dummy options:stream=unix:$OVS_RUNDIR/p1.sock
> ofport_request=3 -- \
> + set interface p4 type=dummy options:stream=unix:$OVS_RUNDIR/p2.sock
> ofport_request=4 -- \
> + add-port br1 p5 -- set interface p5 ofport_request=5 type=dummy
nit: Addition of the p5 can be moved below to the add_of_ports, i.e.
add_of_ports br1 5 6
> +])
> +AT_CHECK([ovs-appctl netdev-dummy/set-admin-state up], 0, [OK
> +])
nit: This call is not needed, ports are UP by default.
> +
> +# Waits for all ifaces enabled.
> +OVS_WAIT_UNTIL([test `ovs-appctl bond/show | grep -c "may_enable: true"` -ge
> 4])
> +
> +add_of_ports br1 6
> +AT_CHECK([ovs-vsctl \
> + set Bridge br1 mirrors=@m --\
> + --id=@bond1 get Port bond1 -- --id=@p6 get Port p6 --\
> + --id=@m create Mirror name=mymirror select_dst_port=@bond1
> select_src_port=@bond1 output_port=@p6],
> + [0], [ignore])
> +
> +# Sends a packet to trigger recirculation.
> +AT_CHECK([ovs-appctl netdev-dummy/receive p5
> "in_port(5),eth(src=50:54:00:00:00:05,dst=50:54:00:00:01:00),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1)"])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep 'in_port(p5)' | \
> + sed 's/.*actions://' | grep -c p6 | tr -d '\n'], [0], [1])
> +
> +OVS_VSWITCHD_STOP()
> +AT_CLEANUP
> +
> +
> # This test verifies that the table ID is preserved across recirculation
> # when a resubmit action requires it (because the action is relative to
> # the current table rather than specifying a table).
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
