[ovs-dev] [PATCH ovn v2 1/2] lflow.c: Avoid adding redundant resource refs for port-bindings.

Han Zhou hzhou at ovn.org
Tue Sep 15 18:40:25 UTC 2020


When a lport is referenced by a logical flow where port-binding refs
needs to be added, currently it can add the same reference pair multiple
times in below situations (introduced in commit ade4e77):

1) In add_matches_to_flow_table(), different matches from same lflow
   can have same inport/outport.

2) In is_chassis_resident_cb(), a lflow can have multiple is_chassis_resident
   check for same lport (although not very common), and at the same time
   the lport used in is_chassis_resident can overlap with the inport/
   outport of the same flow.

Now because of the redundant entries added, it results in unexpected behavior
such as same lflow being processed multiple times as a waste of processing.
More severely, after commit 580aea72e it can result in orphaned pointer leading
to crash, as reported in [0].

This patch fixes the problems by checking existance of same reference before
adding in lflow_resource_add(). To do this check efficiently, hmap is used to
replace the list struct for the resource-to-lflow index.

[0] https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374991.html

Reported-by: Dumitru Ceara <dceara at redhat.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374991.html
Fixes: ade4e779d3fb ("ovn-controller: Use the tracked runtime data changes for flow calculation.")
Fixes: 580aea72e26f ("ovn-controller: Fix conjunction handling with incremental processing.")
Signed-off-by: Han Zhou <hzhou at ovn.org>
---
v1 -> v2:
 - Addressed Dumitru's comments.
 - Moved the unrelated change to a separate patch in the series:
   "lflow.c: Release ref_lflow_node as soon as it is not needed."

 controller/lflow.c | 69 +++++++++++++++++++++++++++++++++++++-----------------
 controller/lflow.h | 27 +++++++++++----------
 tests/ovn.at       | 49 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 112 insertions(+), 33 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index e785866..86a5b76 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -74,6 +74,15 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
                       struct lflow_ctx_out *l_ctx_out);
 static void lflow_resource_add(struct lflow_resource_ref *, enum ref_type,
                                const char *ref_name, const struct uuid *);
+static struct ref_lflow_node * ref_lflow_lookup(struct hmap *ref_lflow_table,
+                                               enum ref_type,
+                                               const char *ref_name);
+static struct lflow_ref_node * lflow_ref_lookup(struct hmap *lflow_ref_table,
+                                               const struct uuid *lflow_uuid);
+static void ref_lflow_node_destroy(struct ref_lflow_node *);
+static void lflow_resource_destroy_lflow(struct lflow_resource_ref *,
+                                         const struct uuid *lflow_uuid);
+
 
 static bool
 lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp)
@@ -161,15 +170,14 @@ lflow_resource_destroy(struct lflow_resource_ref *lfrr)
 {
     struct ref_lflow_node *rlfn, *rlfn_next;
     HMAP_FOR_EACH_SAFE (rlfn, rlfn_next, node, &lfrr->ref_lflow_table) {
-        free(rlfn->ref_name);
         struct lflow_ref_list_node *lrln, *next;
-        LIST_FOR_EACH_SAFE (lrln, next, ref_list, &rlfn->ref_lflow_head) {
-            ovs_list_remove(&lrln->ref_list);
-            ovs_list_remove(&lrln->lflow_list);
+        HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids) {
+            ovs_list_remove(&lrln->list_node);
+            hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node);
             free(lrln);
         }
         hmap_remove(&lfrr->ref_lflow_table, &rlfn->node);
-        free(rlfn);
+        ref_lflow_node_destroy(rlfn);
     }
     hmap_destroy(&lfrr->ref_lflow_table);
 
