Russell Bryant <russ...@ovn.org> wrote on 08/25/2016 12:45:44 PM:
> From: Russell Bryant <russ...@ovn.org>
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: ovs dev <dev@openvswitch.org>
> Date: 08/25/2016 12:46 PM
> Subject: Re: [ovs-dev] [PATCH v4] ovn-controller: Back out
> incremental processing
>
> On Wed, Aug 24, 2016 at 9:30 PM, Ryan Moats <rmo...@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 <rmo...@us.ibm.com>
> Co-authored-by: Guru Shetty <g...@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.
Not true - as I said in the commit message, local_datapaths and
all_lports are no longer persisted, so they are rebuilt during
each cycle again...
>
> 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.
See my above comment about not persisting things as part of this backout...
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
