This makes a lot of sense, just a few comments
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"?
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
> +
> 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?
>
> 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?
> +
> # 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
