[ovs-dev] [PATCH ovn v10 6/8] ovn-controller: Handle runtime data changes in flow output engine

numans at ovn.org numans at ovn.org
Mon Jun 8 13:50:30 UTC 2020


From: Numan Siddique <numans at ovn.org>

In order to handle runtime data changes incrementally, the flow outut
runtime data handle should know the changed runtime data.
Runtime data now tracks the changed data for any OVS interface
and SB port binding changes. The tracked data contains a hmap
of tracked datapaths (which changed during runtime data processing.

The flow outout runtime_data handler in this patch doesn't do much
with the tracked data. It returns false if there is tracked data available
so that flow_output run is called. If no tracked data is available
then there is no need for flow computation and the handler returns true.

Next patch in the series processes the tracked data incrementally.

Co-Authored-by: Venkata Anil <anilvenkata at redhat.com>
Signed-off-by: Venkata Anil <anilvenkata at redhat.com>
Signed-off-by: Numan Siddique <numans at ovn.org>
---
 controller/binding.c        | 299 ++++++++++++++++++++++++++++--------
 controller/binding.h        |  37 +++++
 controller/ovn-controller.c | 144 +++++++++++++++--
 tests/ovn-performance.at    |  20 +--
 4 files changed, 413 insertions(+), 87 deletions(-)

diff --git a/controller/binding.c b/controller/binding.c
index c2d9a4c6b..3c226cd7d 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -69,13 +69,26 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
     ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type);
 }
 
+static struct tracked_binding_datapath *tracked_binding_datapath_create(
+    const struct sbrec_datapath_binding *,
+    bool is_new, struct hmap *tracked_dps);
+static struct tracked_binding_datapath *tracked_binding_datapath_find(
+    struct hmap *, const struct sbrec_datapath_binding *);
+static void tracked_binding_datapath_lport_add(
+    const struct sbrec_port_binding *, bool deleted,
+    struct hmap *tracked_datapaths);
+static void update_lport_tracking(const struct sbrec_port_binding *pb,
+                                  bool old_claim, bool new_claim,
+                                  struct hmap *tracked_dp_bindings);
+
 static void
 add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
                      struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
                      struct ovsdb_idl_index *sbrec_port_binding_by_name,
                      const struct sbrec_datapath_binding *datapath,
                      bool has_local_l3gateway, int depth,
-                     struct hmap *local_datapaths)
+                     struct hmap *local_datapaths,
+                     struct hmap *tracked_datapaths)
 {
     uint32_t dp_key = datapath->tunnel_key;
     struct local_datapath *ld = get_local_datapath(local_datapaths, dp_key);
@@ -92,6 +105,11 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
     ld->localnet_port = NULL;
     ld->has_local_l3gateway = has_local_l3gateway;
 
+    if (tracked_datapaths &&
+        !tracked_binding_datapath_find(tracked_datapaths, datapath)) {
+        tracked_binding_datapath_create(datapath, true, tracked_datapaths);
+    }
+
     if (depth >= 100) {
         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
         VLOG_WARN_RL(&rl, "datapaths nested too deep");
@@ -124,7 +142,8 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
                                              sbrec_port_binding_by_datapath,
                                              sbrec_port_binding_by_name,
                                              peer->datapath, false,
-                                             depth + 1, local_datapaths);
+                                             depth + 1, local_datapaths,
+                                             tracked_datapaths);
                     }
                     ld->n_peer_ports++;
                     if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
@@ -147,12 +166,14 @@ add_local_datapath(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
                    struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
                    struct ovsdb_idl_index *sbrec_port_binding_by_name,
                    const struct sbrec_datapath_binding *datapath,
-                   bool has_local_l3gateway, struct hmap *local_datapaths)
+                   bool has_local_l3gateway, struct hmap *local_datapaths,
+                   struct hmap *tracked_datapaths)
 {
     add_local_datapath__(sbrec_datapath_binding_by_key,
                          sbrec_port_binding_by_datapath,
                          sbrec_port_binding_by_name,
-                         datapath, has_local_l3gateway, 0, local_datapaths);
+                         datapath, has_local_l3gateway, 0, local_datapaths,
+                         tracked_datapaths);
 }
 
 static void
@@ -473,23 +494,45 @@ update_ld_localnet_port(const struct sbrec_port_binding *binding_rec,
 
 static void
 update_local_lport_ids(struct sset *local_lport_ids,
-                       const struct sbrec_port_binding *pb)
+                       const struct sbrec_port_binding *pb,
+                       struct hmap *tracked_datapaths)
 {
     char buf[16];
     snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
              pb->datapath->tunnel_key, pb->tunnel_key);
-    sset_add(local_lport_ids, buf);
+    bool added = sset_add(local_lport_ids, buf);
+    if (added && tracked_datapaths) {
+        /* Add the 'pb' to the tracked_datapaths. */
+        tracked_binding_datapath_lport_add(pb, false, tracked_datapaths);
+    }
 }
 
 static void
