[ovs-dev] [PATCH ovn 4/4] extend-table: Fix reusing group/meter by multiple logical flows.
Han Zhou
hzhou at ovn.org
Fri Dec 6 22:28:55 UTC 2019
A meter/group can be used by multiple logical flows. However, current
code didn't handle this properly. Each table_info item has a lflow_uuid
field, which can keep track of only a single lflow.
In most cases this doesn't create problems because multiple table_info
entries are created even for same "name".
However, when multiple lflows are added in the same main loop iteration
using the same "name" (i.e. when the new_table_id == true), the function
ovn_extend_table_assign_id() will return the old id without creating a
new entry, and the reference by the second lflow is untracked. Later
with incremental processing, if the old lflow is deleted, the table_info
will be deleted, which results in the deletion of group/meter in OVS,
even when it is still used by the second lflow.
This patch fixes the problem by adding a hmap in each desired table_info
item to keep track of multiple lflow references. A test case is added.
The test case would fail without this fix.
At the same time, this patch adds an index that maps from lflow_uuid to
a list of desired table_info items used by the lflow, so that the
ovn_extend_table_remove_desired() is more efficient, without having
to do a O(N) iteration every time.
Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine - quiet mode.")
Signed-off-by: Han Zhou <hzhou at ovn.org>
---
lib/extend-table.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++-------
lib/extend-table.h | 31 ++++++++-
tests/ovn.at | 55 ++++++++++++++++
3 files changed, 241 insertions(+), 25 deletions(-)
diff --git a/lib/extend-table.c b/lib/extend-table.c
index 82dfcfa..b102e44 100644
--- a/lib/extend-table.c
+++ b/lib/extend-table.c
@@ -31,9 +31,37 @@ ovn_extend_table_init(struct ovn_extend_table *table)
table->table_ids = bitmap_allocate(MAX_EXT_TABLE_ID);
bitmap_set1(table->table_ids, 0); /* table id 0 is invalid. */
hmap_init(&table->desired);
+ hmap_init(&table->lflow_to_desired);
hmap_init(&table->existing);
}
+static struct ovn_extend_table_info *
+ovn_extend_table_info_alloc(const char *name, uint32_t id, bool is_new_id,
+ uint32_t hash)
+{
+ struct ovn_extend_table_info *e = xmalloc(sizeof *e);
+ e->name = xstrdup(name);
+ e->table_id = id;
+ e->new_table_id = is_new_id;
+ e->hmap_node.hash = hash;
+ hmap_init(&e->references);
+ return e;
+}
+
+static void
+ovn_extend_table_info_destroy(struct ovn_extend_table_info *e)
+{
+ free(e->name);
+ struct ovn_extend_table_lflow_ref *r, *r_next;
+ HMAP_FOR_EACH_SAFE (r, r_next, hmap_node, &e->references) {
+ hmap_remove(&e->references, &r->hmap_node);
+ ovs_list_remove(&r->list_node);
+ free(r);
+ }
+ hmap_destroy(&e->references);
+ free(e);
+}
+
/* Finds and returns a group_info in 'existing' whose key is identical
* to 'target''s key, or NULL if there is none. */
struct ovn_extend_table_info *
@@ -51,6 +79,89 @@ ovn_extend_table_lookup(struct hmap *exisiting,
return NULL;
}
+static struct ovn_extend_table_lflow_to_desired *
+ovn_extend_table_find_desired_by_lflow(struct ovn_extend_table *table,
+ const struct uuid *lflow_uuid)
+{
+ struct ovn_extend_table_lflow_to_desired *l;
+ HMAP_FOR_EACH_WITH_HASH (l, hmap_node, uuid_hash(lflow_uuid),
+ &table->lflow_to_desired) {
+ if (uuid_equals(&l->lflow_uuid, lflow_uuid)) {
+ return l;
+ }
+ }
+ return NULL;
+}
+
+/* Add a reference to the list of items that <lflow_uuid> uses.
+ * If the <lflow_uuid> entry doesn't exist in lflow_to_desired mapping, add
+ * the <lflow_uuid> entry first. */
+static void
+ovn_extend_table_add_desired_to_lflow(struct ovn_extend_table *table,
+ const struct uuid *lflow_uuid,
+ struct ovn_extend_table_lflow_ref *r)
+{
+ struct ovn_extend_table_lflow_to_desired *l =
+ ovn_extend_table_find_desired_by_lflow(table, lflow_uuid);
+ if (!l) {
+ l = xmalloc(sizeof *l);
+ l->lflow_uuid = *lflow_uuid;
+ ovs_list_init(&l->desired);
+ hmap_insert(&table->lflow_to_desired, &l->hmap_node,
+ uuid_hash(lflow_uuid));
+ VLOG_DBG("%s: add new lflow_to_desired entry "UUID_FMT,
+ __func__, UUID_ARGS(lflow_uuid));
+ }
+
+ ovs_list_insert(&l->desired, &r->list_node);
+ VLOG_DBG("%s: lflow "UUID_FMT" use new item %s, id %"PRIu32,
+ __func__, UUID_ARGS(lflow_uuid), r->desired->name,
+ r->desired->table_id);
+}
+
+static struct ovn_extend_table_lflow_ref *
+ovn_extend_info_find_lflow_ref(struct ovn_extend_table_info *e,
+ const struct uuid *lflow_uuid)
+{
+ struct ovn_extend_table_lflow_ref *r;
+ HMAP_FOR_EACH_WITH_HASH (r, hmap_node, uuid_hash(lflow_uuid),
+ &e->references) {
+ if (uuid_equals(&r->lflow_uuid, lflow_uuid)) {
+ return r;
+ }
+ }
+ return NULL;
+}
+
+/* Create the cross reference between <e> and <lflow_uuid> */
+static void
+ovn_extend_info_add_lflow_ref(struct ovn_extend_table *table,
+ struct ovn_extend_table_info *e,
+ const struct uuid *lflow_uuid)
+{
+ struct ovn_extend_table_lflow_ref *r =
+ ovn_extend_info_find_lflow_ref(e, lflow_uuid);
+ if (!r) {
+ r = xmalloc(sizeof *r);
+ r->lflow_uuid = *lflow_uuid;
+ r->desired = e;
+ hmap_insert(&e->references, &r->hmap_node, uuid_hash(lflow_uuid));
+
+ ovn_extend_table_add_desired_to_lflow(table, lflow_uuid, r);
+ }
+}
+
+static void
+ovn_extend_info_del_lflow_ref(struct ovn_extend_table_lflow_ref *r)
+{
+ VLOG_DBG("%s: name %s, lflow "UUID_FMT" n %"PRIuSIZE, __func__,
+ r->desired->name, UUID_ARGS(&r->lflow_uuid),
+ hmap_count(&r->desired->references));
+ hmap_remove(&r->desired->references, &r->hmap_node);
+ ovs_list_remove(&r->list_node);
+ free(r);
+}
+
/* Clear either desired or existing in ovn_extend_table. */
void
ovn_extend_table_clear(struct ovn_extend_table *table, bool existing)
@@ -58,6 +169,16 @@ ovn_extend_table_clear(struct ovn_extend_table *table, bool existing)
struct ovn_extend_table_info *g, *next;
struct hmap *target = existing ? &table->existing : &table->desired;
+ /* Clear lflow_to_desired index, if the target is desired table. */
+ if (!existing) {
+ struct ovn_extend_table_lflow_to_desired *l, *l_next;
+ HMAP_FOR_EACH_SAFE (l, l_next, hmap_node, &table->lflow_to_desired) {
+ hmap_remove(&table->lflow_to_desired, &l->hmap_node);
+ free(l);
+ }
+ }
+
+ /* Clear the target table. */
HMAP_FOR_EACH_SAFE (g, next, hmap_node, target) {
hmap_remove(target, &g->hmap_node);
/* Don't unset bitmap for desired group_info if the group_id
@@ -65,8 +186,7 @@ ovn_extend_table_clear(struct ovn_extend_table *table, bool existing)
if (existing || g->new_table_id) {
bitmap_set0(table->table_ids, g->table_id);
}
- free(g->name);
- free(g);
+ ovn_extend_table_info_destroy(g);
}
}
@@ -75,6 +195,7 @@ ovn_extend_table_destroy(struct ovn_extend_table *table)
{
ovn_extend_table_clear(table, false);
hmap_destroy(&table->desired);
+ hmap_destroy(&table->lflow_to_desired);
ovn_extend_table_clear(table, true);
hmap_destroy(&table->existing);
bitmap_free(table->table_ids);
@@ -87,11 +208,10 @@ ovn_extend_table_remove_existing(struct ovn_extend_table *table,
{
/* Remove 'existing' from 'groups->existing' */
hmap_remove(&table->existing, &existing->hmap_node);
- free(existing->name);
/* Dealloc group_id. */
bitmap_set0(table->table_ids, existing->table_id);
- free(existing);
+ ovn_extend_table_info_destroy(existing);
}
/* Remove entries in desired table that are created by the lflow_uuid */
@@ -99,29 +219,39 @@ void
ovn_extend_table_remove_desired(struct ovn_extend_table *table,
const struct uuid *lflow_uuid)
{
- struct ovn_extend_table_info *e, *next_e;
- HMAP_FOR_EACH_SAFE (e, next_e, hmap_node, &table->desired) {
- if (uuid_equals(&e->lflow_uuid, lflow_uuid)) {
+ struct ovn_extend_table_lflow_to_desired *l =
+ ovn_extend_table_find_desired_by_lflow(table, lflow_uuid);
+
+ if (!l) {
+ return;
+ }
+
+ hmap_remove(&table->lflow_to_desired, &l->hmap_node);
+ struct ovn_extend_table_lflow_ref *r, *next_r;
+ LIST_FOR_EACH_SAFE (r, next_r, list_node, &l->desired) {
+ struct ovn_extend_table_info *e = r->desired;
+ ovn_extend_info_del_lflow_ref(r);
+ if (hmap_is_empty(&e->references)) {
+ VLOG_DBG("%s: %s, "UUID_FMT, __func__,
+ e->name, UUID_ARGS(lflow_uuid));
hmap_remove(&table->desired, &e->hmap_node);
- free(e->name);
if (e->new_table_id) {
bitmap_set0(table->table_ids, e->table_id);
}
- free(e);
+ ovn_extend_table_info_destroy(e);
}
}
-
+ free(l);
}
static struct ovn_extend_table_info*
ovn_extend_info_clone(struct ovn_extend_table_info *source)
{
- struct ovn_extend_table_info *clone = xmalloc(sizeof *clone);
- clone->name = xstrdup(source->name);
- clone->table_id = source->table_id;
- clone->new_table_id = source->new_table_id;
- clone->hmap_node.hash = source->hmap_node.hash;
- clone->lflow_uuid = source->lflow_uuid;
+ struct ovn_extend_table_info *clone =
+ ovn_extend_table_info_alloc(source->name,
+ source->table_id,
+ source->new_table_id,
+ source->hmap_node.hash);
return clone;
}
@@ -155,8 +285,12 @@ ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name,
/* Check whether we have non installed but allocated group_id. */
HMAP_FOR_EACH_WITH_HASH (table_info, hmap_node, hash, &table->desired) {
- if (!strcmp(table_info->name, name) &&
- table_info->new_table_id) {
+ if (!strcmp(table_info->name, name)) {
+ VLOG_DBG("ovn_externd_table_assign_id: reuse old id %"PRIu32
+ " for %s, used by lflow "UUID_FMT,
+ table_info->table_id, table_info->name,
+ UUID_ARGS(&lflow_uuid));
+ ovn_extend_info_add_lflow_ref(table, table_info, &lflow_uuid);
return table_info->table_id;
}
}
@@ -183,15 +317,13 @@ ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name,
}
bitmap_set1(table->table_ids, table_id);
- table_info = xmalloc(sizeof *table_info);
- table_info->name = xstrdup(name);
- table_info->table_id = table_id;
- table_info->hmap_node.hash = hash;
- table_info->new_table_id = new_table_id;
- table_info->lflow_uuid = lflow_uuid;
+ table_info = ovn_extend_table_info_alloc(name, table_id, new_table_id,
+ hash);
hmap_insert(&table->desired,
&table_info->hmap_node, table_info->hmap_node.hash);
+ ovn_extend_info_add_lflow_ref(table, table_info, &lflow_uuid);
+
return table_id;
}
diff --git a/lib/extend-table.h b/lib/extend-table.h
index 833dced..4d80cfd 100644
--- a/lib/extend-table.h
+++ b/lib/extend-table.h
@@ -31,16 +31,45 @@ struct ovn_extend_table {
* for allocated group ids in either
* desired or existing. */
struct hmap desired;
+ struct hmap lflow_to_desired; /* Index for looking up desired table
+ * items from given lflow uuid, with
+ * ovn_extend_table_lflow_to_desired nodes.
+ */
struct hmap existing;
};
+struct ovn_extend_table_lflow_to_desired {
+ struct hmap_node hmap_node; /* In ovn_extend_table.lflow_to_desired. */
+ struct uuid lflow_uuid;
+ struct ovs_list desired; /* List of desired items used by the lflow. */
+};
+
struct ovn_extend_table_info {
struct hmap_node hmap_node;
char *name; /* Name for the table entity. */
- struct uuid lflow_uuid;
uint32_t table_id;
bool new_table_id; /* 'True' if 'table_id' was reserved from
* ovn_extend_table's 'table_ids' bitmap. */
+ struct hmap references; /* The lflows that are using this item, with
+ * ovn_extend_table_lflow_ref nodes. Only useful
+ * for items in ovn_extend_table.desired. */
+};
+
+/* Maintains the link between a lflow and an ovn_extend_table_info item in
+ * ovn_extend_table.desired, indexed by both
+ * ovn_extend_table_lflow_to_desired.desired and
+ * ovn_extend_table_info.references.
+ *
+ * The struct is allocated whenever a new reference happens.
+ * It destroyed when a lflow is deleted (for all the desired table_info
+ * used by it), or when the lflow_to_desired table is being cleared.
+ * */
+struct ovn_extend_table_lflow_ref {
+ struct hmap_node hmap_node; /* In ovn_extend_table_info.references. */
+ struct ovs_list list_node; /* In ovn_extend_table_lflow_to_desired.desired.
+ */
+ struct uuid lflow_uuid;
+ struct ovn_extend_table_info *desired;
};
void ovn_extend_table_init(struct ovn_extend_table *);
diff --git a/tests/ovn.at b/tests/ovn.at
index 5dc7af2..7bb97ed 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -7380,6 +7380,61 @@ OVN_CLEANUP([hv])
AT_CLEANUP
+AT_SETUP([ovn -- same meter used by multiple logical flows])
+AT_KEYWORDS([ovn])
+ovn_start
+
+net_add n1
+
+sim_add hv
+as hv
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+for i in lp1 lp2; do
+ ovs-vsctl -- add-port br-int $i -- \
+ set interface $i external-ids:iface-id=$i \
+ options:tx_pcap=hv/$i-tx.pcap \
+ options:rxq_pcap=hv/$i-rx.pcap
+done
+
+lp1_mac="f0:00:00:00:00:01"
+lp1_ip="192.168.1.2"
+
+lp2_mac="f0:00:00:00:00:02"
+lp2_ip="192.168.1.3"
+
+ovn-nbctl ls-add lsw0
+ovn-nbctl --wait=sb lsp-add lsw0 lp1
+ovn-nbctl --wait=sb lsp-add lsw0 lp2
+ovn-nbctl lsp-set-addresses lp1 $lp1_mac
+ovn-nbctl lsp-set-addresses lp2 $lp2_mac
+ovn-nbctl --wait=sb sync
+
+ovn-appctl -t ovn-controller vlog/set file:dbg
+
+# Add acl1 and acl2 using same meter.
+ovn-nbctl meter-add http-rl1 drop 10 pktps
+ovn-nbctl --log --meter=http-rl1 acl-add lsw0 to-lport 1000 'tcp.dst==80' drop \
+ -- --log --meter=http-rl1 acl-add lsw0 to-lport 1000 'tcp.dst==81' allow
+
+ovn-nbctl --wait=hv sync
+
+AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter], [0], [ignore], [ignore])
+
+# Delete acl1, meter should be kept in OVS
+ovn-nbctl acl-del lsw0 to-lport 1000 'tcp.dst==80'
+ovn-nbctl --wait=hv sync
+AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter], [0], [ignore], [ignore])
+
+# Delete acl2, meter should be deleted in OVS
+ovn-nbctl acl-del lsw0 to-lport 1000 'tcp.dst==81'
+ovn-nbctl --wait=hv sync
+AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter], [1])
+
+OVN_CLEANUP([hv])
+AT_CLEANUP
+
+
AT_SETUP([ovn -- DSCP marking and meter check])
AT_KEYWORDS([ovn])
ovn_start
--
2.1.0
More information about the dev
mailing list