[ovs-dev] [PATCH ovn v2 4/9] ovn-controller: Fix conjunction handling with incremental processing.

Han Zhou hzhou at ovn.org
Mon Sep 7 06:45:37 UTC 2020


When translating lflows to OVS flows, different lflows can refer to same OVS
flow as a result of calling ofctrl_add_or_append_flow(), particularly for
conjunction combinding. However, the implementation doesn't work with
incremental processing, because when any of the lflows are removed, we rely on
the lflow's uuid to remove the OVS flow in the desired flow table. Currently
only one single lflow uuid is maintained in the desired flow, so removing one
of the lflows that references to the same desired flow resulted in wrong
behavior: either removing flows that are used by other lflows, or the existing
flows are not updated (part of the conjunction actions should be removed from
the flow).

To solve the problem, this patch maintains the cross reference (M:N) between
lflows (and other sb objects) and desired flows, and handles the flow removal
and updates with a flood-removal and re-add approach.

Fixes: e659bab31a9 ("Combine conjunctions with identical matches into one flow.")
Cc: Mark Michelson <mmichels at redhat.com>
Signed-off-by: Han Zhou <hzhou at ovn.org>
---
 controller/lflow.c  | 103 +++++++++------
 controller/ofctrl.c | 351 ++++++++++++++++++++++++++++++++++++++++++----------
 controller/ofctrl.h |  31 ++++-
 tests/ovn.at        |  90 ++++++++++++++
 4 files changed, 469 insertions(+), 106 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index 0c35b7d..6fbd36f 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -342,41 +342,56 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in,
     struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts);
     nd_ra_opts_init(&nd_ra_opts);
 
-    /* Handle removed flows first, and then other flows, so that when
-     * the flows being added and removed have same match conditions
-     * can be processed in the proper order */
+    struct controller_event_options controller_event_opts;
+    controller_event_opts_init(&controller_event_opts);
+
+    /* Handle flow removing first (for deleted or updated lflows), and then
+     * handle reprocessing or adding flows, so that when the flows being
+     * removed and added with same match conditions can be processed in the
+     * proper order */
+
+    struct hmap flood_remove_nodes = HMAP_INITIALIZER(&flood_remove_nodes);
+    struct ofctrl_flood_remove_node *ofrn, *next;
     SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
                                                l_ctx_in->logical_flow_table) {
-        /* Remove any flows that should be removed. */
-        if (sbrec_logical_flow_is_deleted(lflow)) {
-            VLOG_DBG("handle deleted lflow "UUID_FMT,
+        if (!sbrec_logical_flow_is_new(lflow)) {
+            VLOG_DBG("delete lflow "UUID_FMT,
                      UUID_ARGS(&lflow->header_.uuid));
-            ofctrl_remove_flows(l_ctx_out->flow_table, &lflow->header_.uuid);
-            /* Delete entries from lflow resource reference. */
-            lflow_resource_destroy_lflow(l_ctx_out->lfrr,
-                                         &lflow->header_.uuid);
+            ofrn = xmalloc(sizeof *ofrn);
+            ofrn->sb_uuid = lflow->header_.uuid;
+            hmap_insert(&flood_remove_nodes, &ofrn->hmap_node,
+                        uuid_hash(&ofrn->sb_uuid));
         }
     }
+    ofctrl_flood_remove_flows(l_ctx_out->flow_table, &flood_remove_nodes);
+    HMAP_FOR_EACH (ofrn, hmap_node, &flood_remove_nodes) {
+        /* Delete entries from lflow resource reference. */
+        lflow_resource_destroy_lflow(l_ctx_out->lfrr, &ofrn->sb_uuid);
+        /* Reprocessing the lflow if the sb record is not deleted. */
+        lflow = sbrec_logical_flow_table_get_for_uuid(
+            l_ctx_in->logical_flow_table, &ofrn->sb_uuid);
+        if (lflow) {
+            VLOG_DBG("re-add lflow "UUID_FMT,
+                     UUID_ARGS(&lflow->header_.uuid));
+            if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
+                                       &nd_ra_opts, &controller_event_opts,
+                                       l_ctx_in, l_ctx_out)) {
+                ret = false;
+                break;
+            }
+        }
+    }
+    HMAP_FOR_EACH_SAFE (ofrn, next, hmap_node, &flood_remove_nodes) {
+        hmap_remove(&flood_remove_nodes, &ofrn->hmap_node);
+        free(ofrn);
+    }
+    hmap_destroy(&flood_remove_nodes);
 