-remove_local_lport_ids(const struct sbrec_port_binding *binding_rec,
-                       struct sset *local_lport_ids)
+remove_local_lport_ids(const struct sbrec_port_binding *pb,
+                       struct sset *local_lport_ids,
+                       struct hmap *tracked_datapaths)
 {
     char buf[16];
     snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
-                binding_rec->datapath->tunnel_key,
-                binding_rec->tunnel_key);
-    sset_find_and_delete(local_lport_ids, buf);
+             pb->datapath->tunnel_key, pb->tunnel_key);
+    bool deleted = sset_find_and_delete(local_lport_ids, buf);
+    if (deleted && tracked_datapaths) {
+        /* Add the 'pb' to the tracked_datapaths. */
+        tracked_binding_datapath_lport_add(pb, true, tracked_datapaths);
+    }
+}
+
+static bool
+add_lport_to_local_lports(struct sset *local_lports, const char *lport_name)
+{
+    return !!sset_add(local_lports, lport_name);
+}
+
+static bool
+remove_lport_from_local_lports(struct sset *local_lports,
+                               const char *lport_name)
+{
+    return sset_find_and_delete(local_lports, lport_name);
 }
 
 /* Local bindings. binding.c module binds the logical port (represented by
@@ -637,6 +680,77 @@ is_lport_container(const struct sbrec_port_binding *pb)
     return is_lport_vif(pb) && pb->parent_port && pb->parent_port[0];
 }
 
+static struct tracked_binding_datapath *
+tracked_binding_datapath_create(const struct sbrec_datapath_binding *dp,
+                                bool is_new,
+                                struct hmap *tracked_datapaths)
+{
+    struct tracked_binding_datapath *t_dp = xzalloc(sizeof *t_dp);
+    t_dp->dp = dp;
+    t_dp->is_new = is_new;
+    shash_init(&t_dp->lports);
+    hmap_insert(tracked_datapaths, &t_dp->node, uuid_hash(&dp->header_.uuid));
+    return t_dp;
+}
+
+static struct tracked_binding_datapath *
+tracked_binding_datapath_find(struct hmap *tracked_datapaths,
+                              const struct sbrec_datapath_binding *dp)
+{
+    struct tracked_binding_datapath *t_dp;
+    size_t hash = uuid_hash(&dp->header_.uuid);
+    HMAP_FOR_EACH_WITH_HASH (t_dp, node, hash, tracked_datapaths) {
+        if (uuid_equals(&t_dp->dp->header_.uuid, &dp->header_.uuid)) {
+            return t_dp;
+        }
+    }
+
+    return NULL;
+}
+
+static void
+tracked_binding_datapath_lport_add(const struct sbrec_port_binding *pb,
+                                   bool deleted,
+                                   struct hmap *tracked_datapaths)
+{
+    if (!tracked_datapaths) {
+        return;
+    }
+
+    struct tracked_binding_datapath *tracked_dp =
+        tracked_binding_datapath_find(tracked_datapaths, pb->datapath);
+    if (!tracked_dp) {
+        tracked_dp = tracked_binding_datapath_create(pb->datapath, false,
+                                                     tracked_datapaths);
+    }
+
+    /* Check if the lport is already present or not.
+     * If it is already present, then just update the 'pb' and 'deleted'
+     * fields. */
+    struct tracked_binding_lport *lport =
+        shash_find_data(&tracked_dp->lports, pb->logical_port);
+
+    if (!lport) {
+        lport = xmalloc(sizeof *lport);
+        shash_add(&tracked_dp->lports, pb->logical_port, lport);
+    }
+
+    lport->pb = pb;
+    lport->deleted = deleted;
+}
+
+void
+binding_tracked_dp_destroy(struct hmap *tracked_datapaths)
+{
+    struct tracked_binding_datapath *t_dp;
+    HMAP_FOR_EACH_POP (t_dp, node, tracked_datapaths) {
+        shash_destroy_free_data(&t_dp->lports);
+        free(t_dp);
+    }
+
+    hmap_destroy(tracked_datapaths);
+}
+
 /* Corresponds to each Port_Binding.type. */
 enum en_lport_type {
     LP_UNKNOWN,
@@ -690,7 +804,7 @@ static bool
 claim_lport(const struct sbrec_port_binding *pb,
             const struct sbrec_chassis *chassis_rec,
             const struct ovsrec_interface *iface_rec,
-            bool sb_readonly)
+            bool sb_readonly, struct hmap *tracked_datapaths)
 {
     if (pb->chassis != chassis_rec) {
         if (sb_readonly) {
@@ -709,6 +823,10 @@ claim_lport(const struct sbrec_port_binding *pb,
         }
 
         sbrec_port_binding_set_chassis(pb, chassis_rec);
+
+        if (tracked_datapaths) {
+            update_lport_tracking(pb, false, true, tracked_datapaths);
+        }
     }
 
     /* Check if the port encap binding, if any, has changed */
@@ -726,9 +844,14 @@ claim_lport(const struct sbrec_port_binding *pb,
 
 /* Returns false if lport is not released due to 'sb_readonly'.
  * Returns true otherwise.
+ *
+ * This function assumes that that the the 'pb' was claimed
+ * earlier i.e port binding's chassis is set to this chassis.
+ * Caller should make sure that, that's the case.
  */
 static bool
-release_lport(const struct sbrec_port_binding *pb, bool sb_readonly)
+release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,
+              struct hmap *tracked_datapaths)
 {
     if (pb->encap) {
         if (sb_readonly) {
@@ -751,6 +874,7 @@ release_lport(const struct sbrec_port_binding *pb, bool sb_readonly)
         sbrec_port_binding_set_virtual_parent(pb, NULL);
     }
 
+    update_lport_tracking(pb, true, false, tracked_datapaths);
     VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);
     return true;
 }
@@ -796,13 +920,14 @@ is_lbinding_container_parent(struct local_binding *lbinding)
 static bool
 release_local_binding_children(const struct sbrec_chassis *chassis_rec,
                                struct local_binding *lbinding,
-                               bool sb_readonly)
+                               bool sb_readonly,
+                               struct hmap *tracked_dp_bindings)
 {
     struct shash_node *node;
     SHASH_FOR_EACH (node, &lbinding->children) {
         struct local_binding *l = node->data;
         if (is_lbinding_this_chassis(l, chassis_rec)) {
-            if (!release_lport(l->pb, sb_readonly)) {
+            if (!release_lport(l->pb, sb_readonly, tracked_dp_bindings)) {
                 return false;
             }
         }
@@ -817,16 +942,17 @@ release_local_binding_children(const struct sbrec_chassis *chassis_rec,
 
 static bool
 release_local_binding(const struct sbrec_chassis *chassis_rec,
-                      struct local_binding *lbinding, bool sb_readonly)
+                      struct local_binding *lbinding, bool sb_readonly,
+                      struct hmap *tracked_dp_bindings)
 {
     if (!release_local_binding_children(chassis_rec, lbinding,
-                                        sb_readonly)) {
+                                        sb_readonly, tracked_dp_bindings)) {
         return false;
     }
 
     bool retval = true;
     if (is_lbinding_this_chassis(lbinding, chassis_rec)) {
-        retval = release_lport(lbinding->pb, sb_readonly);
+        retval = release_lport(lbinding->pb, sb_readonly, tracked_dp_bindings);
     }
 
     lbinding->pb = NULL;
@@ -847,7 +973,8 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
         if (can_bind) {
             /* We can claim the lport. */
             if (!claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface,
-                             !b_ctx_in->ovnsb_idl_txn)){
+                             !b_ctx_in->ovnsb_idl_txn,
+                             b_ctx_out->tracked_dp_bindings)){
                 return false;
             }
 
@@ -855,8 +982,10 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
                                b_ctx_in->sbrec_port_binding_by_datapath,
                                b_ctx_in->sbrec_port_binding_by_name,
                                pb->datapath, false,
-                               b_ctx_out->local_datapaths);
-            update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
+                               b_ctx_out->local_datapaths,
+                               b_ctx_out->tracked_dp_bindings);
+            update_local_lport_ids(b_ctx_out->local_lport_ids, pb,
+                                   b_ctx_out->tracked_dp_bindings);
             if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) {
                 get_qos_params(pb, qos_map);
             }
