On 1/30/26 3:10 AM, Jun Gu wrote: > Hi. Thank you for your detailed feedback. I will take your comments into > consideration and send out a v4.
Thanks! > > Best regards, Jun. > > On 1/30/26 08:02, Ilya Maximets wrote: >> On 1/14/26 11:39 AM, Jun Gu wrote: >>> A bond, BM_TCP with dp_hash, is configured in the select_dst_port >>> column of the mirror table. When a packet is forwarded for output >>> through such a bond, there will be two recircs generated. Both >>> recircs currently forward the packet to the mirror's output_port. >>> >>> However, the second recirc only performs a rule lookup in table >>> 254 matching on dp_hash to determine the output and does not need >>> to mirror the packet. This patch avoids forwarding packet to the >>> mirror’s output_port during the second recirc to prevent >>> duplicate mirroring. >>> >>> Signed-off-by: Jun Gu <[email protected]> >>> --- >>> ofproto/ofproto-dpif-rid.c | 2 ++ >>> tests/ofproto-dpif.at | 54 ++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 56 insertions(+) >> >> Hi. Thanks for the patch! And sorry for delay. >> >> See some comments below. >> >>> >>> diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c >>> index f01468025..73be23037 100644 >>> --- a/ofproto/ofproto-dpif-rid.c >>> +++ b/ofproto/ofproto-dpif-rid.c >>> @@ -292,6 +292,8 @@ recirc_alloc_id(struct ofproto_dpif *ofproto) >>> struct frozen_state state = { >>> .table_id = TBL_INTERNAL, >>> .ofproto_uuid = ofproto->uuid, >>> + /* Avoid duplicate mirrors on bond using BM_TCP with dp_hash. */ >>> + .mirrors = UINT32_MAX, >> >> While this accomplishes the goal of preventing bonds from mirroring >> after recirculation, it doesn't seem like the right place to do so. >> A few reasons: >> >> 1. This function is more or less generic. It just happened to be used >> only by the bonding code, but it shouldn't assume that. It may be >> a problem in the future if we want to use this function for something >> other than bonds. It may also have consequences if we ever want to >> mirror from one of the post-recirculation rules, e.g. if the bond >> port is not a simple port, but something that could recirculate >> again or re-enter the bridge by some other means. I don't think this >> is possible today, but may become an unexpected problem in the future. >> >> 2. It seems like the root cause of the problem is that we're mirroring >> on recirculation, while we should only be mirroring on the actual >> output. If for some reason the post-recurculation rules are not >> available, we should not be mirroring the packet. For example, if >> the 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. But with >> this patch we'll do, as mirroring will be part of the first datapath >> flow. >> >> The root of the issue seems to be in the compose_output_action__() >> function that should not call the mirror_packet() in case we're not >> actually performing the output, but doing a recirculation instead. >> >> I also wonder if the same problem exists for the native tunnel termination >> as it's not really an output action as well and we probably shouldn't >> mirror in that case. It seems like we would be mirroring traffic before >> it is decapsulated as if it was sent into the tunnel (or the LOCAL port?). Thinking more about this last bit, the tunnel termination should not be a problem, as we're sort of "sending" the packet out of the tunnel endpoint port where it is consumed and decapsulated, so it should be mirrored in the process if the endpoint port has a mirror configured. The recirculation is not sending a packet anywhere, so that part is still a problem. >> >>> .metadata = { >>> .tunnel = { >>> .ip_dst = htonl(0), >>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at >>> index 7ebbee56d..53896804a 100644 >>> --- a/tests/ofproto-dpif.at >>> +++ b/tests/ofproto-dpif.at >>> @@ -6441,6 +6441,60 @@ 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, avoid duplicate mirrors on balance-tcp >>> bonding with dp_hash]) >> >> nit: I'd remove the ", avoid duplicate mirrors" part from the name. >> >>> +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 br1- -- set interface br1- type=patch options:peer=br1+ >>> ofport_request=100 -- \ >>> + add-br br-int -- \ >>> + set bridge br-int other-config:hwaddr=aa:77:aa:77:00:00 -- \ >>> + set bridge br-int datapath-type=dummy other-config:datapath-id=1235 \ >>> + fail-mode=secure -- \ >>> + add-port br-int br1+ -- set interface br1+ type=patch options:peer=br1- >>> ofport_request=101 -- \ >>> + add-port br-int 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 -- "may_enable: true" | >>> wc -l` -ge 4]) >> >> nit: May be better to use 'grep -c' instead of 'wc -l'. >> >>> + >>> +# The dl_vlan flow should not be ever matched, >>> +# since recirculation should not change the flow handling. >>> +AT_DATA([flows.txt], [dnl >>> +table=0 priority=1 in_port=5 actions=mod_vlan_vid:1,output(101) >>> +table=0 priority=2 in_port=5 dl_vlan=1 actions=drop >>> +]) >>> +AT_CHECK([ovs-ofctl add-flows br-int flows.txt]) >> >> This vlan part seems unnecessary in this test. Looks like it was copied >> from the other bonding test here. But, I think, we can just remove the >> br-int and all the port from it, as they are not needed for this test. >> p5 can be directly added to br1. >> >>> + >>> +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)' | dnl >> >> nit: Since you're using the \ in other parts, may use it here as well. >> It also looks better in the logs then dnl. >> >>> + sed 's/.*actions://' | grep -o 'p6' | wc -l | tr -d >>> '[[:space:]]'], [0], [1]) >> >> nit: maybe tr ',' '\n' | grep -c p6 ? >> >>> + >>> +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). >> >> Best regards, Ilya Maximets. >> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
