[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