@@ -875,7 +1004,8 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
     if (pb->chassis == b_ctx_in->chassis_rec) {
         /* Release the lport if there is no lbinding. */
         if (!lbinding_set || !can_bind) {
-            return release_lport(pb, !b_ctx_in->ovnsb_idl_txn);
+            return release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
+                                 b_ctx_out->tracked_dp_bindings);
         }
     }
 
@@ -962,7 +1092,8 @@ consider_container_lport(const struct sbrec_port_binding *pb,
              * release the container lport, if it was bound earlier. */
             if (is_lbinding_this_chassis(container_lbinding,
                                          b_ctx_in->chassis_rec)) {
-               return release_lport(pb, !b_ctx_in->ovnsb_idl_txn);
+               return release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
+                                    b_ctx_out->tracked_dp_bindings);
             }
 
             return true;
@@ -1037,18 +1168,22 @@ consider_nonvif_lport_(const struct sbrec_port_binding *pb,
                        struct binding_ctx_out *b_ctx_out)
 {
     if (our_chassis) {
-        sset_add(b_ctx_out->local_lports, pb->logical_port);
+        add_lport_to_local_lports(b_ctx_out->local_lports, pb->logical_port);
         add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
                            b_ctx_in->sbrec_port_binding_by_datapath,
                            b_ctx_in->sbrec_port_binding_by_name,
                            pb->datapath, has_local_l3gateway,
-                           b_ctx_out->local_datapaths);
+                           b_ctx_out->local_datapaths,
+                           b_ctx_out->tracked_dp_bindings);
 
-        update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
+        update_local_lport_ids(b_ctx_out->local_lport_ids, pb,
+                               b_ctx_out->tracked_dp_bindings);
         return claim_lport(pb, b_ctx_in->chassis_rec, NULL,
-                           !b_ctx_in->ovnsb_idl_txn);
+                           !b_ctx_in->ovnsb_idl_txn,
+                           b_ctx_out->tracked_dp_bindings);
     } else if (pb->chassis == b_ctx_in->chassis_rec) {
-        return release_lport(pb, !b_ctx_in->ovnsb_idl_txn);
+        return release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
+                             b_ctx_out->tracked_dp_bindings);
     }
 
     return true;
