Skip to content

Commit

Permalink
controller: Add delay after multicast ARP packet
Browse files Browse the repository at this point in the history 
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 <dalvarez@redhat.com>
Signed-off-by: Ales Musil <amusil@redhat.com>
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
(cherry picked from commit b416f6f)
  • Loading branch information
@almusil @numansiddique
almusil authored and numansiddique committed Oct 20, 2022
1 parent dcf50d7 commit 73c5886
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 22 deletions.
41 changes: 33 additions & 8 deletions controller/mac-learn.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 *);
Expand All @@ -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);

Expand All @@ -75,17 +73,44 @@ 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;
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;

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)
Expand Down
9 changes: 7 additions & 2 deletions controller/mac-learn.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);



Expand Down
27 changes: 18 additions & 9 deletions controller/pinctrl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4115,9 +4115,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_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;
Expand Down Expand Up @@ -4277,14 +4281,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
Expand Down Expand Up @@ -4342,9 +4350,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);
}
}

Expand Down
6 changes: 3 additions & 3 deletions tests/ovn.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21503,7 +21503,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
Expand Down Expand Up @@ -21550,7 +21550,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
Expand Down Expand Up @@ -21579,7 +21579,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
Expand Down

0 comments on commit 73c5886

Please sign in to comment.