On 7/14/22 12:37, Ales Musil wrote: > The ovn-controller had a race condition over MAC > binding table with other controllers. When multiple > controllers received GARP from single source usually > the one who was able to win the race put it into SB. > The others got transaction error which triggered > full recompute even if it's not needed. > > In order to reduce the chance of multiple controllers > trying to insert same row at the same time add slight > delay to the MAC binding processing. This delay > is random interval between 1-50 in ms. This greatly > reduces the chance that multiple controllers will try > to add MAC binding at exactly the same time. This applies > only to multicast ARP request which applies only to GARP > that is sent to broadcast address. > > Local testing with this delay vs without show significantly > reduced chance of hitting the transaction error. > > During the testing 10 GARPs was sent to two controllers > at the same time. Without proposed fix at least one controller > had multiple transaction errors present every test iteration. > > With the proposed fix the transaction error was reduced to a > single one when it happened which was usually in less than > 10% of the iterations. > > As was mentioned before the race condition can still happen, > but the chance is greatly reduced. > > Suggested-by: Daniel Alvarez Sanchez <[email protected]> > Signed-off-by: Ales Musil <[email protected]> > --- > v3: Rebase on top of current main. > Change the delay to be per ARP request instead of > single delay for everything. This allows the possibility > to delay only multicast/broadcast AP as suggested by Han. > v4,v5: Address comments from Dumitru. > ---
Thanks for this new revision! I have two very small comments but I don't think you need to send a v6 for this, let's see if the maintainers require that or if the suggestions can be incorporated when the patch is accepted. Acked-by: Dumitru Ceara <[email protected]> > controller/mac-learn.c | 41 +++++++++++++++++++++++++++++++++-------- > controller/mac-learn.h | 9 +++++++-- > controller/pinctrl.c | 27 ++++++++++++++++++--------- > tests/ovn.at | 6 +++--- > 4 files changed, 61 insertions(+), 22 deletions(-) > > diff --git a/controller/mac-learn.c b/controller/mac-learn.c > index 27634dca8..a27607016 100644 > --- a/controller/mac-learn.c > +++ b/controller/mac-learn.c > @@ -18,14 +18,18 @@ > #include "mac-learn.h" > > /* OpenvSwitch lib includes. */ > +#include "openvswitch/poll-loop.h" > #include "openvswitch/vlog.h" > #include "lib/packets.h" > +#include "lib/random.h" > #include "lib/smap.h" > +#include "lib/timeval.h" > > VLOG_DEFINE_THIS_MODULE(mac_learn); > > #define MAX_MAC_BINDINGS 1000 > #define MAX_FDB_ENTRIES 1000 > +#define MAX_MAC_BINDING_DELAY_MSEC 50 > > static size_t mac_binding_hash(uint32_t dp_key, uint32_t port_key, > struct in6_addr *); > @@ -46,25 +50,19 @@ ovn_mac_bindings_init(struct hmap *mac_bindings) > } > > void > -ovn_mac_bindings_flush(struct hmap *mac_bindings) > +ovn_mac_bindings_destroy(struct hmap *mac_bindings) > { > struct mac_binding *mb; > HMAP_FOR_EACH_POP (mb, hmap_node, mac_bindings) { > free(mb); > } > -} > - > -void > -ovn_mac_bindings_destroy(struct hmap *mac_bindings) > -{ > - ovn_mac_bindings_flush(mac_bindings); > hmap_destroy(mac_bindings); > } > > struct mac_binding * > ovn_mac_binding_add(struct hmap *mac_bindings, uint32_t dp_key, > uint32_t port_key, struct in6_addr *ip, > - struct eth_addr mac) > + struct eth_addr mac, bool is_unicast) > { > uint32_t hash = mac_binding_hash(dp_key, port_key, ip); > > @@ -75,10 +73,13 @@ ovn_mac_binding_add(struct hmap *mac_bindings, uint32_t > dp_key, > return NULL; > } > > + uint32_t delay = is_unicast > + ? 0 : random_range(MAX_MAC_BINDING_DELAY_MSEC) + 1; Nit: the coding-style doesn't seem to be extremely specific but the way I read it this should be indented a bit differently: uint32_t delay = is_unicast ? 0 : random_range(MAX_MAC_BINDING_DELAY_MSEC) + 1; Or maybe cleaner: uint32_t delay = !is_unicast ? random_range(MAX_MAC_BINDING_DELAY_MSEC) + 1 : 0; > mb = xmalloc(sizeof *mb); > mb->dp_key = dp_key; > mb->port_key = port_key; > mb->ip = *ip; > + mb->commit_at_ms = time_msec() + delay; > hmap_insert(mac_bindings, &mb->hmap_node, hash); > } > mb->mac = mac; > @@ -86,6 +87,30 @@ ovn_mac_binding_add(struct hmap *mac_bindings, uint32_t > dp_key, > return mb; > } > > +/* This is called from ovn-controller main context */ > +void > +ovn_mac_binding_wait(struct hmap *mac_bindings) > +{ > + struct mac_binding *mb; > + > + HMAP_FOR_EACH (mb, hmap_node, mac_bindings) { > + poll_timer_wait_until(mb->commit_at_ms); > + } > +} > + > +void > +ovn_mac_binding_remove(struct mac_binding *mb, struct hmap *mac_bindings) > +{ > + hmap_remove(mac_bindings, &mb->hmap_node); > + free(mb); > +} > + > +bool > +ovn_mac_binding_can_commit(const struct mac_binding *mb, long long now) > +{ > + return now >= mb->commit_at_ms; > +} > + > /* fdb functions. */ > void > ovn_fdb_init(struct hmap *fdbs) > diff --git a/controller/mac-learn.h b/controller/mac-learn.h > index e7e8ba2d3..57c50c58b 100644 > --- a/controller/mac-learn.h > +++ b/controller/mac-learn.h > @@ -31,16 +31,21 @@ struct mac_binding { > > /* Value. */ > struct eth_addr mac; > + > + /* Timestamp when to commit to SB. */ > + long long commit_at_ms; > }; > > void ovn_mac_bindings_init(struct hmap *mac_bindings); > -void ovn_mac_bindings_flush(struct hmap *mac_bindings); > void ovn_mac_bindings_destroy(struct hmap *mac_bindings); > +void ovn_mac_binding_wait(struct hmap *mac_bindings); > +void ovn_mac_binding_remove(struct mac_binding *mb, struct hmap > *mac_bindings); > +bool ovn_mac_binding_can_commit(const struct mac_binding *mb, long long now); > > struct mac_binding *ovn_mac_binding_add(struct hmap *mac_bindings, > uint32_t dp_key, uint32_t port_key, > struct in6_addr *ip, > - struct eth_addr mac); > + struct eth_addr mac, bool > is_unicast); > > > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index f954362b7..beb3d3344 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -4134,9 +4134,13 @@ pinctrl_handle_put_mac_binding(const struct flow *md, > memcpy(&ip_key, &ip6, sizeof ip_key); > } > > + /* If the ARP reply was unicast we should not delay it, > + * there won't be any race. */ Nit: Now that you encapsulated the delay logic in mac-learn.c, I think this comment can go inside ovn_mac_binding_add() before we compute 'delay'. > + bool is_unicast = !eth_addr_is_multicast(headers->dl_dst); > struct mac_binding *mb = ovn_mac_binding_add(&put_mac_bindings, dp_key, > port_key, &ip_key, > - headers->dl_src); > + headers->dl_src, > + is_unicast); > if (!mb) { > COVERAGE_INC(pinctrl_drop_put_mac_binding); > return; > @@ -4296,14 +4300,18 @@ run_put_mac_bindings(struct ovsdb_idl_txn > *ovnsb_idl_txn, > return; > } > > - const struct mac_binding *mb; > - HMAP_FOR_EACH (mb, hmap_node, &put_mac_bindings) { > - run_put_mac_binding(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > - sbrec_port_binding_by_key, > - sbrec_mac_binding_by_lport_ip, > - mb); > + long long now = time_msec(); > + > + struct mac_binding *mb; > + HMAP_FOR_EACH_SAFE (mb, hmap_node, &put_mac_bindings) { > + if (ovn_mac_binding_can_commit(mb, now)) { > + run_put_mac_binding(ovnsb_idl_txn, > + sbrec_datapath_binding_by_key, > + sbrec_port_binding_by_key, > + sbrec_mac_binding_by_lport_ip, mb); > + ovn_mac_binding_remove(mb, &put_mac_bindings); > + } > } > - ovn_mac_bindings_flush(&put_mac_bindings); > } > > static void > @@ -4352,9 +4360,10 @@ run_buffered_binding(struct ovsdb_idl_index > *sbrec_mac_binding_by_lport_ip, > > static void > wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn) > + OVS_REQUIRES(pinctrl_mutex) > { > if (ovnsb_idl_txn && !hmap_is_empty(&put_mac_bindings)) { > - poll_immediate_wake(); > + ovn_mac_binding_wait(&put_mac_bindings); > } > } > > diff --git a/tests/ovn.at b/tests/ovn.at > index c346975e6..eff18fb84 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -23002,7 +23002,7 @@ send_garp 1 1 $eth_src $eth_dst $spa $tpa > > wait_row_count MAC_Binding 1 > > -AT_CHECK([ovn-sbctl --format=csv --bare --columns logical_port,ip,mac \ > +OVS_WAIT_UNTIL([ovn-sbctl --format=csv --bare --columns logical_port,ip,mac \ > list mac_binding], [0], [lr0-sw0 > 10.0.0.30 > 50:54:00:00:00:03 > @@ -23049,7 +23049,7 @@ grep table_id=10 | wc -l`]) > > check_row_count MAC_Binding 1 > > -AT_CHECK([ovn-sbctl --format=csv --bare --columns logical_port,ip,mac \ > +OVS_WAIT_UNTIL([ovn-sbctl --format=csv --bare --columns logical_port,ip,mac \ > list mac_binding], [0], [lr0-sw0 > 10.0.0.30 > 50:54:00:00:00:13 > @@ -23078,7 +23078,7 @@ OVS_WAIT_UNTIL( > | wc -l`] > ) > > -AT_CHECK([ovn-sbctl --format=csv --bare --columns logical_port,ip,mac \ > +OVS_WAIT_UNTIL([ovn-sbctl --format=csv --bare --columns logical_port,ip,mac \ > find mac_binding ip=10.0.0.50], [0], [lr0-sw0 > 10.0.0.50 > 50:54:00:00:00:33 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