@@ -1084,14 +1219,15 @@ consider_localnet_lport(const struct sbrec_port_binding *pb,
                         struct binding_ctx_out *b_ctx_out,
                         struct hmap *qos_map)
 {
-    /* Add all localnet ports to local_lports so that we allocate ct zones
+    /* Add all localnet ports to local_ifaces so that we allocate ct zones
      * for them. */
-    sset_add(b_ctx_out->local_lports, pb->logical_port);
+    add_lport_to_local_lports(b_ctx_out->local_lports, pb->logical_port);
     if (qos_map && b_ctx_in->ovs_idl_txn) {
         get_qos_params(pb, qos_map);
     }
 
-    update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
+    update_local_lport_ids(b_ctx_out->local_lport_ids, pb,
+                           b_ctx_out->tracked_dp_bindings);
 }
 
 static bool
@@ -1119,7 +1255,10 @@ consider_ha_lport(const struct sbrec_port_binding *pb,
                            b_ctx_in->sbrec_port_binding_by_datapath,
                            b_ctx_in->sbrec_port_binding_by_name,
                            pb->datapath, false,
-                           b_ctx_out->local_datapaths);
+                           b_ctx_out->local_datapaths,
+                           b_ctx_out->tracked_dp_bindings);
+        update_local_lport_ids(b_ctx_out->local_lport_ids, pb,
+                               b_ctx_out->tracked_dp_bindings);
     }
 
     return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out);
@@ -1185,7 +1324,7 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in,
                     ovs_assert(lbinding->type == BT_VIF);
                 }
 
-                sset_add(b_ctx_out->local_lports, iface_id);
+                add_lport_to_local_lports(b_ctx_out->local_lports, iface_id);
                 smap_replace(b_ctx_out->local_iface_ids, iface_rec->name,
                              iface_id);
             }
@@ -1241,7 +1380,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out)
         case LP_PATCH:
         case LP_LOCALPORT:
         case LP_VTEP:
-            update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
+            update_local_lport_ids(b_ctx_out->local_lport_ids, pb, NULL);
             break;
 
         case LP_VIF:
@@ -1409,7 +1548,8 @@ add_local_datapath_peer_port(const struct sbrec_port_binding *pb,
                              b_ctx_in->sbrec_port_binding_by_datapath,
                              b_ctx_in->sbrec_port_binding_by_name,
                              peer->datapath, false,
-                             1, b_ctx_out->local_datapaths);
+                             1, b_ctx_out->local_datapaths,
+                             b_ctx_out->tracked_dp_bindings);
         return;
     }
 
@@ -1471,7 +1611,8 @@ remove_pb_from_local_datapath(const struct sbrec_port_binding *pb,
                               struct binding_ctx_out *b_ctx_out,
                               struct local_datapath *ld)
 {
-    remove_local_lport_ids(pb, b_ctx_out->local_lport_ids);
+    remove_local_lport_ids(pb, b_ctx_out->local_lport_ids,
+                           b_ctx_out->tracked_dp_bindings);
     if (!strcmp(pb->type, "patch") ||
         !strcmp(pb->type, "l3gateway")) {
         remove_local_datapath_peer_port(pb, ld, b_ctx_out->local_datapaths);
@@ -1489,6 +1630,18 @@ remove_pb_from_local_datapath(const struct sbrec_port_binding *pb,
     }
 }
 
+static void
+update_lport_tracking(const struct sbrec_port_binding *pb,
+                      bool old_claim, bool new_claim,
+                      struct hmap *tracked_dp_bindings)
+{
+    if (!tracked_dp_bindings || !pb || (old_claim == new_claim)) {
+        return;
+    }
+
+    tracked_binding_datapath_lport_add(pb, old_claim, tracked_dp_bindings);
+}
+
 /* Considers the ovs iface 'iface_rec' for claiming.
  * This function should be called if the external_ids:iface-id
  * and 'ofport' are set for the 'iface_rec'.
@@ -1501,9 +1654,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec,
                      const char *iface_id,
                      struct binding_ctx_in *b_ctx_in,
                      struct binding_ctx_out *b_ctx_out,
-                     struct hmap *qos_map)
+                     struct hmap *qos_map,
+                     bool *local_lports_changed)
 {
-    sset_add(b_ctx_out->local_lports, iface_id);
+    if (add_lport_to_local_lports(b_ctx_out->local_lports, iface_id)) {
+        *local_lports_changed = true;
+    }
+
     smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id);
 
     struct local_binding *lbinding =
@@ -1566,7 +1723,8 @@ static bool
 consider_iface_release(const struct ovsrec_interface *iface_rec,
                        const char *iface_id,
                        struct binding_ctx_in *b_ctx_in,
-                       struct binding_ctx_out *b_ctx_out, bool *changed)
+                       struct binding_ctx_out *b_ctx_out, bool *changed,
+                       bool *local_lports_changed)
 {
     struct local_binding *lbinding;
     lbinding = local_binding_find(b_ctx_out->local_bindings,
@@ -1585,7 +1743,8 @@ consider_iface_release(const struct ovsrec_interface *iface_rec,
          * lbinding->iface.
          * Cannot access these members of lbinding after this call. */
         if (!release_local_binding(b_ctx_in->chassis_rec, lbinding,
-                                   !b_ctx_in->ovnsb_idl_txn)) {
+                                   !b_ctx_in->ovnsb_idl_txn,
+                                   b_ctx_out->tracked_dp_bindings)) {
             return false;
         }
 
