[ovs-dev] [PATCH] ovn-controller: Clean up bindings handling.

Mickey Spiegel emspiege at us.ibm.com
Thu Jul 14 04:02:28 UTC 2016


>To: dev at openvswitch.org
>From: Russell Bryant 
>Sent by: "dev" 
>Date: 07/13/2016 02:53PM
>Subject: [ovs-dev] [PATCH] ovn-controller: Clean up bindings handling.
>
>Remove the global set of logical port IDs called 'all_lports'.  This is
>no longer used for anything after conntrack ID assignment was moved out
>of binding.c.
>
>Remove the global smap of logical port IDs to ovsrec_interface records.
>We can't persist references to these records, as we may be holding
>references to freed memory.  Instead, replace it with a new global sset
>of logical port IDs called 'local_ids'.  This is used to track when
>interfaces have been added or removed.
>
>Found by inspection.
>
>Signed-off-by: Russell Bryant <russell at ovn.org>
>---
> ovn/controller/binding.c | 101 ++++++++++++++++++++++++++---------------------
> 1 file changed, 56 insertions(+), 45 deletions(-)
>
>diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
>index 4704226..8e6d17a 100644
>--- a/ovn/controller/binding.c
>+++ b/ovn/controller/binding.c
>@@ -27,8 +27,10 @@
> 
> VLOG_DEFINE_THIS_MODULE(binding);
> 
>-static struct sset all_lports = SSET_INITIALIZER(&all_lports);
>+/* A set of the iface-id values of local interfaces on this chassis.
>*/
>+static struct sset local_ids = SSET_INITIALIZER(&local_ids);
> 
>+/* When this gets set to true, the next run will re-check all binding records. */
> static bool process_full_binding = false;
> 
> void
>@@ -60,14 +62,13 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> }
> 
> static bool
>-get_local_iface_ids(const struct ovsrec_bridge *br_int, struct shash *lports)

While your local_ids sset is the right thing to do for persistence, as long as
get_local_iface_ids is walking all br_int interfaces, why not construct a
non-persistent shash of lports, in order to keep performance linear with the
number of ports for the case of process_full_binding?

>+get_local_iface_ids(const struct ovsrec_bridge *br_int)
> {
>         int i;
>     bool changed = false;
> 
>-    /* A local copy of ports that we can use to compare with the persisted
>-    * list. */
>-    struct shash local_ports = SHASH_INITIALIZER(&local_ports);
>+    struct sset old_local_ids = SSET_INITIALIZER(&old_local_ids);
>+    sset_clone(&old_local_ids, &local_ids);
> 
>     for (i = 0; i < br_int->n_ports; i++) {
>         const struct ovsrec_port *port_rec = br_int->ports[i];
>@@ -86,25 +87,21 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int, struct shash *lports)
>             if (!iface_id) {
>                 continue;
>             }
>-            shash_add(&local_ports, iface_id, iface_rec);
>-            if (!shash_find(lports, iface_id)) {
>-                shash_add(lports, iface_id, iface_rec);
>+            if (!sset_find_and_delete(&old_local_ids, iface_id)) {
>+                sset_add(&local_ids, iface_id);
>                 changed = true;
>             }
>-            if (!sset_find(&all_lports, iface_id)) {
>-                sset_add(&all_lports, iface_id);
>-                binding_reset_processing();
>-            }
>         }
>     }
>-    struct shash_node *iter, *next;
>-    SHASH_FOR_EACH_SAFE(iter, next, lports) {
>-        if (!shash_find_and_delete(&local_ports, iter->name)) {
>-            shash_delete(lports, iter);
>-            changed = true;
>-        }
>+
>+    /* Any item left in old_local_ids is an ID for an interface
>+     * that has been removed. */
>+    if (!changed && !sset_is_empty(&old_local_ids)) {
>+        changed = true;
>     }
>-    shash_destroy(&local_ports);
>+
>+    sset_destroy(&old_local_ids);
>+
>     return changed;
> }
> 
>@@ -129,7 +126,6 @@ static void
> remove_local_datapath(struct hmap *local_datapaths, struct local_datapath *ld)
> {
>     if (ld->logical_port) {
>-        sset_find_and_delete(&all_lports, ld->logical_port);
>         free(ld->logical_port);
>         ld->logical_port = NULL;
>     }
>@@ -187,21 +183,48 @@ update_qos(const struct ovsrec_interface *iface_rec,
>     ovsrec_interface_set_ingress_policing_burst(iface_rec, MAX(0, burst));
> }
> 
>+/* Return an ovsrec_interface that has an iface-id matching lport. */
>+static const struct ovsrec_interface *
>+iface_for_lport(const struct ovsrec_bridge *br_int, const char *lport)
>+{

If you create a non-persistent shash of lports in get_local_iface_ids, then
this would not be necessary.

>+    int i;
>+
>+    for (i = 0; i < br_int->n_ports; i++) {
>+        const struct ovsrec_port *port_rec = br_int->ports[i];
>+        const char *iface_id;
>+        int j;
>+
>+        if (!strcmp(port_rec->name, br_int->name)) {
>+            continue;
>+        }
>+
>+        for (j = 0; j < port_rec->n_interfaces; j++) {
>+            const struct ovsrec_interface *iface_rec;
>+
>+            iface_rec = port_rec->interfaces[j];
>+            iface_id = smap_get(&iface_rec->external_ids, "iface-id");
>+            if (iface_id && !strcmp(iface_id, lport)) {
>+                return iface_rec;
>+            }
>+        }
>+    }
>+
>+    return NULL;
>+}
>+
> static void
>-consider_local_datapath(struct controller_ctx *ctx, struct shash *lports,
>+consider_local_datapath(struct controller_ctx *ctx,
>                         const struct sbrec_chassis *chassis_rec,
>                         const struct sbrec_port_binding *binding_rec,
>-                        struct hmap *local_datapaths)
>+                        struct hmap *local_datapaths,
>+                        const struct ovsrec_bridge *br_int)
> {
>     const struct ovsrec_interface *iface_rec
>-        = shash_find_data(lports, binding_rec->logical_port);
>+        = iface_for_lport(br_int, binding_rec->logical_port);

and then you could use the shash here instead of the new iface_for_lport.

Mickey


>+
>     if (iface_rec
>         || (binding_rec->parent_port && binding_rec->parent_port[0] &&
>-            sset_contains(&all_lports, binding_rec->parent_port))) {
>-        if (binding_rec->parent_port && binding_rec->parent_port[0]) {
>-            /* Add child logical port to the set of all local ports. */
>-            sset_add(&all_lports, binding_rec->logical_port);
>-        }
>+            sset_contains(&local_ids, binding_rec->parent_port))) {
>         add_local_datapath(local_datapaths, binding_rec,
>                            &binding_rec->header_.uuid);
>         if (iface_rec && ctx->ovs_idl_txn) {
>@@ -242,7 +265,6 @@ consider_local_datapath(struct controller_ctx *ctx, struct shash *lports,
>             VLOG_INFO("Claiming l2gateway port %s for this chassis.",
>                       binding_rec->logical_port);
>             sbrec_port_binding_set_chassis(binding_rec, chassis_rec);
>-            sset_add(&all_lports, binding_rec->logical_port);
>             add_local_datapath(local_datapaths, binding_rec,
>                                &binding_rec->header_.uuid);
>         }
>@@ -253,20 +275,9 @@ consider_local_datapath(struct controller_ctx *ctx, struct shash *lports,
>                       binding_rec->logical_port);
>             sbrec_port_binding_set_chassis(binding_rec, NULL);
>         }
>-    } else if (!binding_rec->chassis
>-               && !strcmp(binding_rec->type, "localnet")) {
>-        /* Localnet ports will never be bound to a chassis, but we want
>-         * to list them in all_lports because we want to allocate
>-         * a conntrack zone ID for each one, as we'll be creating
>-         * a patch port for each one. */
>-        sset_add(&all_lports, binding_rec->logical_port);
>     }
> }
> 
>-/* We persist lports because we need to know when it changes to
>- * handle ports going away correctly in the binding record. */
>-static struct shash lports = SHASH_INITIALIZER(&lports);
>-
> void
> binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
>             const char *chassis_id, struct hmap *local_datapaths)
>@@ -280,7 +291,7 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
>     }
> 
>     if (br_int) {
>-        if (ctx->ovnsb_idl_txn && get_local_iface_ids(br_int, &lports)) {
>+        if (ctx->ovnsb_idl_txn && get_local_iface_ids(br_int)) {
>             process_full_binding = true;
>         }
>     } else {
>@@ -296,8 +307,8 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
>         struct hmap keep_local_datapath_by_uuid =
>             HMAP_INITIALIZER(&keep_local_datapath_by_uuid);
>         SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) {
>-            consider_local_datapath(ctx, &lports, chassis_rec, binding_rec,
>-                                    local_datapaths);
>+            consider_local_datapath(ctx, chassis_rec, binding_rec,
>+                                    local_datapaths, br_int);
>             struct local_datapath *ld = xzalloc(sizeof *ld);
>             memcpy(&ld->uuid, &binding_rec->header_.uuid, sizeof ld->uuid);
>             hmap_insert(&keep_local_datapath_by_uuid, &ld->uuid_hmap_node,
>@@ -317,8 +328,8 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
>             if (sbrec_port_binding_is_deleted(binding_rec)) {
>                 remove_local_datapath_by_binding(local_datapaths, binding_rec);
>             } else {
>-                consider_local_datapath(ctx, &lports, chassis_rec, binding_rec,
>-                                        local_datapaths);
>+                consider_local_datapath(ctx, chassis_rec, binding_rec,
>+                                        local_datapaths, br_int);
>             }
>         }
>     }
>-- 
>2.7.4
>
>_______________________________________________
>dev mailing list
>dev at openvswitch.org
>http://openvswitch.org/mailman/listinfo/dev
>




More information about the dev mailing list