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: Addressed comments from Dumitru. --- controller/mac-learn.c | 35 +++++++++++++++++++++++++++-------- controller/mac-learn.h | 9 +++++++-- controller/pinctrl.c | 30 +++++++++++++++++++++--------- 3 files changed, 55 insertions(+), 19 deletions(-) diff --git a/controller/mac-learn.c b/controller/mac-learn.c index 27634dca8..5cd2fff76 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_delayed) { uint32_t hash = mac_binding_hash(dp_key, port_key, ip); @@ -79,6 +77,9 @@ ovn_mac_binding_add(struct hmap *mac_bindings, uint32_t dp_key, mb->dp_key = dp_key; mb->port_key = port_key; mb->ip = *ip; + mb->commit_delay_ms = is_delayed ? + random_range(MAX_MAC_BINDING_DELAY_MSEC) + 1 : 0; + mb->created_ms = time_msec(); hmap_insert(mac_bindings, &mb->hmap_node, hash); } mb->mac = mac; @@ -86,6 +87,24 @@ 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(mb->commit_delay_ms); + } +} + +void +ovn_mac_binding_remove(struct mac_binding *mb, struct hmap *mac_bindings) +{ + hmap_remove(mac_bindings, &mb->hmap_node); + free(mb); +} + /* fdb functions. */ void ovn_fdb_init(struct hmap *fdbs) diff --git a/controller/mac-learn.h b/controller/mac-learn.h index e7e8ba2d3..844db036d 100644 --- a/controller/mac-learn.h +++ b/controller/mac-learn.h @@ -31,16 +31,21 @@ struct mac_binding { /* Value. */ struct eth_addr mac; + + /* Delay before the entry is added to SB. */ + long long created_ms; + uint32_t commit_delay_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); 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_delayed); diff --git a/controller/pinctrl.c b/controller/pinctrl.c index f954362b7..6a04b7452 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. */ + bool is_multicast = 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_multicast); if (!mb) { COVERAGE_INC(pinctrl_drop_put_mac_binding); return; @@ -4296,14 +4300,21 @@ 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) { + long long elapsed = now - mb->created_ms; + if (elapsed >= mb->commit_delay_ms) { + 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); + } else { + mb->commit_delay_ms -= elapsed; + } } - ovn_mac_bindings_flush(&put_mac_bindings); } static void @@ -4352,9 +4363,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); } } -- 2.35.3 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