@@ -1598,7 +1757,9 @@ consider_iface_release(const struct ovsrec_interface *iface_rec,
         local_binding_delete(b_ctx_out->local_bindings, lbinding);
     }
 
-    sset_find_and_delete(b_ctx_out->local_lports, iface_id);
+    if (remove_lport_from_local_lports(b_ctx_out->local_lports, iface_id)) {
+        *local_lports_changed = true;
+    }
     smap_remove(b_ctx_out->local_iface_ids, iface_rec->name);
 
     return true;
@@ -1630,6 +1791,8 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in,
     bool handled = true;
     *changed = false;
 
+    *b_ctx_out->local_lports_changed = false;
+
     /* Run the tracked interfaces loop twice. One to handle deleted
      * changes. And another to handle add/update changes.
      * This will ensure correctness.
@@ -1685,7 +1848,8 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in,
 
         if (cleared_iface_id) {
             handled = consider_iface_release(iface_rec, cleared_iface_id,
-                                             b_ctx_in, b_ctx_out, changed);
+                                             b_ctx_in, b_ctx_out, changed,
+                                             b_ctx_out->local_lports_changed);
         }
 
         if (!handled) {
@@ -1727,7 +1891,8 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in,
         if (iface_id && ofport > 0) {
             *changed = true;
             handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in,
-                                           b_ctx_out, qos_map_ptr);
+                                           b_ctx_out, qos_map_ptr,
+                                           b_ctx_out->local_lports_changed);
             if (!handled) {
                 break;
             }
@@ -1792,8 +1957,7 @@ static bool
 handle_deleted_vif_lport(const struct sbrec_port_binding *pb,
                          enum en_lport_type lport_type,
                          struct binding_ctx_in *b_ctx_in,
-                         struct binding_ctx_out *b_ctx_out,
-                         bool *changed)
+                         struct binding_ctx_out *b_ctx_out)
 {
     struct local_binding *lbinding =
         get_lbinding_for_lport(pb, lport_type, b_ctx_out);
@@ -1804,13 +1968,12 @@ handle_deleted_vif_lport(const struct sbrec_port_binding *pb,
          * clear the 'chassis' column of 'pb'. But we need to do
          * for the local_binding's children. */
         if (lbinding->type == BT_VIF &&
-                !release_local_binding_children(b_ctx_in->chassis_rec,
-                                                lbinding,
-                                                !b_ctx_in->ovnsb_idl_txn)) {
+                !release_local_binding_children(
+                    b_ctx_in->chassis_rec, lbinding,
+                    !b_ctx_in->ovnsb_idl_txn,
+                    b_ctx_out->tracked_dp_bindings)) {
             return false;
         }
-
-        *changed = true;
     }
 
     handle_deleted_lport(pb, b_ctx_in, b_ctx_out);
@@ -1822,8 +1985,7 @@ handle_updated_vif_lport(const struct sbrec_port_binding *pb,
                          enum en_lport_type lport_type,
                          struct binding_ctx_in *b_ctx_in,
                          struct binding_ctx_out *b_ctx_out,
-                         struct hmap *qos_map,
-                         bool *changed)
+                         struct hmap *qos_map)
 {
     bool claimed = (pb->chassis == b_ctx_in->chassis_rec);
     bool handled = true;
@@ -1841,14 +2003,10 @@ handle_updated_vif_lport(const struct sbrec_port_binding *pb,
     }
 
     bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec);
-    bool changed_ = (claimed != now_claimed);
-
-    if (changed_) {
-        *changed = true;
-    }
+    bool changed = (claimed != now_claimed);
 
     if (lport_type == LP_VIRTUAL ||
-        (lport_type == LP_VIF && is_lport_container(pb)) || !changed_) {
+        (lport_type == LP_VIF && is_lport_container(pb)) || !changed) {
         return true;
     }
 
@@ -1898,7 +2056,7 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
         enum en_lport_type lport_type = get_lport_type(pb);
         if (lport_type == LP_VIF || lport_type == LP_VIRTUAL) {
             handled = handle_deleted_vif_lport(pb, lport_type, b_ctx_in,
-                                                b_ctx_out, changed);
+                                                b_ctx_out);
         } else {
             handle_deleted_lport(pb, b_ctx_in, b_ctx_out);
         }
@@ -1933,15 +2091,14 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
         case LP_VIF:
         case LP_VIRTUAL:
             handled = handle_updated_vif_lport(pb, lport_type, b_ctx_in,
-                                               b_ctx_out, qos_map_ptr,
-                                               changed);
+                                               b_ctx_out, qos_map_ptr);
             break;
 
         case LP_PATCH:
         case LP_LOCALPORT:
         case LP_VTEP:
