| Message ID | 1468525672-6984-1-git-send-email-russell@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Jul 14, 2016 at 12:47 PM, Russell Bryant <russell@ovn.org> wrote: > 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. We also build a temporary > shash of logical port IDs to ovs interfaces used for fast lookup > of the right interface as needed. > > Found by inspection. > > Signed-off-by: Russell Bryant <russell@ovn.org> > --- > ovn/controller/binding.c | 77 > ++++++++++++++++++++---------------------------- > 1 file changed, 32 insertions(+), 45 deletions(-) > > > v1->v2: > - Add lport_to_iface shash built each run in get_local_iface_ids > > > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c > index 4704226..d1efca4 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); > The name "local_ids" seems a little general. It tells us it is "local" in some way, perhaps local to the HV, and it is a set of ids, but "id" is a synonym for "name". So we know it is "local names". Perhaps qualifying the name with something related to "interfaces", maybe iface, would narrow down the context. The same comment applies to "old_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,14 @@ 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, > + struct shash *lport_to_iface) > { > 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 +88,22 @@ 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); > + shash_add(lport_to_iface, 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 +128,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; > } > @@ -188,20 +186,18 @@ update_qos(const struct ovsrec_interface *iface_rec, > } > > 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, > + struct shash *lport_to_iface) > { > const struct ovsrec_interface *iface_rec > - = shash_find_data(lports, binding_rec->logical_port); > + = shash_find_data(lport_to_iface, 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 +238,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,26 +248,16 @@ 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) > { > const struct sbrec_chassis *chassis_rec; > const struct sbrec_port_binding *binding_rec; > + struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface); > > chassis_rec = get_chassis(ctx->ovnsb_idl, chassis_id); > if (!chassis_rec) { > @@ -280,7 +265,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, > &lport_to_iface)) { > process_full_binding = true; > } > } else { > @@ -296,8 +281,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, &lport_to_iface); > 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,11 +302,13 @@ 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, &lport_to_iface); > } > } > } > + > + shash_destroy(&lport_to_iface); > } > > /* Returns true if the database is all cleaned up, false if more work is > -- > 2.7.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
"dev" <dev-bounces@openvswitch.org> wrote on 07/14/2016 02:47:52 PM: > From: Russell Bryant <russell@ovn.org> > To: dev@openvswitch.org > Date: 07/14/2016 02:48 PM > Subject: [ovs-dev] [PATCH v2] ovn-controller: Clean up bindings handling. > Sent by: "dev" <dev-bounces@openvswitch.org> > > 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. We also build a temporary > shash of logical port IDs to ovs interfaces used for fast lookup > of the right interface as needed. > > Found by inspection. > > Signed-off-by: Russell Bryant <russell@ovn.org> Though I see Darrell's points, I'm comfortable enough with this to say... Acked-by: Ryan Moats <rmoats@us.ibm.com> and we can rename things in a followon patch.
On Sat, Jul 16, 2016 at 10:31 PM, Ryan Moats <rmoats@us.ibm.com> wrote: > "dev" <dev-bounces@openvswitch.org> wrote on 07/14/2016 02:47:52 PM: > > > From: Russell Bryant <russell@ovn.org> > > To: dev@openvswitch.org > > Date: 07/14/2016 02:48 PM > > Subject: [ovs-dev] [PATCH v2] ovn-controller: Clean up bindings handling. > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > 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. We also build a temporary > > shash of logical port IDs to ovs interfaces used for fast lookup > > of the right interface as needed. > > > > Found by inspection. > > > > Signed-off-by: Russell Bryant <russell@ovn.org> > > Though I see Darrell's points, I'm comfortable enough with this to say... > > Acked-by: Ryan Moats <rmoats@us.ibm.com> > > and we can rename things in a followon patch. > Thanks, Ryan. I applied this to master. I'm happy to take a follow-up renaming things.
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c index 4704226..d1efca4 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,14 @@ 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, + struct shash *lport_to_iface) { 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 +88,22 @@ 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); + shash_add(lport_to_iface, 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 +128,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; } @@ -188,20 +186,18 @@ update_qos(const struct ovsrec_interface *iface_rec, } 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, + struct shash *lport_to_iface) { const struct ovsrec_interface *iface_rec - = shash_find_data(lports, binding_rec->logical_port); + = shash_find_data(lport_to_iface, 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 +238,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,26 +248,16 @@ 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) { const struct sbrec_chassis *chassis_rec; const struct sbrec_port_binding *binding_rec; + struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface); chassis_rec = get_chassis(ctx->ovnsb_idl, chassis_id); if (!chassis_rec) { @@ -280,7 +265,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, &lport_to_iface)) { process_full_binding = true; } } else { @@ -296,8 +281,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, &lport_to_iface); 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,11 +302,13 @@ 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, &lport_to_iface); } } } + + shash_destroy(&lport_to_iface); } /* Returns true if the database is all cleaned up, false if more work is
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. We also build a temporary shash of logical port IDs to ovs interfaces used for fast lookup of the right interface as needed. Found by inspection. Signed-off-by: Russell Bryant <russell@ovn.org> --- ovn/controller/binding.c | 77 ++++++++++++++++++++---------------------------- 1 file changed, 32 insertions(+), 45 deletions(-) v1->v2: - Add lport_to_iface shash built each run in get_local_iface_ids