From 6f783c0616ec4c8c72003c14a83adc570663a58f Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Tue, 12 Jul 2016 13:33:08 -0400 Subject: [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. 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. Fixes: a478c4efef4d ("ovn-controller: Refactor conntrack zone allocation.") Fixes: 263064aeaa31 ("Convert binding_run to incremental processing.") Signed-off-by: Russell Bryant Acked-by: Ryan Moats --- ovn/controller/binding.c | 77 +++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 45 deletions(-) diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c index 470422636..d1efca473 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 -- 2.20.1