[ovs-dev] [PATCH v2 ovn] Save the addr set and port groups in lflow expr cache

numans at ovn.org numans at ovn.org
Tue Feb 18 19:53:51 UTC 2020


From: Numan Siddique <numans at ovn.org>

After the patch [1], which added caching of lflow expr, the lflow_resource_ref
is not rebuilt properly when lflow_run() is called. If a lflow is already cached
in lflow expr cache, then the lflow_resource_ref is not updated.
But flow_output_run() clears the lflow_resource_ref cache before calling lflow_run().

This results in incorrect OF flows present in the switch even if the
address set/port group references have changed.

This patch fixes this issue by saving the addr set and port group sets in
the lfow expr cache and updating the lflow_resource_ref.

[1] - 8795bec737b9("ovn-controller: Cache logical flow expr tree for each lflow.")
Fixes: 8795bec737b9("ovn-controller: Cache logical flow expr tree for each lflow.")

Signed-off-by: Numan Siddique <numans at ovn.org>
---
 controller/lflow.c | 63 ++++++++++++++++++++++++++++++++++------------
 tests/ovn.at       |  4 +++
 2 files changed, 51 insertions(+), 16 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index 79d89131a..110809df1 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -268,16 +268,21 @@ struct lflow_expr {
     struct hmap_node node;
     struct uuid lflow_uuid; /* key */
     struct expr *expr;
+    struct sset addr_sets_ref;
+    struct sset port_groups_ref;
 };
 
 static void
 lflow_expr_add(struct hmap *lflow_expr_cache,
                const struct sbrec_logical_flow *lflow,
-               struct expr *lflow_expr)
+               struct expr *lflow_expr, struct sset *addr_sets_ref,
+               struct sset *port_groups_ref)
 {
     struct lflow_expr *le = xmalloc(sizeof *le);
     le->lflow_uuid = lflow->header_.uuid;
     le->expr = lflow_expr;
+    sset_clone(&le->addr_sets_ref, addr_sets_ref);
+    sset_clone(&le->port_groups_ref, port_groups_ref);
     hmap_insert(lflow_expr_cache, &le->node, uuid_hash(&le->lflow_uuid));
 }
 
@@ -301,6 +306,8 @@ lflow_expr_delete(struct hmap *lflow_expr_cache, struct lflow_expr *le)
 {
     hmap_remove(lflow_expr_cache, &le->node);
     expr_destroy(le->expr);
+    sset_destroy(&le->addr_sets_ref);
+    sset_destroy(&le->port_groups_ref);
     free(le);
 }
 
@@ -310,6 +317,8 @@ lflow_expr_destroy(struct hmap *lflow_expr_cache)
     struct lflow_expr *le;
     HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) {
         expr_destroy(le->expr);
+        sset_destroy(&le->addr_sets_ref);
+        sset_destroy(&le->port_groups_ref);
         free(le);
     }
 
@@ -548,6 +557,25 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs)
     return true;
 }
 
+static void
+lflow_update_resource_refs(const struct sbrec_logical_flow *lflow,
+                           struct sset *addr_sets_ref,
+                           struct sset *port_groups_ref,
+                           struct lflow_resource_ref *lfrr)
+{
+    const char *addr_set_name;
+    SSET_FOR_EACH (addr_set_name, addr_sets_ref) {
+        lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name,
+                           &lflow->header_.uuid);
+    }
+
+    const char *port_group_name;
+    SSET_FOR_EACH (port_group_name, port_groups_ref) {
+        lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name,
+                           &lflow->header_.uuid);
+    }
+}
+
 static bool
 consider_logical_flow(const struct sbrec_logical_flow *lflow,
                       struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
@@ -615,33 +643,27 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
     struct hmap matches;
     struct expr *expr = NULL;
 
-    struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
-    struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
     struct lflow_expr *le = lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow);
     if (le) {
         if (delete_expr_from_cache) {
             lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
+            le = NULL;
         } else {
             expr = le->expr;
         }
     }
 
     if (!expr) {
+        struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
+        struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
+
         expr = expr_parse_string(lflow->match, &symtab, l_ctx_in->addr_sets,
                                  l_ctx_in->port_groups, &addr_sets_ref,
                                  &port_groups_ref, &error);
-        const char *addr_set_name;
-        SSET_FOR_EACH (addr_set_name, &addr_sets_ref) {
-            lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET,
-                               addr_set_name, &lflow->header_.uuid);
-        }
-        const char *port_group_name;
-        SSET_FOR_EACH (port_group_name, &port_groups_ref) {
-            lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP,
-                               port_group_name, &lflow->header_.uuid);
-        }
-        sset_destroy(&addr_sets_ref);
-        sset_destroy(&port_groups_ref);
+        /* Add the address set and port groups (if any) to the lflow
+         * references. */
+        lflow_update_resource_refs(lflow, &addr_sets_ref, &port_groups_ref,
+                                   l_ctx_out->lfrr);
 
         if (!error) {
             if (prereqs) {
@@ -658,15 +680,24 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
             free(error);
             ovnacts_free(ovnacts.data, ovnacts.size);
             ofpbuf_uninit(&ovnacts);
+            sset_destroy(&addr_sets_ref);
+            sset_destroy(&port_groups_ref);
             return true;
         }
 
         expr = expr_simplify(expr);
         expr = expr_normalize(expr);
 
-        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr);
+        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr,
+                       &addr_sets_ref, &port_groups_ref);
+        sset_destroy(&addr_sets_ref);
+        sset_destroy(&port_groups_ref);
     } else {
         expr_destroy(prereqs);
+        /* Add the cached address set and port groups (if any) to the lflow
+         * references. */
+        lflow_update_resource_refs(lflow, &le->addr_sets_ref,
+                                   &le->port_groups_ref, l_ctx_out->lfrr);
     }
 
     struct condition_aux cond_aux = {
diff --git a/tests/ovn.at b/tests/ovn.at
index ea6760e1f..254645a3a 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -13778,6 +13778,10 @@ for i in 1 2 3; do
     n_flows_after=`ovs-ofctl dump-flows br-int | grep "10.1.2.10" | wc -l`
     AT_CHECK([test $(expr $n_flows_before \* 2) = $n_flows_after], [0], [ignore])
 
+    # Trigger full recompute. Creating a chassis would trigger full recompute.
+    ovn-sbctl chassis-add tst geneve 127.0.0.4
+    ovn-sbctl chassis-del tst
+
     # Remove an ACL
     ovn-nbctl --wait=hv acl-del ls1 to-lport 200 \
             'outport=="lp2" && ip4 && ip4.src == $as1'
-- 
2.24.1



More information about the dev mailing list