[ovs-dev,2.12,4/4] extend-table: Fix reusing group/meter by multiple logical flows.
diff mbox series

Message ID 1576957867-69488-5-git-send-email-hzhou@ovn.org
State New
Headers show
Series
  • Fix reusing meter/group by multiple logical flows.
Related show

Commit Message

Han Zhou Dec. 21, 2019, 7:51 p.m. UTC
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.")
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Han Zhou <hzhou@ovn.org>
---
 ovn/lib/extend-table.c | 180 ++++++++++++++++++++++++++++++++++++++++++-------
 ovn/lib/extend-table.h |  31 ++++++++-
 tests/ovn.at           |  55 +++++++++++++++
 3 files changed, 241 insertions(+), 25 deletions(-)

Patch
diff mbox series

diff --git a/ovn/lib/extend-table.c b/ovn/lib/extend-table.c
index 3263da8..18e16f7 100644
--- a/ovn/lib/extend-table.c
+++ b/ovn/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/ovn/lib/extend-table.h b/ovn/lib/extend-table.h
index 5be13fe..c4bc5ff 100644
--- a/ovn/lib/extend-table.h
+++ b/ovn/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 3acd8a1..4792e28 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -7336,6 +7336,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