-    struct controller_event_options controller_event_opts;
-    controller_event_opts_init(&controller_event_opts);
-
+    /* Now handle new lflows only. */
     SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
                                                l_ctx_in->logical_flow_table) {
-        if (!sbrec_logical_flow_is_deleted(lflow)) {
-            /* Now, add/modify existing flows. If the logical
-             * flow is a modification, just remove the flows
-             * for this row, and then add new flows. */
-            if (!sbrec_logical_flow_is_new(lflow)) {
-                VLOG_DBG("handle updated lflow "UUID_FMT,
-                         UUID_ARGS(&lflow->header_.uuid));
-                ofctrl_remove_flows(l_ctx_out->flow_table,
-                                    &lflow->header_.uuid);
-                /* Delete entries from lflow resource reference. */
-                lflow_resource_destroy_lflow(l_ctx_out->lfrr,
-                                             &lflow->header_.uuid);
-            }
-            VLOG_DBG("handle new lflow "UUID_FMT,
+        if (sbrec_logical_flow_is_new(lflow)) {
+            VLOG_DBG("add lflow "UUID_FMT,
                      UUID_ARGS(&lflow->header_.uuid));
             if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
                                        &nd_ra_opts, &controller_event_opts,
@@ -420,7 +435,6 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
      * when reparsing the lflows. */
     LIST_FOR_EACH (lrln, ref_list, &rlfn->ref_lflow_head) {
         ovs_list_remove(&lrln->lflow_list);
-        lflow_resource_destroy_lflow(l_ctx_out->lfrr, &lrln->lflow_uuid);
     }
 
     struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
@@ -446,22 +460,32 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
     controller_event_opts_init(&controller_event_opts);
 
     /* Re-parse the related lflows. */
+    /* 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) {
+        VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d,"
+                 " name: %s.",
+                 UUID_ARGS(&lrln->lflow_uuid),
+                 ref_type, ref_name);
+        ofctrl_flood_remove_add_node(&flood_remove_nodes, &lrln->lflow_uuid);
+    }
+    ofctrl_flood_remove_flows(l_ctx_out->flow_table, &flood_remove_nodes);
+
+    /* Secondly, for each lflow that is actually removed, reprocessing it. */
+    HMAP_FOR_EACH (ofrn, hmap_node, &flood_remove_nodes) {
+        lflow_resource_destroy_lflow(l_ctx_out->lfrr, &ofrn->sb_uuid);
+
         const struct sbrec_logical_flow *lflow =
             sbrec_logical_flow_table_get_for_uuid(l_ctx_in->logical_flow_table,
-                                                  &lrln->lflow_uuid);
+                                                  &ofrn->sb_uuid);
         if (!lflow) {
-            VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d,"
-                     " name: %s - not found.",
-                     UUID_ARGS(&lrln->lflow_uuid),
+            VLOG_DBG("lflow "UUID_FMT" not found while reprocessing for"
+                     " resource type: %d, name: %s.",
+                     UUID_ARGS(&ofrn->sb_uuid),
                      ref_type, ref_name);
             continue;
         }
-        VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d,"
-                 " name: %s.",
-                 UUID_ARGS(&lrln->lflow_uuid),
-                 ref_type, ref_name);
-        ofctrl_remove_flows(l_ctx_out->flow_table, &lrln->lflow_uuid);
 
         if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
                                    &nd_ra_opts, &controller_event_opts,
@@ -471,6 +495,11 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
         }
         *changed = true;
     }
+    HMAP_FOR_EACH_SAFE (ofrn, ofrn_next, hmap_node, &flood_remove_nodes) {
+        hmap_remove(&flood_remove_nodes, &ofrn->hmap_node);
+        free(ofrn);
+    }
+    hmap_destroy(&flood_remove_nodes);
 
     LIST_FOR_EACH_SAFE (lrln, next, ref_list, &rlfn->ref_lflow_head) {
         ovs_list_remove(&lrln->ref_list);
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 919db6d..0fc3e69 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -51,11 +51,45 @@
 
 VLOG_DEFINE_THIS_MODULE(ofctrl);
 
-/* An OpenFlow flow. */
+/* An OpenFlow flow.
+ *
+ * Links are maintained between desired flows and SB data. The relationship
+ * is M to N. The struct sb_flow_ref is used to link a pair of desired flow
+ * and SB UUID. The below diagram depicts the data structure.
+ *
+ *                   SB UUIDs
+ *                 +-----+-----+-----+-----+-----+-----+-----+
+ *                 |     |     |     |     |     |     |     |
+ *                 +--+--+--+--+--+--+-----+--+--+--+--+--+--+
+ *                    |     |     |           |     |     |
+ *  Desired Flows     |     |     |           |     |     |
+ *     +----+       +-+-+   |   +-+-+         |   +-+-+   |
+ *     |    +-------+   +-------+   +-------------+   |   |
+ *     +----+       +---+   |   +-+-+         |   +---+   |
+ *     |    |               |     |           |           |
+ *     +----+               |     |         +-+-+         |
+ *     |    +-------------------------------+   |         |
+ *     +----+             +---+   |         +---+         |
+ *     |    +-------------+   |   |                       |
+ *     +----+             +---+   |                       |
+ *     |    |                     |                       |
+ *     +----+                   +-+-+                   +-+-+
+ *     |    +-------------------+   +-------------------+   |
+ *     +----+                   +---+                   +---+
+ *     |    |
+ *     +----+
+ *
+ * The links are updated whenever there is a change in desired flows, which is
+ * usually triggered by a SB data change in I-P engine.
+ */
 struct ovn_flow {
     struct hmap_node match_hmap_node; /* For match based hashing. */
-    struct hindex_node uuid_hindex_node; /* For uuid based hashing. */
     struct ovs_list list_node; /* For handling lists of flows. */
+    struct ovs_list references; /* A list of struct sb_flow_ref nodes, which
+                                   references this flow. (There are cases
+                                   that multiple SB entities share the same
+                                   desired OpenFlow flow, e.g. when
+                                   conjunction is used.) */
 
     /* Key. */
     uint8_t table_id;
@@ -63,21 +97,34 @@ struct ovn_flow {
     struct minimatch match;
 
     /* Data. */
-    struct uuid sb_uuid;
     struct ofpact *ofpacts;
     size_t ofpacts_len;
     uint64_t cookie;
 };
 
+struct sb_to_flow {
+    struct hmap_node hmap_node; /* Node in
+                                   ovn_desired_flow_table.uuid_flow_table. */
+    struct uuid sb_uuid;
+    struct ovs_list flows; /* A list of struct sb_flow_ref nodes that are
+                              referenced by the sb_uuid. */
+};
+
+struct sb_flow_ref {
+    struct ovs_list sb_list; /* List node in ovn_flow.references. */
+    struct ovs_list flow_list; /* List node in sb_to_flow.ovn_flows. */
+    struct ovn_flow *flow;
+    struct uuid sb_uuid;
+};
+
 static struct ovn_flow *ovn_flow_alloc(uint8_t table_id, uint16_t priority,
                                        uint64_t cookie,
                                        const struct match *match,
-                                       const struct ofpbuf *actions,
-                                       const struct uuid *sb_uuid);
+                                       const struct ofpbuf *actions);
 static uint32_t ovn_flow_match_hash(const struct ovn_flow *);
 static struct ovn_flow *ovn_flow_lookup(struct hmap *flow_table,
                                         const struct ovn_flow *target,
-                                        bool cmp_sb_uuid);
+                                        const struct uuid *sb_uuid);
 static char *ovn_flow_to_string(const struct ovn_flow *);
 static void ovn_flow_log(const struct ovn_flow *, const char *action);
 static void ovn_flow_destroy(struct ovn_flow *);
@@ -644,13 +691,46 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type)
     }
 }
 
