On 2/2/26 9:33 AM, Jun Gu wrote: > Thinking more about the code, it's probably more appropriate to jump out > instead of using mirrored variable. However, I'm not sure whether there > might be additional logical added before out in the future.
I think, the variable is fine, goto may be a little awkward there. I'll take a closer look to the patch tomorrow. Best regards, Ilya Maximets. > > Best regards, Jun. > > On 2/1/26 07:52, 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]> >> --- >> 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; >> + >> 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. >> + */ >> + if (mirrored && mbridge_has_mirrors(ctx->xbridge->mbridge) >> + && xport->xbundle) { >> + mirror_packet(ctx, xport->xbundle, >> + xbundle_mirror_dst(xport->xbundle->xbridge, >> + xport->xbundle)); >> + } >> } >> >> 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 >> +]) >> +AT_CHECK([ovs-appctl netdev-dummy/set-admin-state up], 0, [OK >> +]) >> + >> +# 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