@@ -224,17 +232,28 @@ lflow_resource_add(struct lflow_resource_ref *lfrr, enum ref_type type,
 {
     struct ref_lflow_node *rlfn = ref_lflow_lookup(&lfrr->ref_lflow_table,
                                                    type, ref_name);
+    struct lflow_ref_node *lfrn = lflow_ref_lookup(&lfrr->lflow_ref_table,
+                                                   lflow_uuid);
+    if (rlfn && lfrn) {
+        /* Check if the mapping already existed before adding a new one. */
+        struct lflow_ref_list_node *n;
+        HMAP_FOR_EACH_WITH_HASH (n, hmap_node, uuid_hash(lflow_uuid),
+                                &rlfn->lflow_uuids) {
+            if (uuid_equals(&n->lflow_uuid, lflow_uuid)) {
+                return;
+            }
+        }
+    }
+
     if (!rlfn) {
         rlfn = xzalloc(sizeof *rlfn);
         rlfn->node.hash = hash_string(ref_name, type);
         rlfn->type = type;
         rlfn->ref_name = xstrdup(ref_name);
-        ovs_list_init(&rlfn->ref_lflow_head);
+        hmap_init(&rlfn->lflow_uuids);
         hmap_insert(&lfrr->ref_lflow_table, &rlfn->node, rlfn->node.hash);
     }
 
-    struct lflow_ref_node *lfrn = lflow_ref_lookup(&lfrr->lflow_ref_table,
-                                                   lflow_uuid);
     if (!lfrn) {
         lfrn = xzalloc(sizeof *lfrn);
         lfrn->node.hash = uuid_hash(lflow_uuid);
@@ -245,8 +264,17 @@ lflow_resource_add(struct lflow_resource_ref *lfrr, enum ref_type type,
 
     struct lflow_ref_list_node *lrln = xzalloc(sizeof *lrln);
     lrln->lflow_uuid = *lflow_uuid;
-    ovs_list_push_back(&rlfn->ref_lflow_head, &lrln->ref_list);
-    ovs_list_push_back(&lfrn->lflow_ref_head, &lrln->lflow_list);
+    lrln->rlfn = rlfn;
+    hmap_insert(&rlfn->lflow_uuids, &lrln->hmap_node, uuid_hash(lflow_uuid));
+    ovs_list_push_back(&lfrn->lflow_ref_head, &lrln->list_node);
+}
+
+static void
+ref_lflow_node_destroy(struct ref_lflow_node *rlfn)
+{
+    free(rlfn->ref_name);
+    hmap_destroy(&rlfn->lflow_uuids);
+    free(rlfn);
 }
 
 static void
@@ -261,9 +289,9 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr,
 
     hmap_remove(&lfrr->lflow_ref_table, &lfrn->node);
     struct lflow_ref_list_node *lrln, *next;
-    LIST_FOR_EACH_SAFE (lrln, next, lflow_list, &lfrn->lflow_ref_head) {
-        ovs_list_remove(&lrln->ref_list);
-        ovs_list_remove(&lrln->lflow_list);
+    LIST_FOR_EACH_SAFE (lrln, next, list_node, &lfrn->lflow_ref_head) {
+        ovs_list_remove(&lrln->list_node);
+        hmap_remove(&lrln->rlfn->lflow_uuids, &lrln->hmap_node);
         free(lrln);
     }
     free(lfrn);
@@ -531,12 +559,12 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
     hmap_remove(&l_ctx_out->lfrr->ref_lflow_table, &rlfn->node);
 
     struct lflow_ref_list_node *lrln, *next;
-    /* Detach the rlfn->ref_lflow_head nodes from the lfrr table and clean
+    /* Detach the rlfn->lflow_uuids nodes from the lfrr table and clean
      * up all other nodes related to the lflows that uses the resource,
      * so that the old nodes won't interfere with updating the lfrr table
      * when reparsing the lflows. */
-    LIST_FOR_EACH (lrln, ref_list, &rlfn->ref_lflow_head) {
-        ovs_list_remove(&lrln->lflow_list);
+    HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
+        ovs_list_remove(&lrln->list_node);
     }
 
     struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
@@ -565,7 +593,7 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
     /* Firstly, flood remove the flows from desired flow table. */
     struct hmap flood_remove_nodes = HMAP_INITIALIZER(&flood_remove_nodes);
     struct ofctrl_flood_remove_node *ofrn, *ofrn_next;
-    LIST_FOR_EACH (lrln, ref_list, &rlfn->ref_lflow_head) {
+    HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
         VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d,"
                  " name: %s.",
                  UUID_ARGS(&lrln->lflow_uuid),
@@ -604,12 +632,11 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
     }
     hmap_destroy(&flood_remove_nodes);
 
-    LIST_FOR_EACH_SAFE (lrln, next, ref_list, &rlfn->ref_lflow_head) {
-        ovs_list_remove(&lrln->ref_list);
+    HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids) {
+        hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node);
         free(lrln);
     }
-    free(rlfn->ref_name);
-    free(rlfn);
+    ref_lflow_node_destroy(rlfn);
 
     dhcp_opts_destroy(&dhcp_opts);
     dhcp_opts_destroy(&dhcpv6_opts);
diff --git a/controller/lflow.h b/controller/lflow.h
index c66b318..1251fb0 100644
--- a/controller/lflow.h
+++ b/controller/lflow.h
@@ -78,25 +78,28 @@ enum ref_type {
     REF_TYPE_PORTBINDING
 };
 
-/* Maintains the relationship for a pair of named resource and
- * a lflow, indexed by both ref_lflow_table and lflow_ref_table. */
-struct lflow_ref_list_node {
-    struct ovs_list lflow_list; /* list for same lflow */
-    struct ovs_list ref_list; /* list for same ref */
-    struct uuid lflow_uuid;
-};
-
 struct ref_lflow_node {
-    struct hmap_node node;
+    struct hmap_node node; /* node in lflow_resource_ref.ref_lflow_table. */
     enum ref_type type; /* key */
     char *ref_name; /* key */
-    struct ovs_list ref_lflow_head;
+    struct hmap lflow_uuids; /* Contains lflow_ref_list_node. Use hmap instead
+                                of list so that lflow_resource_add() can check
+                                and avoid adding redundant entires in O(1). */
 };
 
 struct lflow_ref_node {
-    struct hmap_node node;
+    struct hmap_node node; /* node in lflow_resource_ref.lflow_ref_table. */
     struct uuid lflow_uuid; /* key */
-    struct ovs_list lflow_ref_head;
+    struct ovs_list lflow_ref_head; /* Contains lflow_ref_list_node. */
+};
+
+/* Maintains the relationship for a pair of named resource and
+ * a lflow, indexed by both ref_lflow_table and lflow_ref_table. */
+struct lflow_ref_list_node {
+    struct ovs_list list_node; /* node in lflow_ref_node.lflow_ref_head. */
+    struct hmap_node hmap_node; /* node in ref_lflow_node.lflow_uuids. */
+    struct uuid lflow_uuid;
+    struct ref_lflow_node *rlfn;
 };
 
 struct lflow_resource_ref {
diff --git a/tests/ovn.at b/tests/ovn.at
index 41fe577..d368fb9 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -22117,3 +22117,52 @@ OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected])
 
 OVN_CLEANUP([hv1])
 AT_CLEANUP
+
+AT_SETUP([ovn -- port bind/unbind change handling with conj flows - IPv6])
+ovn_start
+
+ovn-nbctl ls-add ls1
+
+ovn-nbctl lsp-add ls1 lsp1 \
+    -- lsp-set-addresses lsp1 "f0:00:00:00:00:01 2001::1" \
+    -- lsp-set-port-security lsp1 "f0:00:00:00:00:01 2001::1"
+
+net_add n1
+sim_add hv1
+
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+ovs-vsctl -- add-port br-int hv1-vif1 -- \
+    set interface hv1-vif1 external-ids:iface-id=lsp1 \
+    options:tx_pcap=hv1/vif1-tx.pcap \
+    options:rxq_pcap=hv1/vif1-rx.pcap \
+    ofport-request=1
+
+ovn-nbctl --wait=hv sync
+
+# Expected conjunction flows:
+# ... nd_tll=00:00:00:00:00:00 actions=conjunction(2,2/2)
+# ... nd_tll=f0:00:00:00:00:01 actions=conjunction(2,2/2)
+# ... nd_target=fe80::f200:ff:fe00:1 actions=conjunction(2,1/2)
+# ... nd_target=2001::1 actions=conjunction(2,1/2)
+OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction | wc -l`])
+
+# unbind the port
+ovs-vsctl set interface hv1-vif1 external_ids:iface-id=foo
+OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction | wc -l`])
+
+# bind the port again
+ovs-vsctl set interface hv1-vif1 external_ids:iface-id=lsp1
+OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction | wc -l`])
+
+# unbind the port again
+ovs-vsctl set interface hv1-vif1 external_ids:iface-id=foo
+OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction | wc -l`])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
-- 
2.1.0



More information about the dev mailing list