+static struct sb_to_flow *
+sb_to_flow_find(struct hmap *uuid_flow_table, const struct uuid *sb_uuid)
+{
+    struct sb_to_flow *stf;
+    HMAP_FOR_EACH_WITH_HASH (stf, hmap_node, uuid_hash(sb_uuid),
+                            uuid_flow_table) {
+        if (uuid_equals(sb_uuid, &stf->sb_uuid)) {
+            return stf;
+        }
+    }
+    return NULL;
+}
+
+static void
+link_flow_to_sb(struct ovn_desired_flow_table *flow_table,
+                struct ovn_flow *f, const struct uuid *sb_uuid)
+{
+    struct sb_flow_ref *sfr = xmalloc(sizeof *sfr);
+    sfr->flow = f;
+    sfr->sb_uuid = *sb_uuid;
+    ovs_list_insert(&f->references, &sfr->sb_list);
+    struct sb_to_flow *stf = sb_to_flow_find(&flow_table->uuid_flow_table,
+                                             sb_uuid);
+    if (!stf) {
+        stf = xmalloc(sizeof *stf);
+        stf->sb_uuid = *sb_uuid;
+        ovs_list_init(&stf->flows);
+        hmap_insert(&flow_table->uuid_flow_table, &stf->hmap_node,
+                    uuid_hash(sb_uuid));
+    }
+    ovs_list_insert(&stf->flows, &sfr->flow_list);
+}
+
 /* Flow table interfaces to the rest of ovn-controller. */
 
 /* Adds a flow to 'desired_flows' with the specified 'match' and 'actions' to
  * the OpenFlow table numbered 'table_id' with the given 'priority' and
  * OpenFlow 'cookie'.  The caller retains ownership of 'match' and 'actions'.
  *
- * The flow is also added to a hash index based on sb_uuid.
+ * The flow is also linked to the sb_uuid that generates it.
  *
  * This just assembles the desired flow table in memory.  Nothing is actually
  * sent to the switch until a later call to ofctrl_put().
@@ -665,11 +745,9 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table,
                           bool log_duplicate_flow)
 {
     struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match,
-                                        actions, sb_uuid);
+                                        actions);
 
-    ovn_flow_log(f, "ofctrl_add_flow");
-
-    if (ovn_flow_lookup(&flow_table->match_flow_table, f, true)) {
+    if (ovn_flow_lookup(&flow_table->match_flow_table, f, sb_uuid)) {
         if (log_duplicate_flow) {
             static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
             if (!VLOG_DROP_DBG(&rl)) {
@@ -684,31 +762,8 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table,
 
     hmap_insert(&flow_table->match_flow_table, &f->match_hmap_node,
                 f->match_hmap_node.hash);
-    hindex_insert(&flow_table->uuid_flow_table, &f->uuid_hindex_node,
-                  f->uuid_hindex_node.hash);
-}
-
-/* Removes a bundles of flows from the flow table. */
-void
-ofctrl_remove_flows(struct ovn_desired_flow_table *flow_table,
-                    const struct uuid *sb_uuid)
-{
-    struct ovn_flow *f, *next;
-    HINDEX_FOR_EACH_WITH_HASH_SAFE (f, next, uuid_hindex_node,
-                                    uuid_hash(sb_uuid),
-                                    &flow_table->uuid_flow_table) {
-        if (uuid_equals(&f->sb_uuid, sb_uuid)) {
-            ovn_flow_log(f, "ofctrl_remove_flow");
-            hmap_remove(&flow_table->match_flow_table,
-                        &f->match_hmap_node);
-            hindex_remove(&flow_table->uuid_flow_table, &f->uuid_hindex_node);
-            ovn_flow_destroy(f);
-        }
-    }
-
-    /* remove any related group and meter info */
-    ovn_extend_table_remove_desired(groups, sb_uuid);
-    ovn_extend_table_remove_desired(meters, sb_uuid);
+    link_flow_to_sb(flow_table, f, sb_uuid);
+    ovn_flow_log(f, "ofctrl_add_flow");
 }
 
 void