-            *changed = true;
-            update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
+            update_local_lport_ids(b_ctx_out->local_lport_ids, pb,
+                                   b_ctx_out->tracked_dp_bindings);
             if (lport_type ==  LP_PATCH) {
                 /* Add the peer datapath to the local datapaths if it's
                  * not present yet.
@@ -1950,30 +2107,32 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
                     add_local_datapath_peer_port(pb, b_ctx_in, b_ctx_out, ld);
                 }
             }
+
+            if (lport_type == LP_VTEP) {
+                /* VTEP lports are claimed/released by ovn-controller-vteps.
+                 * We are not sure what changed. So set *changed to true
+                 * for any changes to vtep lports. */
+                *changed = true;
+            }
             break;
 
         case LP_L2GATEWAY:
-            *changed = true;
             handled = consider_l2gw_lport(pb, b_ctx_in, b_ctx_out);
             break;
 
         case LP_L3GATEWAY:
-            *changed = true;
             handled = consider_l3gw_lport(pb, b_ctx_in, b_ctx_out);
             break;
 
         case LP_CHASSISREDIRECT:
-            *changed = true;
             handled = consider_cr_lport(pb, b_ctx_in, b_ctx_out);
             break;
 
         case LP_EXTERNAL:
-            *changed = true;
             handled = consider_external_lport(pb, b_ctx_in, b_ctx_out);
             break;
 
         case LP_LOCALNET: {
-            *changed = true;
             consider_localnet_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr);
 
             struct shash bridge_mappings =
@@ -2008,6 +2167,10 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
         }
     }
 
+    if (handled && !hmap_is_empty(b_ctx_out->tracked_dp_bindings)) {
+        *changed = true;
+    }
+
     destroy_qos_map(&qos_map);
     return handled;
 }
diff --git a/controller/binding.h b/controller/binding.h
index 21118ecd4..2974ebb30 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -19,6 +19,9 @@
 
 #include <stdbool.h>
 #include "openvswitch/shash.h"
+#include "openvswitch/hmap.h"
+#include "openvswitch/uuid.h"
+#include "openvswitch/list.h"
 
 struct hmap;
 struct ovsdb_idl;
