From: Ales Musil <amu...@redhat.com>
Use datapath tunnel key instead of UUID for the mac cache threshold
handling. At the same time simplify the thresholds into single hmap.
The tunnel key is unique so there shouldn't be any overlap. Having
two thresholds per datapath is currently invalid configuration anyway.
The switch to datapath's tunnel key requires somehow costly
synchronization when the tunnel key changes. However, this is fine as
the key shouldn't change very often in some cases it won't change at
all.
Also fix wrong check in the aging tests that would ignore failure.
Acked-by: Mark Michelson <mmich...@redhat.com>
Signed-off-by: Ales Musil <amu...@redhat.com>
Signed-off-by: Numan Siddique <num...@ovn.org>
---
controller/mac-cache.c | 131 ++++++++++++++++++------------------
controller/mac-cache.h | 29 ++++----
controller/ovn-controller.c | 101 +++++++++++++--------------
tests/ovn.at | 2 +-
4 files changed, 125 insertions(+), 138 deletions(-)
diff --git a/controller/mac-cache.c b/controller/mac-cache.c
index 0b249e11f..4e6b50663 100644
--- a/controller/mac-cache.c
+++ b/controller/mac-cache.c
@@ -16,6 +16,7 @@
#include <config.h>
#include <stdbool.h>
+#include "local_data.h"
#include "lport.h"
#include "mac-cache.h"
#include "openvswitch/hmap.h"
@@ -45,11 +46,8 @@ static uint32_t
fdb_data_hash(const struct fdb_data *fdb_data);
static inline bool
fdb_data_equals(const struct fdb_data *a, const struct fdb_data *b);
-static struct mac_cache_threshold *
-mac_cache_threshold_find(struct hmap *thresholds, const struct uuid *uuid);
static uint64_t
-mac_cache_threshold_get_value_ms(const struct sbrec_datapath_binding *dp,
- enum mac_cache_type type);
+mac_cache_threshold_get_value_ms(const struct sbrec_datapath_binding *dp);
static void
mac_cache_threshold_remove(struct hmap *thresholds,
struct mac_cache_threshold *threshold);
@@ -73,25 +71,23 @@ buffered_packets_db_lookup(struct buffered_packets *bp,
struct ovsdb_idl_index *sbrec_mb_by_lport_ip);
/* Thresholds. */
-bool
+void
mac_cache_threshold_add(struct mac_cache_data *data,
- const struct sbrec_datapath_binding *dp,
- enum mac_cache_type type)
+ const struct sbrec_datapath_binding *dp)
{
- struct hmap *thresholds = &data->thresholds[type];
struct mac_cache_threshold *threshold =
- mac_cache_threshold_find(thresholds, &dp->header_.uuid);
+ mac_cache_threshold_find(data, dp->tunnel_key);
if (threshold) {
- return true;
+ return;
}
- uint64_t value = mac_cache_threshold_get_value_ms(dp, type);
+ uint64_t value = mac_cache_threshold_get_value_ms(dp);
if (!value) {
- return false;
+ return;
}
threshold = xmalloc(sizeof *threshold);
- threshold->uuid = dp->header_.uuid;
+ threshold->dp_key = dp->tunnel_key;
threshold->value = value;
threshold->dump_period = value / 2;
threshold->cooldown_period = value / 4;
@@ -104,38 +100,62 @@ mac_cache_threshold_add(struct mac_cache_data *data,
ovs_assert(threshold->cooldown_period + threshold->dump_period
< (9 * value) / 10);
- hmap_insert(thresholds, &threshold->hmap_node,
- uuid_hash(&dp->header_.uuid));
-
- return true;
+ hmap_insert(&data->thresholds, &threshold->hmap_node, dp->tunnel_key);
}
-bool
+void
mac_cache_threshold_replace(struct mac_cache_data *data,
const struct sbrec_datapath_binding *dp,
- enum mac_cache_type type)
+ const struct hmap *local_datapaths)
{
- struct hmap *thresholds = &data->thresholds[type];
struct mac_cache_threshold *threshold =
- mac_cache_threshold_find(thresholds, &dp->header_.uuid);
+ mac_cache_threshold_find(data, dp->tunnel_key);
if (threshold) {
- mac_cache_threshold_remove(thresholds, threshold);
+ mac_cache_threshold_remove(&data->thresholds, threshold);
}
- return mac_cache_threshold_add(data, dp, type);
+ if (!get_local_datapath(local_datapaths, dp->tunnel_key)) {
+ return;
+ }
+
+ mac_cache_threshold_add(data, dp);
+}
+
+
+struct mac_cache_threshold *
+mac_cache_threshold_find(struct mac_cache_data *data, uint32_t dp_key)
+{
+ struct mac_cache_threshold *threshold;
+ HMAP_FOR_EACH_WITH_HASH (threshold, hmap_node, dp_key, &data->thresholds) {
+ if (threshold->dp_key == dp_key) {
+ return threshold;
+ }
+ }
+
+ return NULL;
}
void
-mac_cache_thresholds_clear(struct mac_cache_data *data)
+mac_cache_thresholds_sync(struct mac_cache_data *data,
+ const struct hmap *local_datapaths)
{
- for (size_t i = 0; i < MAC_CACHE_MAX; i++) {
- struct mac_cache_threshold *threshold;
- HMAP_FOR_EACH_POP (threshold, hmap_node, &data->thresholds[i]) {
- free(threshold);
+ struct mac_cache_threshold *threshold;
+ HMAP_FOR_EACH_SAFE (threshold, hmap_node, &data->thresholds) {
+ if (!get_local_datapath(local_datapaths, threshold->dp_key)) {
+ mac_cache_threshold_remove(&data->thresholds, threshold);
}
}
}
+void
+mac_cache_thresholds_clear(struct mac_cache_data *data)
+{
+ struct mac_cache_threshold *threshold;
+ HMAP_FOR_EACH_POP (threshold, hmap_node, &data->thresholds) {
+ free(threshold);
+ }
+}
+
/* MAC binding. */
void
mac_binding_add(struct hmap *map, struct mac_binding_data mb_data,
@@ -287,7 +307,6 @@ fdb_add(struct hmap *map, struct fdb_data fdb_data) {
if (!fdb) {
fdb = xmalloc(sizeof *fdb);
fdb->sbrec_fdb = NULL;
- fdb->dp_uuid = UUID_ZERO;
hmap_insert(map, &fdb->hmap_node, fdb_data_hash(&fdb_data));
}
@@ -428,7 +447,6 @@ mac_binding_stats_run(struct ovs_list *stats_list, uint64_t
*req_delay,
void *data)
{
struct mac_cache_data *cache_data = data;
- struct hmap *thresholds = &cache_data->thresholds[MAC_CACHE_MAC_BINDING];
long long timewall_now = time_wall_msec();
struct mac_cache_stats *stats;
@@ -442,10 +460,9 @@ mac_binding_stats_run(struct ovs_list *stats_list,
uint64_t *req_delay,
continue;
}
- struct uuid *dp_uuid = &mb->sbrec->datapath->header_.uuid;
uint64_t since_updated_ms = timewall_now - mb->sbrec->timestamp;
struct mac_cache_threshold *threshold =
- mac_cache_threshold_find(thresholds, dp_uuid);
+ mac_cache_threshold_find(cache_data, mb->data.dp_key);
/* If "idle_age" is under threshold it means that the mac binding is
* used on this chassis. */
@@ -470,7 +487,7 @@ mac_binding_stats_run(struct ovs_list *stats_list, uint64_t
*req_delay,
free(stats);
}
- mac_cache_update_req_delay(thresholds, req_delay);
+ mac_cache_update_req_delay(&cache_data->thresholds, req_delay);
if (*req_delay) {
VLOG_DBG("MAC binding statistics dalay: %"PRIu64, *req_delay);
}
@@ -526,7 +543,6 @@ fdb_stats_run(struct ovs_list *stats_list, uint64_t
*req_delay,
void *data)
{
struct mac_cache_data *cache_data = data;
- struct hmap *thresholds = &cache_data->thresholds[MAC_CACHE_FDB];
long long timewall_now = time_wall_msec();
struct mac_cache_stats *stats;
@@ -542,7 +558,7 @@ fdb_stats_run(struct ovs_list *stats_list, uint64_t
*req_delay,
uint64_t since_updated_ms = timewall_now - fdb->sbrec_fdb->timestamp;
struct mac_cache_threshold *threshold =
- mac_cache_threshold_find(thresholds, &fdb->dp_uuid);
+ mac_cache_threshold_find(cache_data, fdb->data.dp_key);
/* If "idle_age" is under threshold it means that the fdb entry is
* used on this chassis. */
@@ -567,7 +583,7 @@ fdb_stats_run(struct ovs_list *stats_list, uint64_t
*req_delay,
free(stats);
}
- mac_cache_update_req_delay(thresholds, req_delay);
+ mac_cache_update_req_delay(&cache_data->thresholds, req_delay);
if (*req_delay) {
VLOG_DBG("FDB entry statistics dalay: %"PRIu64, *req_delay);
}
@@ -774,41 +790,22 @@ fdb_data_equals(const struct fdb_data *a, const struct
fdb_data *b)
eth_addr_equals(a->mac, b->mac);
}
-
-static struct mac_cache_threshold *
-mac_cache_threshold_find(struct hmap *thresholds, const struct uuid *uuid)
+static uint64_t
+mac_cache_threshold_get_value_ms(const struct sbrec_datapath_binding *dp)
{
- uint32_t hash = uuid_hash(uuid);
+ uint64_t mb_value =
+ smap_get_uint(&dp->external_ids, "mac_binding_age_threshold", 0);
+ uint64_t fdb_value =
+ smap_get_uint(&dp->external_ids, "fdb_age_threshold", 0);
- struct mac_cache_threshold *threshold;
- HMAP_FOR_EACH_WITH_HASH (threshold, hmap_node, hash, thresholds) {
- if (uuid_equals(&threshold->uuid, uuid)) {
- return threshold;
- }
+ if (mb_value && fdb_value) {
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+ VLOG_WARN_RL(&rl, "Invalid aging threshold configuration for datapath:"
+ " "UUID_FMT, UUID_ARGS(&dp->header_.uuid));
+ return 0;
}
- return NULL;
-}
-
-static uint64_t
-mac_cache_threshold_get_value_ms(const struct sbrec_datapath_binding *dp,
- enum mac_cache_type type)
-{
- uint64_t value = 0;
- switch (type) {
- case MAC_CACHE_MAC_BINDING:
- value = smap_get_uint(&dp->external_ids, "mac_binding_age_threshold",
- 0);
- break;
- case MAC_CACHE_FDB:
- value = smap_get_uint(&dp->external_ids, "fdb_age_threshold", 0);
- break;
- case MAC_CACHE_MAX:
- default:
- break;
- }
-
- return value * 1000;
+ return mb_value ? mb_value * 1000 : fdb_value * 1000;
}
static void
diff --git a/controller/mac-cache.h b/controller/mac-cache.h
index 6c5b61879..4d016c995 100644
--- a/controller/mac-cache.h
+++ b/controller/mac-cache.h
@@ -29,15 +29,9 @@
struct ovsdb_idl_index;
-enum mac_cache_type {
- MAC_CACHE_MAC_BINDING,
- MAC_CACHE_FDB,
- MAC_CACHE_MAX
-};
-
struct mac_cache_data {
- /* 'struct mac_cache_threshold' by datapath UUID. */
- struct hmap thresholds[MAC_CACHE_MAX];
+ /* 'struct mac_cache_threshold' by datapath's tunnel_key. */
+ struct hmap thresholds;
/* 'struct mac_cache_mac_binding' by 'struct mac_cache_mb_data' that are
* local and have threshold > 0. */
struct hmap mac_bindings;
@@ -48,8 +42,8 @@ struct mac_cache_data {
struct mac_cache_threshold {
struct hmap_node hmap_node;
- /* Datapath UUID. */
- struct uuid uuid;
+ /* Datapath tunnel key. */
+ uint32_t dp_key;
/* Aging threshold in ms. */
uint64_t value;
/* Statistics dump period. */
@@ -92,8 +86,6 @@ struct fdb {
struct fdb_data data;
/* Reference to the SB FDB record. */
const struct sbrec_fdb *sbrec_fdb;
- /* UUID of datapath for this FDB record. */
- struct uuid dp_uuid;
};
struct bp_packet_data {
@@ -126,12 +118,15 @@ struct buffered_packets_ctx {
};
/* Thresholds. */
-bool mac_cache_threshold_add(struct mac_cache_data *data,
- const struct sbrec_datapath_binding *dp,
- enum mac_cache_type type);
-bool mac_cache_threshold_replace(struct mac_cache_data *data,
+void mac_cache_threshold_add(struct mac_cache_data *data,
+ const struct sbrec_datapath_binding *dp);
+void mac_cache_threshold_replace(struct mac_cache_data *data,
const struct sbrec_datapath_binding *dp,
- enum mac_cache_type type);
+ const struct hmap *local_datapaths);
+struct mac_cache_threshold *
+mac_cache_threshold_find(struct mac_cache_data *data, uint32_t dp_key);
+void mac_cache_thresholds_sync(struct mac_cache_data *data,
+ const struct hmap *local_datapaths);
void mac_cache_thresholds_clear(struct mac_cache_data *data);
/* MAC binding. */
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 386de6987..93725b6db 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -3388,8 +3388,7 @@ mac_binding_remove_sb(struct mac_cache_data *data,
}
static void
-fdb_add_sb(struct mac_cache_data *data, const struct sbrec_fdb *sfdb,
- struct uuid dp_uuid)
+fdb_add_sb(struct mac_cache_data *data, const struct sbrec_fdb *sfdb)
{
struct fdb_data fdb_data;
if (!fdb_data_from_sbrec(&fdb_data, sfdb)) {
@@ -3399,7 +3398,6 @@ fdb_add_sb(struct mac_cache_data *data, const struct
sbrec_fdb *sfdb,
struct fdb *fdb = fdb_add(&data->fdbs, fdb_data);
fdb->sbrec_fdb = sfdb;
- fdb->dp_uuid = dp_uuid;
}
static void
@@ -3423,8 +3421,7 @@ mac_cache_mb_handle_for_datapath(struct mac_cache_data
*data,
const struct sbrec_datapath_binding *dp,
struct ovsdb_idl_index *sbrec_mb_by_dp)
{
- bool has_threshold =
- mac_cache_threshold_replace(data, dp, MAC_CACHE_MAC_BINDING);
+ bool has_threshold = mac_cache_threshold_find(data, dp->tunnel_key);
struct sbrec_mac_binding *mb_index_row =
sbrec_mac_binding_index_init_row(sbrec_mb_by_dp);
@@ -3447,7 +3444,7 @@ mac_cache_fdb_handle_for_datapath(struct mac_cache_data
*data,
const struct sbrec_datapath_binding *dp,
struct ovsdb_idl_index *sbrec_fdb_by_dp_key)
{
- bool has_threshold = mac_cache_threshold_replace(data, dp, MAC_CACHE_FDB);
+ bool has_threshold = mac_cache_threshold_find(data, dp->tunnel_key);
struct sbrec_fdb *fdb_index_row =
sbrec_fdb_index_init_row(sbrec_fdb_by_dp_key);
@@ -3456,7 +3453,7 @@ mac_cache_fdb_handle_for_datapath(struct mac_cache_data
*data,
const struct sbrec_fdb *fdb;
SBREC_FDB_FOR_EACH_EQUAL (fdb, fdb_index_row, sbrec_fdb_by_dp_key) {
if (has_threshold) {
- fdb_add_sb(data, fdb, dp->header_.uuid);
+ fdb_add_sb(data, fdb);
} else {
fdb_remove_sb(data, fdb);
}
@@ -3471,9 +3468,7 @@ en_mac_cache_init(struct engine_node *node OVS_UNUSED,
{
struct mac_cache_data *cache_data = xzalloc(sizeof *cache_data);
- for (size_t i = 0; i < MAC_CACHE_MAX; i++) {
- hmap_init(&cache_data->thresholds[i]);
- }
+ hmap_init(&cache_data->thresholds);
hmap_init(&cache_data->mac_bindings);
hmap_init(&cache_data->fdbs);
@@ -3486,42 +3481,33 @@ en_mac_cache_run(struct engine_node *node, void *data)
struct mac_cache_data *cache_data = data;
struct ed_type_runtime_data *rt_data =
engine_get_input_data("runtime_data", node);
- const struct sbrec_mac_binding_table *mb_table =
- EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
- const struct sbrec_fdb_table *fdb_table =
- EN_OVSDB_GET(engine_get_input("SB_fdb", node));
+ const struct sbrec_datapath_binding_table *dp_table =
+ EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node));
+ struct ovsdb_idl_index *sbrec_mb_by_dp =
+ engine_ovsdb_node_get_index(
+ engine_get_input("SB_mac_binding", node),
+ "datapath");
+ struct ovsdb_idl_index *sbrec_fdb_by_dp_key =
+ engine_ovsdb_node_get_index(
+ engine_get_input("SB_fdb", node),
+ "dp_key");
mac_cache_thresholds_clear(cache_data);
mac_bindings_clear(&cache_data->mac_bindings);
fdbs_clear(&cache_data->fdbs);
- const struct sbrec_mac_binding *sbrec_mb;
- SBREC_MAC_BINDING_TABLE_FOR_EACH (sbrec_mb, mb_table) {
+ const struct sbrec_datapath_binding *sbrec_dp;
+ SBREC_DATAPATH_BINDING_TABLE_FOR_EACH (sbrec_dp, dp_table) {
if (!get_local_datapath(&rt_data->local_datapaths,
- sbrec_mb->datapath->tunnel_key)) {
- continue;
- }
-
- if (mac_cache_threshold_add(cache_data, sbrec_mb->datapath,
- MAC_CACHE_MAC_BINDING)) {
- mac_binding_add_sb(cache_data, sbrec_mb);
- }
- }
-
- struct local_datapath *local_dp;
- const struct sbrec_fdb *sbrec_fdb;
- SBREC_FDB_TABLE_FOR_EACH (sbrec_fdb, fdb_table) {
- local_dp = get_local_datapath(&rt_data->local_datapaths,
- sbrec_fdb->dp_key);
- if (!local_dp) {
+ sbrec_dp->tunnel_key)) {
continue;
}
- if (mac_cache_threshold_add(cache_data, local_dp->datapath,
- MAC_CACHE_FDB)) {
- fdb_add_sb(cache_data, sbrec_fdb,
- local_dp->datapath->header_.uuid);
- }
+ mac_cache_threshold_add(cache_data, sbrec_dp);
+ mac_cache_mb_handle_for_datapath(cache_data, sbrec_dp,
+ sbrec_mb_by_dp);
+ mac_cache_fdb_handle_for_datapath(cache_data, sbrec_dp,
+ sbrec_fdb_by_dp_key);
}
engine_set_node_state(node, EN_UPDATED);
@@ -3553,8 +3539,8 @@ mac_cache_sb_mac_binding_handler(struct engine_node
*node, void *data)
continue;
}
- if (mac_cache_threshold_add(cache_data, sbrec_mb->datapath,
- MAC_CACHE_MAC_BINDING)) {
+ if (mac_cache_threshold_find(cache_data,
+ sbrec_mb->datapath->tunnel_key)) {
mac_binding_add_sb(cache_data, sbrec_mb);
}
}
@@ -3594,10 +3580,8 @@ mac_cache_sb_fdb_handler(struct engine_node *node, void
*data)
continue;
}
- if (mac_cache_threshold_add(cache_data, local_dp->datapath,
- MAC_CACHE_FDB)) {
- fdb_add_sb(cache_data, sbrec_fdb,
- local_dp->datapath->header_.uuid);
+ if (mac_cache_threshold_find(cache_data, sbrec_fdb->dp_key)) {
+ fdb_add_sb(cache_data, sbrec_fdb);
}
}
@@ -3633,10 +3617,14 @@ mac_cache_runtime_data_handler(struct engine_node
*node, void *data OVS_UNUSED)
struct tracked_datapath *tdp;
HMAP_FOR_EACH (tdp, node, &rt_data->tracked_dp_bindings) {
- if (tdp->tracked_type != TRACKED_RESOURCE_NEW) {
+ if (tdp->tracked_type == TRACKED_RESOURCE_UPDATED) {
continue;
}
+ mac_cache_threshold_replace(cache_data, tdp->dp,
+ &rt_data->local_datapaths);
+ }
+ HMAP_FOR_EACH (tdp, node, &rt_data->tracked_dp_bindings) {
mac_cache_mb_handle_for_datapath(cache_data, tdp->dp,
sbrec_mb_by_dp);
@@ -3671,16 +3659,25 @@ mac_cache_sb_datapath_binding_handler(struct
engine_node *node, void *data)
size_t previous_mb_size = hmap_count(&cache_data->mac_bindings);
size_t previous_fdb_size = hmap_count(&cache_data->fdbs);
+ bool sync_needed = false;
const struct sbrec_datapath_binding *sbrec_dp;
- SBREC_DATAPATH_BINDING_TABLE_FOR_EACH (sbrec_dp, dp_table) {
- if (sbrec_datapath_binding_is_new(sbrec_dp) ||
- sbrec_datapath_binding_is_deleted(sbrec_dp) ||
- !get_local_datapath(&rt_data->local_datapaths,
- sbrec_dp->tunnel_key)) {
- continue;
+ SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (sbrec_dp, dp_table) {
+ if (!sbrec_datapath_binding_is_new(sbrec_dp) &&
+ sbrec_datapath_binding_is_updated(
+ sbrec_dp, SBREC_DATAPATH_BINDING_COL_TUNNEL_KEY)) {
+ sync_needed = true;
}
+ mac_cache_threshold_replace(cache_data, sbrec_dp,
+ &rt_data->local_datapaths);
+ }
+
+ if (sync_needed) {
+ mac_cache_thresholds_sync(cache_data, &rt_data->local_datapaths);
+ }
+
+ SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (sbrec_dp, dp_table) {
mac_cache_mb_handle_for_datapath(cache_data, sbrec_dp,
sbrec_mb_by_dp);
@@ -3703,9 +3700,7 @@ en_mac_cache_cleanup(void *data)
struct mac_cache_data *cache_data = data;
mac_cache_thresholds_clear(cache_data);
- for (size_t i = 0; i < MAC_CACHE_MAX; i++) {
- hmap_destroy(&cache_data->thresholds[i]);
- }
+ hmap_destroy(&cache_data->thresholds);
mac_bindings_clear(&cache_data->mac_bindings);
hmap_destroy(&cache_data->mac_bindings);
fdbs_clear(&cache_data->fdbs);
diff --git a/tests/ovn.at b/tests/ovn.at
index c39a23cd7..d59610d61 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -37120,7 +37120,7 @@ OVS_WAIT_UNTIL([
test "$timestamp" != "$curr_timestamp"
])
timestamp=$(fetch_column fdb timestamp mac='"00:00:00:00:10:20"')
-check $(test "$timestamp" != "")
+check test "$timestamp" != ""
# Check if the records are removed after some inactivity
wait_row_count fdb 0 mac='"00:00:00:00:10:20"'
--
2.48.1
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