@@ -721,6 +776,9 @@ ofctrl_add_flow(struct ovn_desired_flow_table *desired_flows,
                               match, actions, sb_uuid, true);
 }
 
+/* Either add a new flow, or append actions on an existing flow. If the
+ * flow existed, a new link will also be created between the new sb_uuid
+ * and the existing flow. */
 void
 ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
                           uint8_t table_id, uint16_t priority, uint64_t cookie,
@@ -729,12 +787,10 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
                           const struct uuid *sb_uuid)
 {
     struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match,
-                                        actions, sb_uuid);
-
-    ovn_flow_log(f, "ofctrl_add_or_append_flow");
+                                        actions);
 
     struct ovn_flow *existing;
-    existing = ovn_flow_lookup(&desired_flows->match_flow_table, f, false);
+    existing = ovn_flow_lookup(&desired_flows->match_flow_table, f, NULL);
     if (existing) {
         /* There's already a flow with this particular match. Append the
          * action to that flow rather than adding a new flow
@@ -751,11 +807,169 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
 
         ofpbuf_uninit(&compound);
         ovn_flow_destroy(f);
+        f = existing;
     } else {
         hmap_insert(&desired_flows->match_flow_table, &f->match_hmap_node,
                     f->match_hmap_node.hash);
-        hindex_insert(&desired_flows->uuid_flow_table, &f->uuid_hindex_node,
-                      f->uuid_hindex_node.hash);
+    }
+    link_flow_to_sb(desired_flows, f, sb_uuid);
+
+    if (existing) {
+        ovn_flow_log(f, "ofctrl_add_or_append_flow (append)");
+    } else {
+        ovn_flow_log(f, "ofctrl_add_or_append_flow (add)");
+    }
+}
+
+/* Remove ovn_flows for the given "sb_to_flow" node in the uuid_flow_table.
+ * Optionally log the message for each flow that is acturally removed, if
+ * log_msg is not NULL. */
+static void
+remove_flows_from_sb_to_flow(struct ovn_desired_flow_table *flow_table,
+                             struct sb_to_flow *stf,
+                             const char *log_msg)
+{
+    struct sb_flow_ref *sfr, *next;
+    LIST_FOR_EACH_SAFE (sfr, next, flow_list, &stf->flows) {
+        ovs_list_remove(&sfr->sb_list);
+        ovs_list_remove(&sfr->flow_list);
+        struct ovn_flow *f = sfr->flow;
+        free(sfr);
+
+        if (ovs_list_is_empty(&f->references)) {
+            if (log_msg) {
+                ovn_flow_log(f, log_msg);
+            }
+            hmap_remove(&flow_table->match_flow_table,
+                        &f->match_hmap_node);
+            ovn_flow_destroy(f);
+        }
+    }
+    hmap_remove(&flow_table->uuid_flow_table, &stf->hmap_node);
+    free(stf);
+}
+
+void
+ofctrl_remove_flows(struct ovn_desired_flow_table *flow_table,
+                    const struct uuid *sb_uuid)
+{
+    struct sb_to_flow *stf = sb_to_flow_find(&flow_table->uuid_flow_table,
+                                             sb_uuid);
+    if (stf) {
+        remove_flows_from_sb_to_flow(flow_table, stf, "ofctrl_remove_flow");
+    }
+
+    /* remove any related group and meter info */
+    ovn_extend_table_remove_desired(groups, sb_uuid);
+    ovn_extend_table_remove_desired(meters, sb_uuid);
+}
+
+static struct ofctrl_flood_remove_node *
+flood_remove_find_node(struct hmap *flood_remove_nodes, struct uuid *sb_uuid)
+{
+    struct ofctrl_flood_remove_node *ofrn;
+    HMAP_FOR_EACH_WITH_HASH (ofrn, hmap_node, uuid_hash(sb_uuid),
+                             flood_remove_nodes) {
+        if (uuid_equals(&ofrn->sb_uuid, sb_uuid)) {
+            return ofrn;
+        }
+    }
+    return NULL;
+}
+
+void
+ofctrl_flood_remove_add_node(struct hmap *flood_remove_nodes,
+                             const struct uuid *sb_uuid)
+{
+    struct ofctrl_flood_remove_node *ofrn = xmalloc(sizeof *ofrn);
+    ofrn->sb_uuid = *sb_uuid;
+    hmap_insert(flood_remove_nodes, &ofrn->hmap_node, uuid_hash(sb_uuid));
+}
+
+static void
+flood_remove_flows_for_sb_uuid(struct ovn_desired_flow_table *flow_table,
+                               const struct uuid *sb_uuid,
+                               struct hmap *flood_remove_nodes)
+{
+    struct sb_to_flow *stf = sb_to_flow_find(&flow_table->uuid_flow_table,
+                                             sb_uuid);
+    if (!stf) {
+        return;
+    }
+
+    /* ovn_flows that have other references and waiting to be removed. */
+    struct ovs_list to_be_removed = OVS_LIST_INITIALIZER(&to_be_removed);
+
+    /* Traverse all flows for the given sb_uuid. */
+    struct sb_flow_ref *sfr, *next;
+    LIST_FOR_EACH_SAFE (sfr, next, flow_list, &stf->flows) {
+        struct ovn_flow *f = sfr->flow;
+        ovn_flow_log(f, "flood remove");
+
+        ovs_list_remove(&sfr->sb_list);
+        ovs_list_remove(&sfr->flow_list);
+        free(sfr);
+
+        ovs_assert(ovs_list_is_empty(&f->list_node));
+        if (ovs_list_is_empty(&f->references)) {
+            /* This is to optimize the case when most flows have only
+             * one referencing sb_uuid, so to_be_removed list should
+             * be empty in most cases. */
+            hmap_remove(&flow_table->match_flow_table,
+                        &f->match_hmap_node);
+            ovn_flow_destroy(f);
+        } else {
+            ovs_list_insert(&to_be_removed, &f->list_node);
+        }
+    }
+    hmap_remove(&flow_table->uuid_flow_table, &stf->hmap_node);
+    free(stf);
+
+    /* Traverse other referencing sb_uuids for the flows in the to_be_removed
+     * list. */
+
+    /* Detach the items in f->references from the sfr.flow_list lists,
+     * so that recursive calls will not mess up the sfr.sb_list list. */
+    struct ovn_flow *f, *f_next;
+    LIST_FOR_EACH (f, list_node, &to_be_removed) {
+        ovs_assert(!ovs_list_is_empty(&f->references));
+        LIST_FOR_EACH (sfr, sb_list, &f->references) {
+            ovs_list_remove(&sfr->flow_list);
+        }
+    }
+    LIST_FOR_EACH_SAFE (f, f_next, list_node, &to_be_removed) {
+        LIST_FOR_EACH_SAFE (sfr, next, sb_list, &f->references) {
+            if (!flood_remove_find_node(flood_remove_nodes, &sfr->sb_uuid)) {
+                ofctrl_flood_remove_add_node(flood_remove_nodes,
+                                             &sfr->sb_uuid);
+                flood_remove_flows_for_sb_uuid(flow_table, &sfr->sb_uuid,
+                                               flood_remove_nodes);
+            }
+            ovs_list_remove(&sfr->sb_list);
+            free(sfr);
+        }
+        ovs_list_remove(&f->list_node);
+        hmap_remove(&flow_table->match_flow_table,
+                    &f->match_hmap_node);
+        ovn_flow_destroy(f);
+    }
+
+}
+
+void
+ofctrl_flood_remove_flows(struct ovn_desired_flow_table *flow_table,
+                          struct hmap *flood_remove_nodes)
+{
+    struct ofctrl_flood_remove_node *ofrn;
+    HMAP_FOR_EACH (ofrn, hmap_node, flood_remove_nodes) {
+        flood_remove_flows_for_sb_uuid(flow_table, &ofrn->sb_uuid,
+                                       flood_remove_nodes);
+    }
+
+    /* remove any related group and meter info */
+    HMAP_FOR_EACH (ofrn, hmap_node, flood_remove_nodes) {
+        ovn_extend_table_remove_desired(groups, &ofrn->sb_uuid);
+        ovn_extend_table_remove_desired(meters, &ofrn->sb_uuid);
     }
 }
 
@@ -763,18 +977,17 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
 
 static struct ovn_flow *
 ovn_flow_alloc(uint8_t table_id, uint16_t priority, uint64_t cookie,
-               const struct match *match, const struct ofpbuf *actions,
-               const struct uuid *sb_uuid)
+               const struct match *match, const struct ofpbuf *actions)
 {
     struct ovn_flow *f = xmalloc(sizeof *f);
+    ovs_list_init(&f->references);
+    ovs_list_init(&f->list_node);
     f->table_id = table_id;
     f->priority = priority;
     minimatch_init(&f->match, match);
     f->ofpacts = xmemdup(actions->data, actions->size);
     f->ofpacts_len = actions->size;
-    f->sb_uuid = *sb_uuid;
     f->match_hmap_node.hash = ovn_flow_match_hash(f);
-    f->uuid_hindex_node.hash = uuid_hash(&f->sb_uuid);
     f->cookie = cookie;
 
     return f;
@@ -793,23 +1006,27 @@ static struct ovn_flow *
 ovn_flow_dup(struct ovn_flow *src)
 {
     struct ovn_flow *dst = xmalloc(sizeof *dst);
+    ovs_list_init(&dst->references);
     dst->table_id = src->table_id;
     dst->priority = src->priority;
     minimatch_clone(&dst->match, &src->match);
     dst->ofpacts = xmemdup(src->ofpacts, src->ofpacts_len);
     dst->ofpacts_len = src->ofpacts_len;
-    dst->sb_uuid = src->sb_uuid;
     dst->match_hmap_node.hash = src->match_hmap_node.hash;
-    dst->uuid_hindex_node.hash = uuid_hash(&src->sb_uuid);
     dst->cookie = src->cookie;
     return dst;
 }
 
 /* Finds and returns an ovn_flow in 'flow_table' whose key is identical to
- * 'target''s key, or NULL if there is none. */
+ * 'target''s key, or NULL if there is none.
+ *
+ * If sb_uuid is not NULL, the function will also check if the found flow is
+ * referenced by the sb_uuid.
+ *
+ * NOTE: sb_uuid can only be used for ovn_desired_flow_table lookup. */
 static struct ovn_flow *
 ovn_flow_lookup(struct hmap *flow_table, const struct ovn_flow *target,
-                bool cmp_sb_uuid)
+                const struct uuid *sb_uuid)
 {
     struct ovn_flow *f;
 
@@ -818,9 +1035,16 @@ ovn_flow_lookup(struct hmap *flow_table, const struct ovn_flow *target,
         if (f->table_id == target->table_id
             && f->priority == target->priority
             && minimatch_equal(&f->match, &target->match)) {
-            if (!cmp_sb_uuid || uuid_equals(&target->sb_uuid, &f->sb_uuid)) {
+            if (!sb_uuid) {
                 return f;
             }
+            ovs_assert(flow_table != &installed_flows);
+            struct sb_flow_ref *sfr;
+            LIST_FOR_EACH (sfr, sb_list, &f->references) {
+                if (uuid_equals(sb_uuid, &sfr->sb_uuid)) {
+                    return f;
+                }
+            }
         }
     }
     return NULL;
@@ -830,7 +1054,7 @@ static char *
 ovn_flow_to_string(const struct ovn_flow *f)
 {
     struct ds s = DS_EMPTY_INITIALIZER;
-    ds_put_format(&s, "sb_uuid="UUID_FMT", ", UUID_ARGS(&f->sb_uuid));
+
     ds_put_format(&s, "cookie=%"PRIx64", ", f->cookie);
     ds_put_format(&s, "table_id=%"PRIu8", ", f->table_id);
     ds_put_format(&s, "priority=%"PRIu16", ", f->priority);
@@ -855,6 +1079,7 @@ static void
 ovn_flow_destroy(struct ovn_flow *f)
 {
     if (f) {
+        ovs_assert(ovs_list_is_empty(&f->references));
         minimatch_destroy(&f->match);
         free(f->ofpacts);
         free(f);
@@ -866,18 +1091,16 @@ void
 ovn_desired_flow_table_init(struct ovn_desired_flow_table *flow_table)
 {
     hmap_init(&flow_table->match_flow_table);
-    hindex_init(&flow_table->uuid_flow_table);
+    hmap_init(&flow_table->uuid_flow_table);
 }
 
 void
 ovn_desired_flow_table_clear(struct ovn_desired_flow_table *flow_table)
 {
-    struct ovn_flow *f, *next;
-    HMAP_FOR_EACH_SAFE (f, next, match_hmap_node,
-                        &flow_table->match_flow_table) {
-        hmap_remove(&flow_table->match_flow_table, &f->match_hmap_node);
-        hindex_remove(&flow_table->uuid_flow_table, &f->uuid_hindex_node);
-        ovn_flow_destroy(f);
+    struct sb_to_flow *stf, *next;
+    HMAP_FOR_EACH_SAFE (stf, next, hmap_node,
+                        &flow_table->uuid_flow_table) {
+        remove_flows_from_sb_to_flow(flow_table, stf, NULL);
     }
 }
 
@@ -886,7 +1109,7 @@ ovn_desired_flow_table_destroy(struct ovn_desired_flow_table *flow_table)
 {
     ovn_desired_flow_table_clear(flow_table);
     hmap_destroy(&flow_table->match_flow_table);
-    hindex_destroy(&flow_table->uuid_flow_table);
+    hmap_destroy(&flow_table->uuid_flow_table);
 }
 
 static void
@@ -1221,7 +1444,7 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table,
     struct ovn_flow *i, *next;
     HMAP_FOR_EACH_SAFE (i, next, match_hmap_node, &installed_flows) {
         struct ovn_flow *d = ovn_flow_lookup(&flow_table->match_flow_table,
-                                             i, false);
+                                             i, NULL);
         if (!d) {
             /* Installed flow is no longer desirable.  Delete it from the
              * switch and from installed_flows. */
@@ -1237,10 +1460,6 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table,
             hmap_remove(&installed_flows, &i->match_hmap_node);
             ovn_flow_destroy(i);
         } else {
-            if (!uuid_equals(&i->sb_uuid, &d->sb_uuid)) {
-                /* Update installed flow's UUID. */
-                i->sb_uuid = d->sb_uuid;
-            }
             if (!ofpacts_equal(i->ofpacts, i->ofpacts_len,
                                d->ofpacts, d->ofpacts_len) ||
                 i->cookie != d->cookie) {
@@ -1277,7 +1496,7 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table,
      * in the installed flow table. */
     struct ovn_flow *d;
     HMAP_FOR_EACH (d, match_hmap_node, &flow_table->match_flow_table) {
-        i = ovn_flow_lookup(&installed_flows, d, false);
+        i = ovn_flow_lookup(&installed_flows, d, NULL);
         if (!i) {
             /* Send flow_mod to add flow. */
             struct ofputil_flow_mod fm = {
diff --git a/controller/ofctrl.h b/controller/ofctrl.h
index 37f06db..8789ce4 100644
--- a/controller/ofctrl.h
+++ b/controller/ofctrl.h
@@ -35,8 +35,9 @@ struct ovn_desired_flow_table {
     /* Hash map flow table using flow match conditions as hash key.*/
     struct hmap match_flow_table;
 
-    /* SB uuid index for the nodes in match_flow_table.*/
-    struct hindex uuid_flow_table;
+    /* SB uuid index for the cross reference nodes that link to the nodes in
+     * match_flow_table.*/
+    struct hmap uuid_flow_table;
 };
 
 /* Interface for OVN main loop. */
@@ -74,7 +75,30 @@ void ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
                                const struct ofpbuf *actions,
                                const struct uuid *sb_uuid);
 
-void ofctrl_remove_flows(struct ovn_desired_flow_table *, const struct uuid *);
+/* Removes a bundles of flows from the flow table for a specific sb_uuid. The
+ * flows are removed only if they are not referenced by any other sb_uuid(s).
+ * For flood-removing all related flows referenced by other sb_uuid(s), use
+ * ofctrl_flood_remove_flows(). */
+void ofctrl_remove_flows(struct ovn_desired_flow_table *,
+                         const struct uuid *sb_uuid);
+
+/* The function ofctrl_flood_remove_flows flood-removes flows from the desired
+ * flow table for the sb_uuids provided in the flood_remove_nodes argument.
+ * For each given sb_uuid in flood_remove_nodes, it removes all the flows
+ * generated by the sb_uuid, and if any of the flows are referenced by another
+ * sb_uuid, it continues removing all the flows used by that sb_uuid as well,
+ * and so on, recursively.
+ *
+ * It adds all the sb_uuids that are actually removed in the
+ * flood_remove_nodes. */
+struct ofctrl_flood_remove_node {
+    struct hmap_node hmap_node;
+    struct uuid sb_uuid;
+};
+void ofctrl_flood_remove_flows(struct ovn_desired_flow_table *,
+                               struct hmap *flood_remove_nodes);
+void ofctrl_flood_remove_add_node(struct hmap *flood_remove_nodes,
+                                  const struct uuid *sb_uuid);
 
 void ovn_desired_flow_table_init(struct ovn_desired_flow_table *);
 void ovn_desired_flow_table_clear(struct ovn_desired_flow_table *);
@@ -86,6 +110,7 @@ void ofctrl_check_and_add_flow(struct ovn_desired_flow_table *,
                                const struct ofpbuf *ofpacts,
                                const struct uuid *, bool log_duplicate_flow);
 
+
 bool ofctrl_is_connected(void);
 void ofctrl_set_probe_interval(int probe_interval);
 
diff --git a/tests/ovn.at b/tests/ovn.at
index 6c71614..31cfddc 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -13596,6 +13596,8 @@ AT_CHECK([cat 2.packets], [0], [expout])
 
 OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \
 grep conjunction | wc -l`])
+OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction.*conjunction | wc -l`])
 OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \
 grep conj_id | wc -l`])
 
@@ -13613,9 +13615,97 @@ sip=`ip_to_hex 10 0 0 4`
 dip=`ip_to_hex 10 0 0 7`
 
 test_ip 1 f00000000001 f00000000002 $sip $dip
+
 $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets
 AT_CHECK([cat 2.packets], [0], [])
 
+# Remove the first ACL, and verify that the conjunction flows are updated
+# properly.
+# There should be total of 6 flows present with conjunction action and 1 flow
+# with conj match. Eg.
+# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,2/2)
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,2/2)
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,2/2)
+# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(4,1/2)
+# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(4,1/2)
+# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(4,1/2)
+
+ovn-nbctl acl-del ls1 to-lport 1001 \
+'ip4 && ip4.src == $set1 && ip4.dst == $set1'
+
+OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction | wc -l`])
+OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction.*conjunction | wc -l`])
+OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conj_id | wc -l`])
+
+# Add the ACL back
+ovn-nbctl acl-add ls1 to-lport 1001 \
+'ip4 && ip4.src == $set1 && ip4.dst == $set1' allow
+# Add one more ACL with more overlapping
+ovn-nbctl acl-add ls1 to-lport 1001 \
+'ip4 && ip4.src == $set1 && ip4.dst == {10.0.0.9, 10.0.0.10}' drop
+
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,1/2)
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,1/2)
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(5,1/2)
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(5,1/2)
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(5,1/2)
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,1/2),conjunction(6,1/2)
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(6,1/2)
+# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2)
+# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2)
+# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2)
+
+OVS_WAIT_UNTIL([test 10 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction | wc -l`])
+OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction.*conjunction | wc -l`])
+OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction.*conjunction.*conjunction | wc -l`])
+
+# Remove 10.0.0.7 from address set2. All flows should be updated properly.
+ovn-nbctl set Address_Set set2 \
+addresses=\"10.0.0.8\",\"10.0.0.9\"
+
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(9,1/2)
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(7,1/2)
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(8,1/2)
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(9,1/2)
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(7,1/2),conjunction(8,1/2)
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(9,1/2)
+# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2)
+# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2)
+# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2)
+
+OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction | wc -l`])
+OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction.*conjunction | wc -l`])
+OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction.*conjunction.*conjunction | wc -l`])
+
+# Remove an ACL again
+ovn-nbctl acl-del ls1 to-lport 1001 \
+'ip4 && ip4.src == $set1 && ip4.dst == $set1'
+
+ovn-nbctl --wait=hv sync
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(10,1/2)
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(11,1/2)
+# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(10,1/2),conjunction(11,1/2)
+# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(10,2/2),conjunction(11,2/2)
+# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(10,2/2),conjunction(11,2/2)
+# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(10,2/2),conjunction(11,2/2)
+
+OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction | wc -l`])
+OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction.*conjunction | wc -l`])
+OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction.*conjunction.*conjunction | wc -l`])
+
 OVN_CLEANUP([hv1])
 AT_CLEANUP
 
-- 
2.1.0




More information about the dev mailing list