@@ -54,10 +57,28 @@ struct binding_ctx_in {
 struct binding_ctx_out {
     struct hmap *local_datapaths;
     struct shash *local_bindings;
+
     struct sset *local_lports;
+
+    /* If the sset local_lports is modified, this is set to true by
+     * the callee. */
+    bool *local_lports_changed;
+
+    /* sset of local lport ids in the format
+     * <datapath-tunnel-key>_<port-tunnel-key>. */
     struct sset *local_lport_ids;
+
     struct sset *egress_ifaces;
+
+    /* smap of OVS interface name as key and
+     * OVS interface external_ids:iface-id as value. */
     struct smap *local_iface_ids;
+
+    /* hmap of 'struct tracked_binding_datapath' which the
+     * callee (binding_handle_ovs_interface_changes and
+     * binding_handle_port_binding_changes) fills in for
+     * the changed datapaths and port bindings. */
+    struct hmap *tracked_dp_bindings;
 };
 
 enum local_binding_type {
@@ -82,6 +103,21 @@ local_binding_find(struct shash *local_bindings, const char *name)
     return shash_find_data(local_bindings, name);
 }
 
+/* Represents a tracked binding logical port. */
+struct tracked_binding_lport {
+    const struct sbrec_port_binding *pb;
+    struct ovs_list list_node;
+    bool deleted;
+};
+
+/* Represent a tracked binding datapath. */
+struct tracked_binding_datapath {
+    struct hmap_node node;
+    const struct sbrec_datapath_binding *dp;
+    bool is_new;
+    struct shash lports; /* shash of struct tracked_binding_lport. */
+};
+
 void binding_register_ovs_idl(struct ovsdb_idl *);
 void binding_run(struct binding_ctx_in *, struct binding_ctx_out *);
 bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
@@ -96,4 +132,5 @@ bool binding_handle_ovs_interface_changes(struct binding_ctx_in *,
 bool binding_handle_port_binding_changes(struct binding_ctx_in *,
                                          struct binding_ctx_out *,
                                          bool *changed);
+void binding_tracked_dp_destroy(struct hmap *tracked_datapaths);
 #endif /* controller/binding.h */
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 795a1c297..98652866c 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -973,10 +973,88 @@ struct ed_type_runtime_data {
     struct sset local_lport_ids;
     struct sset active_tunnels;
 
+    /* runtime data engine private data. */
     struct sset egress_ifaces;
     struct smap local_iface_ids;
+
+    /* Tracked data. See below for more details and comments. */
+    bool tracked;
+    bool local_lports_changed;
+    struct hmap tracked_dp_bindings;
 };
 
+/* struct ed_type_runtime_data has the below members for tracking the
+ * changes done to the runtime_data engine by the runtime_data engine
+ * handlers. Since this engine is an input to the flow_output engine,
+ * the flow output runtime data handler will make use of this tracked data.
+ *
+ *  ------------------------------------------------------------------------
+ * |                      | This is a hmap of                               |
+ * |                      | 'struct tracked_binding_datapath' defined in    |
+ * |                      | binding.h. Runtime data handlers for OVS        |
+ * |                      | Interface and Port Binding changes store the    |
+ * | @tracked_dp_bindings | changed datapaths (datapaths added/removed from |
+ * |                      | local_datapaths) and changed port bindings      |
+ * |                      | (added/updated/deleted in 'local_bindings').    |
+ * |                      | So any changes to the runtime data -            |
+ * |                      | local_datapaths and local_bindings is captured  |
+ * |                      | here.                                           |
+ *  ------------------------------------------------------------------------
+ * |                      | This is a bool which represents if the runtime  |
+ * |                      | data 'local_lports' changed by the runtime data |
+ * |                      | handlers for OVS Interface and Port Binding     |
+ * |                      | changes. If 'local_lports' is updated and also  |
+ * |                      | results in any port binding updates, it is      |
+ * |@local_lports_changed | captured in the @tracked_dp_bindings. So there  |
+ * |                      | is no need to capture the changes in the        |
+ * |                      | local_lports. If @local_lports_changed is true  |
+ * |                      | but without anydata in the @tracked_dp_bindings,|
+ * |                      | it means we needto only update the SB monitor   |
+ * |                      | clauses and there isno need for any flow        |
+ * |                      | (re)computations.                               |
+ *  ------------------------------------------------------------------------
+ * |                      | This represents if the data was tracked or not  |
+ * |                      | by the runtime data handlers during the engine  |
+ * |   @tracked           | run. If the runtime data recompute is           |
+ * |                      | triggered, it means there is no tracked data.   |
+ *  ------------------------------------------------------------------------
+ *
+ *
+ * The changes to the following runtime_data variables are not tracked.
+ *
+ *  ---------------------------------------------------------------------
+ * | local_datapaths  | The changes to these runtime data is captured in |
+ * | local_bindings   | the @tracked_dp_bindings indirectly and hence it |
+ * | local_lport_ids  | is not tracked explicitly.                       |
+ *  ---------------------------------------------------------------------
+ * | local_iface_ids  | This is used internally within the runtime data  |
+ * | egress_ifaces    | engine (used only in binding.c) and hence there  |
+ * |                  | there is no need to track.                       |
+ *  ---------------------------------------------------------------------
+ * |                  | Active tunnels is built in the                   |
+ * |                  | bfd_calculate_active_tunnels() for the tunnel    |
+ * |                  | OVS interfaces. Any changes to non VIF OVS       |
+ * |                  | interfaces results in triggering the full        |
+ * | active_tunnels   | recompute of runtime data engine and hence there |
+ * |                  | the tracked data doesn't track it. When we       |
+ * |                  | support handling changes to non VIF OVS          |
+ * |                  | interfaces we need to track the changes to the   |
+ * |                  | active tunnels.                                  |
+ *  ---------------------------------------------------------------------
+ *
+ */
+
+static void
+en_runtime_data_clear_tracked_data(void *data_)
+{
+    struct ed_type_runtime_data *data = data_;
+
+    binding_tracked_dp_destroy(&data->tracked_dp_bindings);
+    hmap_init(&data->tracked_dp_bindings);
+    data->local_lports_changed = false;
+    data->tracked = false;
+}
+
 static void *
 en_runtime_data_init(struct engine_node *node OVS_UNUSED,
                      struct engine_arg *arg OVS_UNUSED)
@@ -990,6 +1068,10 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED,
     sset_init(&data->egress_ifaces);
     smap_init(&data->local_iface_ids);
     local_bindings_init(&data->local_bindings);
+
+    /* init the tracked data. */
+    hmap_init(&data->tracked_dp_bindings);
+
     return data;
 }
 
@@ -1012,6 +1094,8 @@ en_runtime_data_cleanup(void *data)
     }
     hmap_destroy(&rt_data->local_datapaths);
     local_bindings_destroy(&rt_data->local_bindings);
+
+    en_runtime_data_clear_tracked_data(data);
 }
 
 static void
@@ -1092,6 +1176,8 @@ init_binding_ctx(struct engine_node *node,
     b_ctx_out->egress_ifaces = &rt_data->egress_ifaces;
     b_ctx_out->local_bindings = &rt_data->local_bindings;
     b_ctx_out->local_iface_ids = &rt_data->local_iface_ids;
+    b_ctx_out->tracked_dp_bindings = NULL;
+    b_ctx_out->local_lports_changed = NULL;
 }
 
 static void
@@ -1103,6 +1189,13 @@ en_runtime_data_run(struct engine_node *node, void *data)
     struct sset *local_lport_ids = &rt_data->local_lport_ids;
     struct sset *active_tunnels = &rt_data->active_tunnels;
 
+    /* Clear the tracked data. Even though the tracked data
+     * gets cleared in the beginning of engine_init_run(),
+     * any of the runtime data handler might have set some tracked
+     * data and later another runtime data handler might return false
+     * resulting full recompute of runtime engine. */
+    en_runtime_data_clear_tracked_data(data);
+
     static bool first_run = true;
     if (first_run) {
         /* don't cleanup since there is no data yet */
@@ -1158,6 +1251,9 @@ runtime_data_ovs_interface_handler(struct engine_node *node, void *data)
     struct binding_ctx_in b_ctx_in;
     struct binding_ctx_out b_ctx_out;
     init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out);
+    rt_data->tracked = true;
+    b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings;
+    b_ctx_out.local_lports_changed = &rt_data->local_lports_changed;
 
     bool changed = false;
     if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out,
@@ -1190,6 +1286,9 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data)
         return false;
     }
 
+    rt_data->tracked = true;
+    b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings;
+
     bool changed = false;
     if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out,
                                              &changed)) {
@@ -1797,6 +1896,39 @@ flow_output_port_groups_handler(struct engine_node *node, void *data)
     return _flow_output_resource_ref_handler(node, data, REF_TYPE_PORTGROUP);
 }
 
+static bool
+flow_output_runtime_data_handler(struct engine_node *node,
+                                 void *data OVS_UNUSED)
+{
+    struct ed_type_runtime_data *rt_data =
+        engine_get_input_data("runtime_data", node);
+
+    /* There is no tracked data. Fall back to full recompute of
+     * flow_output. */
+    if (!rt_data->tracked) {
+        return false;
+    }
+
+    if (!hmap_is_empty(&rt_data->tracked_dp_bindings)) {
+        /* We are not yet handling the tracked datapath binding
+         * changes. Return false to trigger full recompute. */
+        return false;
+    }
+
+    if (rt_data->local_lports_changed) {
+        engine_set_node_state(node, EN_UPDATED);
+    }
+
+    return true;
+}
+
+static bool
+flow_output_noop_handler(struct engine_node *node OVS_UNUSED,
+                         void *data OVS_UNUSED)
+{
+    return true;
+}
+
 /* Engine node en_physical_flow_changes indicates whether
  * there is a need to
  *   - recompute only physical flows or
@@ -1908,13 +2040,6 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data)
     return true;
 }
 
-static bool
-flow_output_noop_handler(struct engine_node *node OVS_UNUSED,
-                         void *data OVS_UNUSED)
-{
-    return true;
-}
-
 struct ovn_controller_exit_args {
     bool *exiting;
     bool *restart;
@@ -2036,7 +2161,7 @@ main(int argc, char *argv[])
 
     /* Define inc-proc-engine nodes. */
     ENGINE_NODE_CUSTOM_DATA(ct_zones, "ct_zones");
-    ENGINE_NODE(runtime_data, "runtime_data");
+    ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data");
     ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve");
     ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected");
     ENGINE_NODE_WITH_CLEAR_TRACK_DATA(physical_flow_changes,
@@ -2073,7 +2198,8 @@ main(int argc, char *argv[])
                      flow_output_addr_sets_handler);
     engine_add_input(&en_flow_output, &en_port_groups,
                      flow_output_port_groups_handler);
-    engine_add_input(&en_flow_output, &en_runtime_data, NULL);
+    engine_add_input(&en_flow_output, &en_runtime_data,
+                     flow_output_runtime_data_handler);
     engine_add_input(&en_flow_output, &en_mff_ovn_geneve, NULL);
     engine_add_input(&en_flow_output, &en_physical_flow_changes,
                      flow_output_physical_flow_changes_handler);
diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
index 5cc1960b6..08acd3bae 100644
--- a/tests/ovn-performance.at
+++ b/tests/ovn-performance.at
@@ -286,11 +286,11 @@ for i in 1 2; do
         [hv1 hv2], [lflow_run],
         [ovn-nbctl --wait=hv lsp-set-type $lsp router]
     )
-    OVN_CONTROLLER_EXPECT_HIT(
+    OVN_CONTROLLER_EXPECT_NO_HIT(
         [hv1 hv2], [lflow_run],
         [ovn-nbctl --wait=hv lsp-set-options $lsp router-port=$lrp]
     )
-    OVN_CONTROLLER_EXPECT_HIT(
+    OVN_CONTROLLER_EXPECT_NO_HIT(
         [hv1 hv2], [lflow_run],
         [ovn-nbctl --wait=hv lsp-set-addresses $lsp router]
     )
@@ -416,15 +416,15 @@ OVN_CONTROLLER_EXPECT_NO_HIT(
     [ovn-nbctl --wait=hv destroy Port_Group pg1]
 )
 
-for i in 1 2; do
-    ls=ls$i
+OVN_CONTROLLER_EXPECT_HIT(
+    [hv1 hv2], [lflow_run],
+    [ovn-nbctl --wait=hv ls-del ls1]
+)
 
-    # Delete switch $ls
-    OVN_CONTROLLER_EXPECT_HIT(
-        [hv1 hv2], [lflow_run],
-        [ovn-nbctl --wait=hv ls-del $ls]
-    )
-done
+OVN_CONTROLLER_EXPECT_NO_HIT(
+    [hv1 hv2], [lflow_run],
+    [ovn-nbctl --wait=hv ls-del ls2]
+)
 
 # Delete router lr1
 OVN_CONTROLLER_EXPECT_NO_HIT(
-- 
2.26.2



More information about the dev mailing list