It's valid that port_groups contain non-vif ports, they can actually
contain any type of logical_switch_port.

Also, there's no need to allocate a new sset containing the local ports'
names every time the I-P engine processes a change, we can maintain a
sset and incrementally update it when port bindings are added/removed.

Reported-at: https://github.com/ovn-org/ovn/pull/61#issuecomment-865094163
Reported-by: Antonio Ojea <ao...@redhat.com>
Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow explosion 
problem.")
Signed-off-by: Dumitru Ceara <dce...@redhat.com>
---
 controller/binding.c        | 25 +++++--------------------
 controller/binding.h        | 11 ++---------
 controller/ovn-controller.c | 24 +++++++-----------------
 3 files changed, 14 insertions(+), 46 deletions(-)

diff --git a/controller/binding.c b/controller/binding.c
index 7fde0fdbb..1f6188e0d 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -549,6 +549,7 @@ update_local_lport_ids(const struct sbrec_port_binding *pb,
             tracked_binding_datapath_lport_add(pb, b_ctx->tracked_dp_bindings);
         }
     }
+    sset_add(&b_ctx->lbinding_data->port_bindings, pb->logical_port);
 }
 
 /* Remove a port binding id from the set of local lport IDs. Also track if
@@ -569,6 +570,8 @@ remove_local_lport_ids(const struct sbrec_port_binding *pb,
             tracked_binding_datapath_lport_add(pb, b_ctx->tracked_dp_bindings);
         }
     }
+    sset_find_and_delete(&b_ctx->lbinding_data->port_bindings,
+                         pb->logical_port);
 }
 
 /* Corresponds to each Port_Binding.type. */
@@ -683,6 +686,7 @@ local_binding_data_init(struct local_binding_data 
*lbinding_data)
 {
     shash_init(&lbinding_data->bindings);
     shash_init(&lbinding_data->lports);
+    sset_init(&lbinding_data->port_bindings);
 }
 
 void
@@ -702,6 +706,7 @@ local_binding_data_destroy(struct local_binding_data 
*lbinding_data)
         shash_delete(&lbinding_data->bindings, node);
     }
 
+    sset_destroy(&lbinding_data->port_bindings);
     shash_destroy(&lbinding_data->lports);
     shash_destroy(&lbinding_data->bindings);
 }
@@ -2926,23 +2931,3 @@ cleanup:
 
     return b_lport;
 }
-
-struct sset *
-binding_collect_local_binding_lports(struct local_binding_data *lbinding_data)
-{
-    struct sset *lports = xzalloc(sizeof *lports);
-    sset_init(lports);
-    struct shash_node *shash_node;
-    SHASH_FOR_EACH (shash_node, &lbinding_data->lports) {
-        struct binding_lport *b_lport = shash_node->data;
-        sset_add(lports, b_lport->name);
-    }
-    return lports;
-}
-
-void
-binding_destroy_local_binding_lports(struct sset *lports)
-{
-    sset_destroy(lports);
-    free(lports);
-}
diff --git a/controller/binding.h b/controller/binding.h
index 8f3289476..7a35faa0d 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -22,6 +22,7 @@
 #include "openvswitch/hmap.h"
 #include "openvswitch/uuid.h"
 #include "openvswitch/list.h"
+#include "sset.h"
 
 struct hmap;
 struct ovsdb_idl;
@@ -93,6 +94,7 @@ struct binding_ctx_out {
 struct local_binding_data {
     struct shash bindings;
     struct shash lports;
+    struct sset port_bindings;
 };
 
 void local_binding_data_init(struct local_binding_data *);
@@ -133,13 +135,4 @@ bool binding_handle_port_binding_changes(struct 
binding_ctx_in *,
 void binding_tracked_dp_destroy(struct hmap *tracked_datapaths);
 
 void binding_dump_local_bindings(struct local_binding_data *, struct ds *);
-
-/* Generates a sset of lport names from local_binding_data.
- * Note: the caller is responsible for destroying and freeing the returned
- * sset, by calling binding_detroy_local_binding_lports(). */
-struct sset *binding_collect_local_binding_lports(struct local_binding_data *);
-
-/* Destroy and free the lports sset returned by
- * binding_collect_local_binding_lports(). */
-void binding_destroy_local_binding_lports(struct sset *lports);
 #endif /* controller/binding.h */
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 3968ef059..5675b97dd 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1635,11 +1635,8 @@ en_port_groups_run(struct engine_node *node, void *data)
     struct ed_type_runtime_data *rt_data =
         engine_get_input_data("runtime_data", node);
 
-    struct sset *local_b_lports = binding_collect_local_binding_lports(
-        &rt_data->lbinding_data);
-    port_groups_init(pg_table, local_b_lports, &pg->port_group_ssets,
-                     &pg->port_groups_cs_local);
-    binding_destroy_local_binding_lports(local_b_lports);
+    port_groups_init(pg_table, &rt_data->lbinding_data.port_bindings,
+                     &pg->port_group_ssets, &pg->port_groups_cs_local);
 
     engine_set_node_state(node, EN_UPDATED);
 }
@@ -1656,12 +1653,9 @@ port_groups_sb_port_group_handler(struct engine_node 
*node, void *data)
     struct ed_type_runtime_data *rt_data =
         engine_get_input_data("runtime_data", node);
 
-    struct sset *local_b_lports = binding_collect_local_binding_lports(
-        &rt_data->lbinding_data);
-    port_groups_update(pg_table, local_b_lports, &pg->port_group_ssets,
-                       &pg->port_groups_cs_local, &pg->new, &pg->deleted,
-                       &pg->updated);
-    binding_destroy_local_binding_lports(local_b_lports);
+    port_groups_update(pg_table, &rt_data->lbinding_data.port_bindings,
+                       &pg->port_group_ssets, &pg->port_groups_cs_local,
+                       &pg->new, &pg->deleted, &pg->updated);
 
     if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) ||
             !sset_is_empty(&pg->updated)) {
@@ -1694,9 +1688,6 @@ port_groups_runtime_data_handler(struct engine_node 
*node, void *data)
         goto out;
     }
 
-    struct sset *local_b_lports = binding_collect_local_binding_lports(
-        &rt_data->lbinding_data);
-
     const struct sbrec_port_group *pg_sb;
     SBREC_PORT_GROUP_TABLE_FOR_EACH (pg_sb, pg_table) {
         struct sset *pg_lports = shash_find_data(&pg->port_group_ssets,
@@ -1723,13 +1714,12 @@ port_groups_runtime_data_handler(struct engine_node 
*node, void *data)
         if (need_update) {
             expr_const_sets_add_strings(&pg->port_groups_cs_local, pg_sb->name,
                                         (const char *const *) pg_sb->ports,
-                                        pg_sb->n_ports, local_b_lports);
+                                        pg_sb->n_ports,
+                                        &rt_data->lbinding_data.port_bindings);
             sset_add(&pg->updated, pg_sb->name);
         }
     }
 
-    binding_destroy_local_binding_lports(local_b_lports);
-
 out:
     if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) ||
             !sset_is_empty(&pg->updated)) {
-- 
2.27.0

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to