On Thu, Sep 10, 2026 at 10:36 PM Jacob Tanenbaum <[email protected]> wrote:
> This makes a lot of sense, just a few comments > Hi Jacob, thank you for the review. > > On Fri, Sep 4, 2026 at 7:46 AM Ales Musil via dev <[email protected]> > wrote: > >> The MAC binding probing mechanism is based on egress traffic >> activity. However, we would send probes for entries that never >> processed any egress traffic. Skip probes for those as it doesn't >> make sense to probe inactive entries. >> >> Fixes: 1e4d4409f391 ("controller: Send ARP/ND for stale mac_bindings >> entries.") >> Reported-at: https://redhat.atlassian.net/browse/FDP-4285 >> Signed-off-by: Ales Musil <[email protected]> >> --- >> controller/mac-cache.c | 4 ++++ >> tests/ovn.at | 20 ++++++++++++++++++-- >> 2 files changed, 22 insertions(+), 2 deletions(-) >> >> diff --git a/controller/mac-cache.c b/controller/mac-cache.c >> index 359d4c18f..814da223b 100644 >> --- a/controller/mac-cache.c >> +++ b/controller/mac-cache.c >> @@ -853,6 +853,10 @@ mac_binding_probe_stats_process_flow_stats( >> struct vector *stats_vec, >> struct ofputil_flow_stats *ofp_stats) >> { >> + if (!ofp_stats->packet_count) { >> + return; >> + } >> > > nit: Could there be a brief comment explaining why this check exists: > "Skip flows that have never matched a packet"? > I'm sorry but I don't see a reason for a comment there. The intent is pretty clear. > > also it looks like two other places in the code > in fdb_stats_process_flow_stats() > and mac_binding_stats_process_flow_stats() both use "ofp_stats->idle_age == > ofp_stats->duration_sec" to determine if packet count is zero. > "!ofp_stats->packet_count" is clearer, should these functions be kept > consistant? That could be done in a seperate cleanup patch > Sure I'll send v2 with this aligned. > > > >> + >> struct mac_cache_stats stats = (struct mac_cache_stats) { >> .idle_age_ms = ofp_stats->idle_age * 1000, >> .data.mb = (struct mac_binding_data) { >> diff --git a/tests/ovn.at b/tests/ovn.at >> index 20909b8db..380430937 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -37353,11 +37353,12 @@ check ovn-nbctl >> \ >> -- lrp-add lr lr-ls1 00:00:00:00:10:00 192.168.10.1/24 \ >> -- lsp-add-router-port ls1 ls1-lr lr-ls1 \ >> -- lsp-add ls1 vif1 \ >> - -- lsp-set-addresses vif1 "00:00:00:00:10:10 192.168.10.10" >> + -- lsp-set-addresses vif1 "unknown" >> > > Is the address changed to "unknown" so that vif1 would accept packets > destined to any MAC? Could you add a comment as to why unknown is used as > opposed to a specific address? > That is the reason, but again I don't see any value in adding a comment there. We would have to comment most of the test files like that. > > >> >> check ovs-vsctl \ >> -- add-port br-int vif1 \ >> - -- set interface vif1 external-ids:iface-id=vif1 >> + -- set interface vif1 external-ids:iface-id=vif1 \ >> + options:tx_pcap=hv1/vif1-tx.pcap options:rxq_pcap=hv1/vif1-rx.pcap >> >> OVN_POPULATE_ARP >> wait_for_ports_up >> @@ -37390,6 +37391,10 @@ dnl After waiting until the age threshold the >> entry should be deleted >> sleep 3 >> check_row_count mac_binding 0 ip="192.168.10.20" >> >> +dnl There shouldn't be any probe packet. >> +check touch empty >> +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [empty]) >> + >> OVN_CLEANUP([hv1]) >> AT_CLEANUP >> ]) >> @@ -37497,6 +37502,9 @@ send_na hv1 vif2 00:00:00:00:10:1b >> 00:00:00:00:20:00 fd12::64 fd12::1 >> check_row_count mac_binding 1 mac=\"00:00:00:00:10:1a\" ip=\"fd11::64\" >> check_row_count mac_binding 1 mac=\"00:00:00:00:10:1b\" ip=\"fd12::64\" >> >> +send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:2a fd12::64 fd11::64 >> +send_udp6 hv1 vif2 00:00:00:00:20:00 00:00:00:00:10:2b fd11::64 fd12::64 >> + >> ts0=$(fetch_column Mac_Binding timestamp mac=\"00:00:00:00:10:1a\" >> ip=\"fd11::64\") >> uuid=$(fetch_column Mac_Binding _uuid mac=\"00:00:00:00:10:1a\" >> ip=\"fd11::64\") >> OVS_WAIT_UNTIL([test $(ovs-ofctl dump-flows br-int >> table=OFTABLE_MAC_CACHE_USE | \ >> @@ -37675,6 +37683,9 @@ wait_row_count mac_binding 1 ip="10.10.10.100" >> logical_port="lr-ls1" >> send_na hv1 vif1 00:00:00:00:10:1a 00:00:00:00:10:00 fd11::64 fd11::1 >> wait_row_count mac_binding 1 ip=\"fd11::64\" logical_port=\"lr-ls1\" >> >> +send_udp hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:2a 10.10.10.100 >> 42.42.42.100 >> +send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:2a fd11::64 fd12::100 >> + >> # The first 2 probes are unicast (arp_attempts 0-1); the entry then falls >> # back to broadcast ARP / multicast NS. >> dump_arp 1 00:00:00:00:10:00 ff:ff:ff:ff:ff:ff 10.10.10.1 10.10.10.100 >> 00:00:00:00:00:00 > expected_bcast >> @@ -37793,6 +37804,7 @@ send_garp hv1 snoopvif 1 $ext_mac >> "ff:ff:ff:ff:ff:ff" $ext_ip $ext_ip >> >> # Wait for mac binding to be created >> wait_row_count mac_binding 1 ip="$ext_ip" logical_port="lr-ls" >> +send_udp hv1 snoopvif $lrp_mac 00:00:00:00:00:1e $ext_ip 192.168.1.30 >> >> # Wait for mac binding to be removed >> wait_row_count mac_binding 0 ip="$ext_ip" logical_port="lr-ls" >> @@ -37897,6 +37909,8 @@ OVS_WAIT_UNTIL([$(ovs-ofctl dump-flows br-int >> table=OFTABLE_MAC_BINDING | \ >> sed >> 's/reg15=0x.,metadata=0x./reg15=<cleared>,metadata=<cleared>/g' | \ >> grep -q >> "reg0=0xc0a81402,reg15=<cleared>,metadata=<cleared> >> actions=mod_dl_dst:00:00:00:00:30:00")]) >> >> +send_imcp_echo_req hv1 public 00:00:00:00:10:00 00:00:00:00:10:1a >> 192.168.20.2 192.168.10.100 >> > > Is this function a typo? should it be send_icmp_echo_req? I know it wasn't > introduced here but is this worth another quick cleanup patch? > Sure I will send fix in v2. > > >> + >> # Wait until ovn-controller sends the ARP request. >> OVS_WAIT_UNTIL([test $(grep -c "Sending ARP/ND.*ip: 192.168.20.2" >> hv1/ovn-controller.log) -eq 1]) >> send_imcp_echo_req hv1 public 00:00:00:00:10:00 00:00:00:00:10:1a >> 192.168.20.2 192.168.10.100 >> @@ -37916,6 +37930,8 @@ wait_row_count mac_binding 0 ip="192.168.20.2" >> logical_port="gw-join" >> send_icmp6_echo_req hv1 public 00:00:00:00:10:00 00:00:00:00:50:01 >> fd12::2 fd11::64 >> wait_row_count mac_binding 1 mac=\"00:00:00:00:30:00\" ip=\"fd12::2\" >> >> +send_icmp6_echo_req hv1 public 00:00:00:00:10:00 00:00:00:00:50:01 >> fd12::2 fd11::64 >> + >> # Wait until ovn-controller sends the NS request. >> OVS_WAIT_UNTIL([test $(grep -c "Sending ARP/ND.*ip: fd12::2" >> hv1/ovn-controller.log) -eq 1]) >> send_icmp6_echo_req hv1 public 00:00:00:00:10:00 00:00:00:00:50:01 >> fd12::2 fd11::64 >> -- >> 2.55.0 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > > Jacob > Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
