On Mon, Jul 10, 2017 at 11:05 AM, Ben Pfaff <b...@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 <zhou...@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 <zhou...@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 <zhou...@gmail.com> > Signed-off-by: Ben Pfaff <b...@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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev