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

[Russell Bryant]

On Thu, Jul 14, 2016 at 12:02 AM, Mickey Spiegel <emspiege at us.ibm.com>
wrote:

> >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?


I considered it, but it seemed wasteful to do it in every binding_run()
when it won't be needed the vast majority of times.

The optimization makes sense in the process_full_binding case, though, I
agree.  I can include that in a v2.


>
>
> >+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.


The reason I didn't do this is because the vast majority of times
get_local_iface_ids is called, consider_local_datapath is *not* called.
The benefit wasn't obvious enough.

-- 
Russell Bryant