[ovs-dev] [PATCH v2 2/3] ovn-controller: readonly mode binding_run and get_br_int

Han Zhou zhouhan at gmail.com
Mon Jul 10 19:16:10 UTC 2017


On Mon, Jul 10, 2017 at 11:05 AM, Ben Pfaff <blp at ovn.org> wrote:
>
> On Wed, Jun 07, 2017 at 09:32:46AM -0700, Han Zhou wrote:
> > This change is to prepare for the future change for multi-threading.
> > Both binding_run() and get_br_int() are needed by pinctrl thread,
> > but we don't want to update SB DB or create bridges in that scenario,
> > so need "readonly" mode for these functions.
> >
> > Signed-off-by: Han Zhou <zhouhan at gmail.com>
>
> I prefer to separate code that just reads data from code that might
> write to it, because my experience is that it's useful to be able to
> spot the difference without looking closely at a function invocation.
> Here is a version of the patch that does that.  What do you think?
>
> --8<--------------------------cut here-------------------------->8--
>
> From: Han Zhou <zhouhan at gmail.com>
> Date: Wed, 7 Jun 2017 09:32:46 -0700
> Subject: [PATCH] ovn-controller: readonly mode binding_run and get_br_int
>
> This change is to prepare for the future change for multi-threading.
> Both binding_run() and get_br_int() are needed by pinctrl thread,
> but we don't want to update SB DB or create bridges in that scenario,
> so need "readonly" mode for these functions.
>
> Signed-off-by: Han Zhou <zhouhan at gmail.com>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
>  ovn/controller/binding.c        | 247
+++++++++++++++++++++++-----------------
>  ovn/controller/binding.h        |   4 +
>  ovn/controller/ovn-controller.c |  30 +++--
>  3 files changed, 167 insertions(+), 114 deletions(-)
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index 11145dd4cb22..59d06d306ef4 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -67,36 +67,36 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
>  static void
>  get_local_iface_ids(const struct ovsrec_bridge *br_int,
>                      struct shash *lport_to_iface,
> -                    struct sset *local_lports,
> -                    struct sset *egress_ifaces)
> +                    struct sset *local_lports)
>  {
> -    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");
> -            int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport :
0;
> +    for (int i = 0; i < br_int->n_ports; i++) {
> +        const struct ovsrec_port *port = br_int->ports[i];
> +        for (int j = 0; j < port->n_interfaces; j++) {
> +            const struct ovsrec_interface *iface
> +                = port->interfaces[j];
> +            const char *iface_id = smap_get(&iface->external_ids,
"iface-id");
> +            int64_t ofport = iface->n_ofport ? *iface->ofport : 0;
>
>              if (iface_id && ofport > 0) {
> -                shash_add(lport_to_iface, iface_id, iface_rec);
> +                shash_add(lport_to_iface, iface_id, iface);
>                  sset_add(local_lports, iface_id);
>              }
> +        }
> +    }
> +}
> +
> +static void
> +get_egress_ifaces(const struct ovsrec_bridge *br_int,
> +                  struct sset *egress_ifaces)
> +{
> +    for (int i = 0; i < br_int->n_ports; i++) {
> +        const struct ovsrec_port *port = br_int->ports[i];
> +        for (int j = 0; j < port->n_interfaces; j++) {
> +            const struct ovsrec_interface *iface = port->interfaces[j];
>
> -            /* Check if this is a tunnel interface. */
> -            if (smap_get(&iface_rec->options, "remote_ip")) {
> +            if (smap_get(&iface->options, "remote_ip")) {
>                  const char *tunnel_iface
> -                    = smap_get(&iface_rec->status,
"tunnel_egress_iface");
> +                    = smap_get(&iface->status, "tunnel_egress_iface");
>                  if (tunnel_iface) {
>                      sset_add(egress_ifaces, tunnel_iface);
>                  }
> @@ -353,7 +353,19 @@ setup_qos(const char *egress_iface, struct hmap
*queue_map)
>      netdev_close(netdev_phy);
>  }
>
> -static void
> +static bool
> +is_specially_bound(const struct sbrec_port_binding *binding_rec,
> +                   const struct sbrec_chassis *chassis_rec,
> +                   const char *type, const char *option)
> +{
> +    return (!strcmp(binding_rec->type, type)
> +            && !strcmp(smap_get_def(&binding_rec->options, option, ""),
> +                       chassis_rec->name));
> +}
> +
> +/* Returns true if this chassis owns 'binding_rec', that is, it should
set
> + * 'binding_rec->chassis' to point to 'chassis_rec'. */
> +static bool
>  consider_local_datapath(struct controller_ctx *ctx,
>                          const struct ldatapath_index *ldatapaths,
>                          const struct lport_index *lports,
> @@ -367,7 +379,6 @@ consider_local_datapath(struct controller_ctx *ctx,
>      const struct ovsrec_interface *iface_rec
>          = shash_find_data(lport_to_iface, binding_rec->logical_port);
>
> -    bool our_chassis = false;
>      if (iface_rec
>          || (binding_rec->parent_port && binding_rec->parent_port[0] &&
>              sset_contains(local_lports, binding_rec->parent_port))) {
> @@ -380,65 +391,61 @@ consider_local_datapath(struct controller_ctx *ctx,
>          if (iface_rec && qos_map && ctx->ovs_idl_txn) {
>              get_qos_params(binding_rec, qos_map);
>          }
> +
>          /* This port is in our chassis unless it is a localport. */
> -           if (strcmp(binding_rec->type, "localport")) {
> -            our_chassis = true;
> -        }
> -    } else if (!strcmp(binding_rec->type, "l2gateway")) {
> -        const char *chassis_id = smap_get(&binding_rec->options,
> -                                          "l2gateway-chassis");
> -        our_chassis = chassis_id && !strcmp(chassis_id,
chassis_rec->name);
> -        if (our_chassis) {
> -            sset_add(local_lports, binding_rec->logical_port);
> -            add_local_datapath(ldatapaths, lports, binding_rec->datapath,
> -                               false, local_datapaths);
> -        }
> -    } else if (!strcmp(binding_rec->type, "chassisredirect")) {
> -        const char *chassis_id = smap_get(&binding_rec->options,
> -                                          "redirect-chassis");
> -        our_chassis = chassis_id && !strcmp(chassis_id,
chassis_rec->name);
> -        if (our_chassis) {
> -            add_local_datapath(ldatapaths, lports, binding_rec->datapath,
> -                               false, local_datapaths);
> -        }
> -    } else if (!strcmp(binding_rec->type, "l3gateway")) {
> -        const char *chassis_id = smap_get(&binding_rec->options,
> -                                          "l3gateway-chassis");
> -        our_chassis = chassis_id && !strcmp(chassis_id,
chassis_rec->name);
> -        if (our_chassis) {
> -            add_local_datapath(ldatapaths, lports, binding_rec->datapath,
> -                               true, local_datapaths);
> -        }
> +        return strcmp(binding_rec->type, "localport");
> +    } else if (is_specially_bound(binding_rec, chassis_rec,
> +                                  "l2gateway", "l2gateway-chassis")) {
> +        sset_add(local_lports, binding_rec->logical_port);
> +        add_local_datapath(ldatapaths, lports, binding_rec->datapath,
> +                           false, local_datapaths);
> +        return true;
> +    } else if (is_specially_bound(binding_rec, chassis_rec,
> +                                  "chassisredirect",
"redirect-chassis")) {
> +        add_local_datapath(ldatapaths, lports, binding_rec->datapath,
> +                           false, local_datapaths);
> +        return true;
> +    } else if (is_specially_bound(binding_rec, chassis_rec,
> +                                  "l3gateway", "l3gateway-chassis")) {
> +        add_local_datapath(ldatapaths, lports, binding_rec->datapath,
> +                           true, local_datapaths);
> +        return true;
>      } else if (!strcmp(binding_rec->type, "localnet")) {
>          /* Add all localnet ports to local_lports so that we allocate ct
zones
>           * for them. */
>          sset_add(local_lports, binding_rec->logical_port);
> -        our_chassis = false;
> -    }
> -
> -    if (ctx->ovnsb_idl_txn) {
> -        if (our_chassis) {
> -            if (binding_rec->chassis != chassis_rec) {
> -                if (binding_rec->chassis) {
> -                    VLOG_INFO("Changing chassis for lport %s from %s to
%s.",
> -                              binding_rec->logical_port,
> -                              binding_rec->chassis->name,
> -                              chassis_rec->name);
> -                } else {
> -                    VLOG_INFO("Claiming lport %s for this chassis.",
> -                              binding_rec->logical_port);
> -                }
> -                for (int i = 0; i < binding_rec->n_mac; i++) {
> -                    VLOG_INFO("%s: Claiming %s",
> -                              binding_rec->logical_port,
binding_rec->mac[i]);
> -                }
> -                sbrec_port_binding_set_chassis(binding_rec, chassis_rec);
> +        return false;
> +    } else {
> +        return false;
> +    }
> +}
> +
> +static void
> +update_binding_ownership(const struct sbrec_chassis *chassis_rec,
> +                         const struct sbrec_port_binding *binding_rec,
> +                         bool should_own)
> +{
> +    if (should_own) {
> +        if (binding_rec->chassis != chassis_rec) {
> +            if (binding_rec->chassis) {
> +                VLOG_INFO("Changing chassis for lport %s from %s to %s.",
> +                          binding_rec->logical_port,
> +                          binding_rec->chassis->name,
> +                          chassis_rec->name);
> +            } else {
> +                VLOG_INFO("Claiming lport %s for this chassis.",
> +                          binding_rec->logical_port);
>              }
> -        } else if (binding_rec->chassis == chassis_rec) {
> -            VLOG_INFO("Releasing lport %s from this chassis.",
> -                      binding_rec->logical_port);
> -            sbrec_port_binding_set_chassis(binding_rec, NULL);
> +            for (int i = 0; i < binding_rec->n_mac; i++) {
> +                VLOG_INFO("%s: Claiming %s",
> +                          binding_rec->logical_port,
binding_rec->mac[i]);
> +            }
> +            sbrec_port_binding_set_chassis(binding_rec, chassis_rec);
>          }
> +    } else if (binding_rec->chassis == chassis_rec) {
> +        VLOG_INFO("Releasing lport %s from this chassis.",
> +                  binding_rec->logical_port);
> +        sbrec_port_binding_set_chassis(binding_rec, NULL);
>      }
>  }
>
> @@ -466,38 +473,30 @@ consider_localnet_port(const struct
sbrec_port_binding *binding_rec,
>      ld->localnet_port = binding_rec;
>  }
>
> -void
> -binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge
*br_int,
> -            const struct sbrec_chassis *chassis_rec,
> -            const struct ldatapath_index *ldatapaths,
> -            const struct lport_index *lports, struct hmap
*local_datapaths,
> -            struct sset *local_lports)
> +static void
> +binding_run__(struct controller_ctx *ctx, const struct ovsrec_bridge
*br_int,
> +              const struct sbrec_chassis *chassis_rec,
> +              const struct ldatapath_index *ldatapaths,
> +              const struct lport_index *lports, struct hmap *qos_map,
> +              struct hmap *local_datapaths,
> +              struct sset *local_lports, bool update_sb)
>  {
> -    if (!chassis_rec) {
> -        return;
> -    }
> -
>      const struct sbrec_port_binding *binding_rec;
>      struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface);
> -    struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces);
> -    struct hmap qos_map;
> -
> -    hmap_init(&qos_map);
>      if (br_int) {
> -        get_local_iface_ids(br_int, &lport_to_iface, local_lports,
> -                            &egress_ifaces);
> +        get_local_iface_ids(br_int, &lport_to_iface, local_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. */
>      SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) {
> -        consider_local_datapath(ctx, ldatapaths, lports,
> -                                chassis_rec, binding_rec,
> -                                sset_is_empty(&egress_ifaces) ? NULL :
> -                                &qos_map, local_datapaths,
&lport_to_iface,
> -                                local_lports);
> -
> +        bool should_own = consider_local_datapath(
> +            ctx, ldatapaths, lports, chassis_rec, binding_rec,
> +            qos_map, local_datapaths, &lport_to_iface, local_lports);
> +        if (ctx->ovnsb_idl_txn && update_sb) {
> +            update_binding_ownership(chassis_rec, binding_rec,
should_own);
> +        }
>      }
>
>      /* Run through each binding record to see if it is a localnet port
> @@ -509,6 +508,34 @@ binding_run(struct controller_ctx *ctx, const struct
ovsrec_bridge *br_int,
>          }
>      }
>
> +    shash_destroy(&lport_to_iface);
> +}
> +
> +/* Initializes 'local_datapaths' and 'local_lports' to the sets of
logical
> + * datapaths and logical ports, respectively, that are relevant to this
> + * machine.  Updates Port_Binding records 'chassis' columns to point to
> + * 'chassis_rec' where appropriate.  Sets up QoS appropriately on tunnel
egress
> + * interfaces. */
> +void
> +binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge
*br_int,
> +            const struct sbrec_chassis *chassis_rec,
> +            const struct ldatapath_index *ldatapaths,
> +            const struct lport_index *lports, struct hmap
*local_datapaths,
> +            struct sset *local_lports)
> +{
> +    if (!chassis_rec) {
> +        return;
> +    }
> +
> +    struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces);
> +    if (br_int) {
> +        get_egress_ifaces(br_int, &egress_ifaces);
> +    }
> +
> +    struct hmap qos_map = HMAP_INITIALIZER(&qos_map);
> +    binding_run__(ctx, br_int, chassis_rec, ldatapaths, lports,
> +                  sset_is_empty(&egress_ifaces) ? NULL : &qos_map,
> +                  local_datapaths, local_lports, true);
>      if (!sset_is_empty(&egress_ifaces)
>          && set_noop_qos(ctx, &egress_ifaces)) {
>          const char *entry;
> @@ -516,10 +543,26 @@ binding_run(struct controller_ctx *ctx, const
struct ovsrec_bridge *br_int,
>              setup_qos(entry, &qos_map);
>          }
>      }
> -
> -    shash_destroy(&lport_to_iface);
> -    sset_destroy(&egress_ifaces);
>      hmap_destroy(&qos_map);
> +    sset_destroy(&egress_ifaces);
> +}
> +
> +/* Initializes 'local_datapaths' and 'local_lports' to the sets of
logical
> + * datapaths and logical ports, respectively, that are relevant to this
> + * machine. */
> +void
> +binding_get(struct controller_ctx *ctx, const struct ovsrec_bridge
*br_int,
> +            const struct sbrec_chassis *chassis_rec,
> +            const struct ldatapath_index *ldatapaths,
> +            const struct lport_index *lports, struct hmap
*local_datapaths,
> +            struct sset *local_lports)
> +{
> +    if (!chassis_rec) {
> +        return;
> +    }
> +
> +    binding_run__(ctx, br_int, chassis_rec, ldatapaths, lports, NULL,
> +                  local_datapaths, local_lports, false);
>  }
>
>  /* Returns true if the database is all cleaned up, false if more work is
> diff --git a/ovn/controller/binding.h b/ovn/controller/binding.h
> index 3bfa7d1a924e..98a45a8c6233 100644
> --- a/ovn/controller/binding.h
> +++ b/ovn/controller/binding.h
> @@ -33,6 +33,10 @@ void binding_run(struct controller_ctx *, const struct
ovsrec_bridge *br_int,
>                   const struct sbrec_chassis *, const struct
ldatapath_index *,
>                   const struct lport_index *, struct hmap
*local_datapaths,
>                   struct sset *all_lports);
> +void binding_get(struct controller_ctx *, const struct ovsrec_bridge
*br_int,
> +                 const struct sbrec_chassis *, const struct
ldatapath_index *,
> +                 const struct lport_index *, struct hmap
*local_datapaths,
> +                 struct sset *all_lports);
>  bool binding_cleanup(struct controller_ctx *, const struct sbrec_chassis
*);
>
>  #endif /* ovn/binding.h */
> diff --git a/ovn/controller/ovn-controller.c
b/ovn/controller/ovn-controller.c
> index 45a670b5d754..cc01fe9e3477 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -201,15 +201,26 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
>      ovsdb_idl_condition_destroy(&dns);
>  }
>
> +static const char *
> +br_int_name(const struct ovsrec_open_vswitch *cfg)
> +{
> +    return smap_get_def(&cfg->external_ids, "ovn-bridge",
DEFAULT_BRIDGE_NAME);
> +}
> +
>  static const struct ovsrec_bridge *
> -create_br_int(struct controller_ctx *ctx,
> -              const struct ovsrec_open_vswitch *cfg,
> -              const char *bridge_name)
> +create_br_int(struct controller_ctx *ctx)
>  {
>      if (!ctx->ovs_idl_txn) {
>          return NULL;
>      }
>
> +    const struct ovsrec_open_vswitch *cfg;
> +    cfg = ovsrec_open_vswitch_first(ctx->ovs_idl);
> +    if (!cfg) {
> +        return NULL;
> +    }
> +    const char *bridge_name = br_int_name(cfg);
> +
>      ovsdb_idl_txn_add_comment(ctx->ovs_idl_txn,
>              "ovn-controller: creating integration bridge '%s'",
bridge_name);
>
> @@ -252,15 +263,7 @@ get_br_int(struct controller_ctx *ctx)
>          return NULL;
>      }
>
> -    const char *br_int_name = smap_get_def(&cfg->external_ids,
"ovn-bridge",
> -                                           DEFAULT_BRIDGE_NAME);
> -
> -    const struct ovsrec_bridge *br;
> -    br = get_bridge(ctx->ovs_idl, br_int_name);
> -    if (!br) {
> -        return create_br_int(ctx, cfg, br_int_name);
> -    }
> -    return br;
> +    return get_bridge(ctx->ovs_idl, br_int_name(cfg));
>  }
>
>  static const char *
> @@ -622,6 +625,9 @@ main(int argc, char *argv[])
>          struct sset local_lports = SSET_INITIALIZER(&local_lports);
>
>          const struct ovsrec_bridge *br_int = get_br_int(&ctx);
> +        if (!br_int) {
> +            br_int = create_br_int(&ctx);
> +        }
>          const char *chassis_id = get_chassis_id(ctx.ovs_idl);
>
>          struct ldatapath_index ldatapaths;
> --
> 2.10.2
>

Ben, thanks for the suggestion. I agree it is better. I will test it and
refactor if needed.


More information about the dev mailing list