On Thu, Jul 14, 2016 at 12:02 AM, Mickey Spiegel <emspi...@us.ibm.com> wrote:
> >To: dev@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 <russ...@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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev