[ovs-dev] [PATCH v4] ovn-controller: Back out incremental processing
Russell Bryant
russell at ovn.org
Thu Aug 25 17:45:44 UTC 2016
On Wed, Aug 24, 2016 at 9:30 PM, Ryan Moats <rmoats at us.ibm.com> wrote:
> As [1] indicates, incremental processing hasn't resulted
> in an improvement worth the complexity it has added.
> This patch backs out all of the code specific to incremental
> processing, along with the persisting of OF flows,
> logical ports, multicast groups, all_lports, local and patched
> datapaths
>
> [1] http://openvswitch.org/pipermail/dev/2016-August/078272.html
>
> Signed-off-by: Ryan Moats <rmoats at us.ibm.com>
> Co-authored-by: Guru Shetty <guru at ovn.com>
>
I'm mainly looking at binding.c right now, as that is code that I've gone
through in detail since the incremental processing changes were applied.
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index c26007d..0353a7b 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -30,18 +30,6 @@
>
> VLOG_DEFINE_THIS_MODULE(binding);
>
> -/* 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
> -binding_reset_processing(void)
> -{
> - process_full_binding = true;
> -}
> -
> void
> binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> {
> @@ -64,16 +52,12 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> &ovsrec_interface_col_ingress_policing_burst);
> }
>
> -static bool
> +static void
> get_local_iface_ids(const struct ovsrec_bridge *br_int,
> struct shash *lport_to_iface,
> struct sset *all_lports)
> {
> int i;
> - bool changed = false;
> -
> - 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];
> @@ -93,53 +77,9 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int,
> continue;
> }
> 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);
> - sset_add(all_lports, iface_id);
> - changed = true;
> - }
> + sset_add(all_lports, iface_id);
> }
> }
> -
> - /* 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;
> -
> - const char *cur_id;
> - SSET_FOR_EACH(cur_id, &old_local_ids) {
> - sset_find_and_delete(&local_ids, cur_id);
> - sset_find_and_delete(all_lports, cur_id);
> - }
> - }
> -
> - sset_destroy(&old_local_ids);
> -
> - return changed;
> -}
> -
> -static struct local_datapath *
> -local_datapath_lookup_by_uuid(struct hmap *hmap_p, const struct uuid
> *uuid)
> -{
> - struct local_datapath *ld;
> - HMAP_FOR_EACH_WITH_HASH(ld, uuid_hmap_node, uuid_hash(uuid), hmap_p) {
> - if (uuid_equals(&ld->uuid, uuid)) {
> - return ld;
> - }
> - }
> - return NULL;
> -}
> -
> -static void
> -remove_local_datapath(struct hmap *local_datapaths, struct local_datapath
> *ld)
> -{
> - if (ld->logical_port) {
> - free(ld->logical_port);
> - ld->logical_port = NULL;
> - }
> - hmap_remove(local_datapaths, &ld->hmap_node);
> - free(ld);
> - lflow_reset_processing();
> }
>
>
This patch appears to break local_datapaths as it no longer ever removes
anything from local_datapaths.
all_lports also appears to have at least one problem. localnet ports will
never get removed from all_lports after this change.
> static void
> @@ -156,9 +96,6 @@ add_local_datapath(struct hmap *local_datapaths,
> memcpy(&ld->uuid, &binding_rec->header_.uuid, sizeof ld->uuid);
> hmap_insert(local_datapaths, &ld->hmap_node,
> binding_rec->datapath->tunnel_key);
> - lport_index_reset();
> - mcgroup_index_reset();
> - lflow_reset_processing();
> }
>
> static void
> @@ -185,7 +122,11 @@ consider_local_datapath(struct controller_ctx *ctx,
>
> if (iface_rec
> || (binding_rec->parent_port && binding_rec->parent_port[0] &&
> - sset_contains(&local_ids, binding_rec->parent_port))) {
> + 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);
> + }
> add_local_datapath(local_datapaths, binding_rec);
> if (iface_rec && ctx->ovs_idl_txn) {
> update_qos(iface_rec, binding_rec);
> @@ -204,9 +145,6 @@ consider_local_datapath(struct controller_ctx *ctx,
> binding_rec->logical_port);
> }
> sbrec_port_binding_set_chassis(binding_rec, chassis_rec);
> - if (binding_rec->parent_port && binding_rec->parent_port[0]) {
> - sset_add(all_lports, binding_rec->logical_port);
> - }
> }
> } else if (!strcmp(binding_rec->type, "l2gateway")) {
> const char *chassis_id = smap_get(&binding_rec->options,
> @@ -216,11 +154,12 @@ consider_local_datapath(struct controller_ctx *ctx,
> VLOG_INFO("Releasing l2gateway port %s from this
> chassis.",
> binding_rec->logical_port);
> sbrec_port_binding_set_chassis(binding_rec, NULL);
> - sset_find_and_delete(all_lports,
> binding_rec->logical_port);
>
This appears to be breaking all_lports for l2gateway ports. They will
never get removed after this change.
> }
> return;
> }
>
> + sset_add(all_lports, binding_rec->logical_port);
> + add_local_datapath(local_datapaths, binding_rec);
> if (binding_rec->chassis == chassis_rec) {
> return;
> }
> @@ -229,8 +168,6 @@ consider_local_datapath(struct controller_ctx *ctx,
> 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);
> }
> } else if (!strcmp(binding_rec->type, "l3gateway")) {
> const char *chassis = smap_get(&binding_rec->options,
> @@ -268,79 +205,16 @@ 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,
> &lport_to_iface,
> - all_lports)) {
> - process_full_binding = true;
> - }
> - } else {
> - /* We have no integration bridge, therefore no local logical
> ports.
> - * We'll remove our chassis from all port binding records below.
> */
> - process_full_binding = true;
> + get_local_iface_ids(br_int, &lport_to_iface, all_lports);
> }
>
> /* Run through each binding record to see if it is resident on this
> * chassis and update the binding accordingly. This includes both
> * directly connected logical ports and children of those ports. */
> - if (process_full_binding) {
> - /* Detect any entries in all_lports that have been deleted.
> - * In particular, this will catch localnet ports that we
> - * put in all_lports. */
> - struct sset removed_lports = SSET_INITIALIZER(&removed_lports);
> - sset_clone(&removed_lports, all_lports);
> -
> - struct hmap keep_local_datapath_by_uuid =
> - HMAP_INITIALIZER(&keep_local_datapath_by_uuid);
> - SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) {
> - sset_find_and_delete(&removed_lports,
> binding_rec->logical_port);
> - consider_local_datapath(ctx, chassis_rec, binding_rec,
> - local_datapaths, &lport_to_iface,
> - all_lports);
> - 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,
> - uuid_hash(&ld->uuid));
> - }
> - struct local_datapath *old_ld, *next;
> - HMAP_FOR_EACH_SAFE (old_ld, next, hmap_node, local_datapaths) {
> - if (!local_datapath_lookup_by_uuid(&keep_local_datapath_by_
> uuid,
> - &old_ld->uuid)) {
> - remove_local_datapath(local_datapaths, old_ld);
> - }
> - }
> - struct local_datapath *ld;
> - HMAP_FOR_EACH_SAFE (ld, next, uuid_hmap_node,
> - &keep_local_datapath_by_uuid) {
> - hmap_remove(&keep_local_datapath_by_uuid,
> &ld->uuid_hmap_node);
> - free(ld);
> - }
> - hmap_destroy(&keep_local_datapath_by_uuid);
> -
> - /* Any remaining entries in removed_lports are logical ports that
> - * have been deleted and should also be removed from all_ports. */
> - const char *cur_id;
> - SSET_FOR_EACH(cur_id, &removed_lports) {
> - sset_find_and_delete(all_lports, cur_id);
> - }
> - sset_destroy(&removed_lports);
> -
> - process_full_binding = false;
> - } else {
> - SBREC_PORT_BINDING_FOR_EACH_TRACKED(binding_rec, ctx->ovnsb_idl)
> {
> - if (sbrec_port_binding_is_deleted(binding_rec)) {
> - /* If a port binding was bound to this chassis and
> removed before
> - * the ovs interface was removed, we'll catch that here
> and trigger
> - * a full bindings refresh. This is to see if we need to
> clear
> - * an entry out of local_datapaths. */
> - if (binding_rec->chassis == chassis_rec) {
> - process_full_binding = true;
> - poll_immediate_wake();
> - }
> - } else {
> - consider_local_datapath(ctx, chassis_rec, binding_rec,
> - local_datapaths, &lport_to_iface,
> - all_lports);
> - }
> - }
> + SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) {
> + consider_local_datapath(ctx, chassis_rec, binding_rec,
> + local_datapaths, &lport_to_iface,
> + all_lports);
> }
>
> shash_destroy(&lport_to_iface);
>
>
More information about the dev
mailing list