| Message ID | 1468446788-7731-1-git-send-email-russell@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
>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 <russell@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@openvswitch.org >http://openvswitch.org/mailman/listinfo/dev >
On Thu, Jul 14, 2016 at 12:02 AM, Mickey Spiegel <emspiege@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 <russell@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.
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) +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) +{ + 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); + 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); } } }
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@ovn.org> --- ovn/controller/binding.c | 101 ++++++++++++++++++++++++++--------------------- 1 file changed, 56 insertions(+), 45 deletions(-)