On Fri, Aug 29, 2025 at 07:29:24AM +0200, Ales Musil wrote:
> On Thu, Aug 28, 2025 at 5:52 PM Felix Huettner via dev <
> [email protected]> wrote:
>
> > When installing a new flow then its idle_age will start ticking from 0.
> > We previously treated this as the flow being used and bumped the
> > MAC_Binding
> > timestamp. However if the flow has just been freshly installed it was
> > never used and therefore should not be bumped.
> > This is especially important for chassis that add the flows newly
> > because they became part of the relevant datapath.
> >
> > In case of a large enough mac_binding_age_threshold and if the datapath
> > is added and removed across chassis faster than the
> > mac_binding_age_threshold this can actually lead to MAC_Binding that are
> > never removed even though they are inactive.
> >
> > We also need to fix the test "MAC binding aging - probing GW router
> > Dynamic Neigh" since it previously relied on the fact that a newly
> > created flow directly triggers an update. This pushed the time of the
> > first probe request sufficiently for the test to work. As we now no
> > longer do this we can at most wait for 'cooldown_period' as afterwards a
> > probe might be send immediately.
> >
> > Signed-off-by: Felix Huettner <[email protected]>
> > ---
> >
>
> Hi Felix,
>
> thank you for the fix. I would suggest slightly different approach
> that is simpler IMO and doesn't increase memory usage.
> See down below.
>
>
> > controller/mac-cache.c | 7 +++--
> > controller/mac-cache.h | 2 ++
> > tests/ovn.at | 66 ++++++++++++++++++++++++++++++++++++++++--
> > 3 files changed, 71 insertions(+), 4 deletions(-)
> >
> > diff --git a/controller/mac-cache.c b/controller/mac-cache.c
> > index fcf5fa7d4..173e39145 100644
> > --- a/controller/mac-cache.c
> > +++ b/controller/mac-cache.c
> > @@ -348,6 +348,7 @@ mac_binding_stats_process_flow_stats(struct vector
> > *stats_vec,
> > {
> > struct mac_cache_stats stats = (struct mac_cache_stats) {
> > .idle_age_ms = ofp_stats->idle_age * 1000,
> > + .has_ever_hit = ofp_stats->idle_age < ofp_stats->duration_sec,
> >
>
> Instead of adding new bool we can simply do the following if at the
> start of the "*_stats_process_flow_stats()":
>
> if (ofp_stats->idle_age == ofp_stats->duration_sec) {
> return;
> }
>
> WDYT?
Hi Ales,
that sounds good for me.
I'll send a v2.
Thanks,
Felix
>
> .data.mb = (struct mac_binding_data) {
> > .cookie = ntohll(ofp_stats->cookie),
> > /* The port_key must be zero to match
> > @@ -419,7 +420,7 @@ mac_binding_stats_run(
> >
> > /* If "idle_age" is under threshold it means that the mac binding
> > is
> > * used on this chassis. */
> > - if (stats->idle_age_ms < threshold->value) {
> > + if (stats->has_ever_hit && stats->idle_age_ms < threshold->value)
> > {
> > if (since_updated_ms >= threshold->cooldown_period) {
> > mac_binding_update_log("Updating active", &mb->data, true,
> > threshold, stats->idle_age_ms,
> > @@ -452,6 +453,7 @@ fdb_stats_process_flow_stats(struct vector *stats_vec,
> > {
> > struct mac_cache_stats stats = (struct mac_cache_stats) {
> > .idle_age_ms = ofp_stats->idle_age * 1000,
> > + .has_ever_hit = ofp_stats->idle_age < ofp_stats->duration_sec,
> > .data.fdb = (struct fdb_data) {
> > .port_key = ofp_stats->match.flow.regs[MFF_LOG_INPORT -
> > MFF_REG0],
> > .dp_key = ntohll(ofp_stats->match.flow.metadata),
> > @@ -514,7 +516,7 @@ fdb_stats_run(struct rconn *swconn OVS_UNUSED,
> >
> > /* If "idle_age" is under threshold it means that the fdb entry is
> > * used on this chassis. */
> > - if (stats->idle_age_ms < threshold->value) {
> > + if (stats->has_ever_hit && stats->idle_age_ms < threshold->value)
> > {
> > if (since_updated_ms >= threshold->cooldown_period) {
> > fdb_update_log("Updating active", &fdb->data, true,
> > threshold, stats->idle_age_ms,
> > @@ -838,6 +840,7 @@ mac_binding_probe_stats_process_flow_stats(
> > {
> > struct mac_cache_stats stats = (struct mac_cache_stats) {
> > .idle_age_ms = ofp_stats->idle_age * 1000,
> > + .has_ever_hit = ofp_stats->idle_age < ofp_stats->duration_sec,
> > .data.mb = (struct mac_binding_data) {
> > .cookie = ntohll(ofp_stats->cookie),
> > /* The port_key must be zero to match
> > diff --git a/controller/mac-cache.h b/controller/mac-cache.h
> > index d7b5b9cd5..fa462e82c 100644
> > --- a/controller/mac-cache.h
> > +++ b/controller/mac-cache.h
> > @@ -93,6 +93,8 @@ struct fdb {
> >
> > struct mac_cache_stats {
> > int64_t idle_age_ms;
> > + bool has_ever_hit; /* Defines if the entry might have been hit at all,
> > + * independent of idle_age_ms. */
> >
> > union {
> > /* Common data to identify MAC binding. */
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 292ca0dae..81fbfc629 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -36578,6 +36578,68 @@ OVN_CLEANUP([hv1])
> > AT_CLEANUP
> > ])
> >
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([MAC binding aging - unused entries])
> > +ovn_start
> > +
> > +net_add n1
> > +sim_add hv1
> > +as hv1
> > +check ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.1
> > +ovn-appctl -t ovn-controller vlog/set mac_cache:file:dbg pinctrl:file:dbg
> > +
> > +check ovn-nbctl \
> > + -- ls-add ls1 \
> > + -- lr-add lr \
> > + -- set logical_router lr options:mac_binding_age_threshold=4 \
> > + -- lrp-add lr lr-ls1 00:00:00:00:10:00 192.168.10.1/24 \
> > + -- lsp-add ls1 ls1-lr \
> > + -- lsp-set-type ls1-lr router \
> > + -- lsp-set-addresses ls1-lr router \
> > + -- lsp-set-options ls1-lr router-port=lr-ls1 \
> > + -- lsp-add ls1 vif1 \
> > + -- lsp-set-addresses vif1 "00:00:00:00:10:10 192.168.10.10"
> > +
> > +check ovs-vsctl \
> > + -- add-port br-int vif1 \
> > + -- set interface vif1 external-ids:iface-id=vif1
> > +
> > +OVN_POPULATE_ARP
> > +wait_for_ports_up
> > +check ovn-nbctl --wait=hv sync
> > +
> > +dnl Wait for pinctrl thread to be connected.
> > +OVS_WAIT_UNTIL([grep pinctrl hv1/ovn-controller.log | grep -q connected])
> > +
> > +dnl Create the MAC_Binding directly in the database to ensure it is never
> > used.
> > +dnl The timestamp is set to now.
> > +dp=$(fetch_column Datapath_Binding _uuid external_ids:name=lr)
> > +now=$(date +%s)000
> > +check_uuid ovn-sbctl create MAC_Binding datapath=$dp logical_port=lr-ls1 \
> > + ip=192.168.10.20 'mac="00:00:00:11:22:33"' timestamp=$now
> > +
> > +dnl Wait until the flow is installed locally
> > +OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int table=OFTABLE_MAC_CACHE_USE |
> > grep 192.168.10.20])
> > +
> > +dnl Wait long enough to give ovn-controller the chance to decide if the
> > entry
> > +dnl should be bumped or not.
> > +sleep 2
> > +
> > +dnl The entry should still be there and the timestamp should not have been
> > +dnl updated
> > +check_row_count mac_binding 1 ip="192.168.10.20"
> > +new_ts=$(fetch_column MAC_Binding timestamp ip="192.168.10.20")
> > +check test "$now" = "$new_ts"
> > +
> > +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"
> > +
> > +OVN_CLEANUP([hv1])
> > +AT_CLEANUP
> > +])
> > +
> > OVN_FOR_EACH_NORTHD([
> > AT_SETUP([MAC binding aging - probing])
> > AT_SKIP_IF([test $HAVE_SCAPY = no])
> > @@ -36844,7 +36906,7 @@ OVS_WAIT_UNTIL([$(ovs-ofctl dump-flows br-int
> > table=OFTABLE_MAC_BINDING | \
> > n_arp=$(ovs-ofctl dump-flows br-int table=OFTABLE_MAC_CACHE_USE |awk
> > '/arp_spa=192.168.20.2/{print <http://192.168.20.2/%7Bprint>
> > substr($4,11,1)}')
> > # Check GW router does not send any ARP requests in this case.
> > OVS_WAIT_UNTIL([test $(ovs-ofctl dump-flows br-int
> > table=OFTABLE_MAC_CACHE_USE | \
> > - awk '/nw_src=192.168.10.100/{print
> > <http://192.168.10.100/%7Bprint> substr($6,10,1)}') -ge $((aging_th/2))])
> > + awk '/nw_src=192.168.10.100/{print
> > <http://192.168.10.100/%7Bprint> substr($6,10,1)}') -ge
> > $((aging_th*3/16))])
> > AT_CHECK([test $(ovs-ofctl dump-flows br-int table=OFTABLE_MAC_CACHE_USE
> > |awk '/arp_spa=192.168.20.2/{print <http://192.168.20.2/%7Bprint>
> > substr($4,11,1)}') -eq $n_arp])
> >
> > 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
> > @@ -36865,7 +36927,7 @@ check_row_count mac_binding 1
> > mac=\"00:00:00:00:30:00\" ip=\"fd12::2\"
> >
> > n_ipv6=$(ovs-ofctl dump-flows br-int table=OFTABLE_MAC_CACHE_USE |awk
> > '/ipv6_src=fd12::2/{print substr($4,11,1)}')
> > OVS_WAIT_UNTIL([test $(ovs-ofctl dump-flows br-int
> > table=OFTABLE_MAC_CACHE_USE | \
> > - awk '/ipv6_src=fd12::2/{print substr($6,10,1)}')
> > -ge $((aging_th/2))])
> > + awk '/ipv6_src=fd12::2/{print substr($6,10,1)}')
> > -ge $((aging_th*3/16))])
> > # Check GW router does not send any NS requests in this case.
> > AT_CHECK([test $(ovs-ofctl dump-flows br-int table=OFTABLE_MAC_CACHE_USE
> > |awk '/ipv6_src=fd12::2/{print substr($4,11,1)}') -eq $n_ipv6])
> > send_icmp6_echo_req hv1 public 00:00:00:00:10:00 00:00:00:00:50:01
> > fd12::2 fd11::64
> >
> > base-commit: 0935bd0fa6afa24ce013c08a2796758cd26d196b
> > --
> > 2.43.0
> >
> > _______________________________________________
> > dev mailing list
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> >
> Thanks,
> Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
