Hi Numan, Please see my comments below.
On Thu, Apr 30, 2020 at 9:59 AM <[email protected]> wrote: > > From: Numan Siddique <[email protected]> > > This patch adds a new data structure - 'struct local_binding' which represents > a probable local port binding. A new object of this structure is creared for > each interface present in the integration bridge (br-int) with the > external_ids:iface-id set. This struct has 2 main fields > - 'struct ovsrec_interface *' and 'struct sbrec_port_binding *'. These fields > are updated during port claim and release. > > A shash of the local bindings is maintained with the 'iface-id' as the hash key > in the runtime data of the incremental processing engine. > > This patch helps in the upcoming patch to add incremental processing support > for OVS interface and SB port binding changes. > > Signed-off-by: Numan Siddique <[email protected]> > --- > controller/binding.c | 722 ++++++++++++++++++++++-------------- > controller/binding.h | 16 +- > controller/ovn-controller.c | 46 +-- > controller/pinctrl.c | 1 + > controller/pinctrl.h | 1 + > 5 files changed, 467 insertions(+), 319 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 007a94866..66f4f65e3 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -69,47 +69,6 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) > ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type); > } > > -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) > -{ > - 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; > - > - if (iface_id && ofport > 0) { > - shash_add(lport_to_iface, iface_id, iface_rec); > - sset_add(local_lports, iface_id); > - } > - > - /* Check if this is a tunnel interface. */ > - if (smap_get(&iface_rec->options, "remote_ip")) { > - const char *tunnel_iface > - = smap_get(&iface_rec->status, "tunnel_egress_iface"); > - if (tunnel_iface) { > - sset_add(egress_ifaces, tunnel_iface); > - } > - } > - } > - } > -} > - > static void > add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > @@ -441,25 +400,11 @@ static bool > is_our_chassis(const struct sbrec_chassis *chassis_rec, > const struct sbrec_port_binding *binding_rec, > const struct sset *active_tunnels, > - const struct ovsrec_interface *iface_rec, > const struct sset *local_lports) > { > - /* Ports of type "virtual" should never be explicitly bound to an OVS > - * port in the integration bridge. If that's the case, ignore the binding > - * and log a warning. > - */ > - if (iface_rec && !strcmp(binding_rec->type, "virtual")) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > - VLOG_WARN_RL(&rl, > - "Virtual port %s should not be bound to OVS port %s", > - binding_rec->logical_port, iface_rec->name); > - return false; > - } > - > bool our_chassis = false; > - if (iface_rec > - || (binding_rec->parent_port && binding_rec->parent_port[0] && > - sset_contains(local_lports, binding_rec->parent_port))) { > + if (binding_rec->parent_port && binding_rec->parent_port[0] && > + sset_contains(local_lports, binding_rec->parent_port)) { > /* This port is in our chassis unless it is a localport. */ > our_chassis = strcmp(binding_rec->type, "localport"); > } else if (!strcmp(binding_rec->type, "l2gateway")) { > @@ -481,175 +426,6 @@ is_our_chassis(const struct sbrec_chassis *chassis_rec, > return our_chassis; > } > > -/* Updates 'binding_rec' and if the port binding is local also updates the > - * local datapaths and ports. > - * Updates and returns the array of local virtual ports that will require > - * additional processing. > - */ > -static const struct sbrec_port_binding ** > -consider_local_datapath(const struct sbrec_port_binding *binding_rec, > - struct hmap *qos_map, > - const struct ovsrec_interface *iface_rec, > - struct binding_ctx_in *b_ctx_in, > - struct binding_ctx_out *b_ctx_out, > - const struct sbrec_port_binding **vpbs, > - size_t *n_vpbs, size_t *n_allocated_vpbs) > -{ > - bool our_chassis = is_our_chassis(b_ctx_in->chassis_rec, binding_rec, > - b_ctx_in->active_tunnels, iface_rec, > - b_ctx_out->local_lports); > - if (iface_rec > - || (binding_rec->parent_port && binding_rec->parent_port[0] && > - sset_contains(b_ctx_out->local_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(b_ctx_out->local_lports, binding_rec->logical_port); > - } > - add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > - b_ctx_in->sbrec_port_binding_by_datapath, > - b_ctx_in->sbrec_port_binding_by_name, > - binding_rec->datapath, false, > - b_ctx_out->local_datapaths); > - if (iface_rec && qos_map && b_ctx_in->ovs_idl_txn) { > - get_qos_params(binding_rec, qos_map); > - } > - } else if (!strcmp(binding_rec->type, "l2gateway")) { > - if (our_chassis) { > - sset_add(b_ctx_out->local_lports, binding_rec->logical_port); > - add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > - b_ctx_in->sbrec_port_binding_by_datapath, > - b_ctx_in->sbrec_port_binding_by_name, > - binding_rec->datapath, false, > - b_ctx_out->local_datapaths); > - } > - } else if (!strcmp(binding_rec->type, "chassisredirect")) { > - if (ha_chassis_group_contains(binding_rec->ha_chassis_group, > - b_ctx_in->chassis_rec)) { > - add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > - b_ctx_in->sbrec_port_binding_by_datapath, > - b_ctx_in->sbrec_port_binding_by_name, > - binding_rec->datapath, false, > - b_ctx_out->local_datapaths); > - } > - } else if (!strcmp(binding_rec->type, "l3gateway")) { > - if (our_chassis) { > - add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > - b_ctx_in->sbrec_port_binding_by_datapath, > - b_ctx_in->sbrec_port_binding_by_name, > - binding_rec->datapath, true, > - b_ctx_out->local_datapaths); > - } > - } else if (!strcmp(binding_rec->type, "localnet")) { > - /* Add all localnet ports to local_lports so that we allocate ct zones > - * for them. */ > - sset_add(b_ctx_out->local_lports, binding_rec->logical_port); > - if (qos_map && b_ctx_in->ovs_idl_txn) { > - get_qos_params(binding_rec, qos_map); > - } > - } else if (!strcmp(binding_rec->type, "external")) { > - if (ha_chassis_group_contains(binding_rec->ha_chassis_group, > - b_ctx_in->chassis_rec)) { > - add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > - b_ctx_in->sbrec_port_binding_by_datapath, > - b_ctx_in->sbrec_port_binding_by_name, > - binding_rec->datapath, false, > - b_ctx_out->local_datapaths); > - } > - } > - > - if (our_chassis > - || !strcmp(binding_rec->type, "patch") > - || !strcmp(binding_rec->type, "localport") > - || !strcmp(binding_rec->type, "vtep") > - || !strcmp(binding_rec->type, "localnet")) { > - update_local_lport_ids(b_ctx_out->local_lport_ids, binding_rec); > - } > - > - ovs_assert(b_ctx_in->ovnsb_idl_txn); > - const char *vif_chassis = smap_get(&binding_rec->options, > - "requested-chassis"); > - bool can_bind = !vif_chassis || !vif_chassis[0] > - || !strcmp(vif_chassis, b_ctx_in->chassis_rec->name) > - || !strcmp(vif_chassis, b_ctx_in->chassis_rec->hostname); > - > - if (can_bind && our_chassis) { > - if (binding_rec->chassis != b_ctx_in->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, > - b_ctx_in->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, b_ctx_in->chassis_rec); > - } > - /* Check if the port encap binding, if any, has changed */ > - struct sbrec_encap *encap_rec = > - sbrec_get_port_encap(b_ctx_in->chassis_rec, iface_rec); > - if (encap_rec && binding_rec->encap != encap_rec) { > - sbrec_port_binding_set_encap(binding_rec, encap_rec); > - } > - } else if (binding_rec->chassis == b_ctx_in->chassis_rec) { > - if (!strcmp(binding_rec->type, "virtual")) { > - if (*n_vpbs == *n_allocated_vpbs) { > - vpbs = x2nrealloc(vpbs, n_allocated_vpbs, sizeof *vpbs); > - } > - vpbs[(*n_vpbs)] = binding_rec; > - (*n_vpbs)++; > - } else { > - VLOG_INFO("Releasing lport %s from this chassis.", > - binding_rec->logical_port); > - if (binding_rec->encap) { > - sbrec_port_binding_set_encap(binding_rec, NULL); > - } > - sbrec_port_binding_set_chassis(binding_rec, NULL); > - } > - } else if (our_chassis) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > - VLOG_INFO_RL(&rl, > - "Not claiming lport %s, chassis %s " > - "requested-chassis %s", > - binding_rec->logical_port, b_ctx_in->chassis_rec->name, > - vif_chassis); > - } > - > - return vpbs; > -} > - > -static void > -consider_local_virtual_port(struct ovsdb_idl_index *sbrec_port_binding_by_name, > - const struct sbrec_chassis *chassis_rec, > - const struct sbrec_port_binding *binding_rec) > -{ > - if (binding_rec->virtual_parent) { > - const struct sbrec_port_binding *parent = > - lport_lookup_by_name(sbrec_port_binding_by_name, > - binding_rec->virtual_parent); > - if (parent && parent->chassis == chassis_rec) { > - return; > - } > - } > - > - /* pinctrl module takes care of binding the ports of type 'virtual'. > - * Release such ports if their virtual parents are no longer claimed by > - * this chassis. > - */ > - VLOG_INFO("Releasing lport %s from this chassis.", > - binding_rec->logical_port); > - if (binding_rec->encap) { > - sbrec_port_binding_set_encap(binding_rec, NULL); > - } > - sbrec_port_binding_set_chassis(binding_rec, NULL); > - sbrec_port_binding_set_virtual_parent(binding_rec, NULL); > -} > - > static void > add_localnet_egress_interface_mappings( > const struct sbrec_port_binding *port_binding, > @@ -712,6 +488,374 @@ consider_localnet_port(const struct sbrec_port_binding *binding_rec, > ld->localnet_port = binding_rec; > } > > +enum local_binding_type { > + BT_VIF, > + BT_CHILD, > + BT_VIRTUAL > +}; > + > +struct local_binding { > + struct ovs_list node; /* In parent if any. */ > + char *name; > + enum local_binding_type type; > + const struct ovsrec_interface *iface; > + const struct sbrec_port_binding *pb; > + > + struct ovs_list children; > +}; > + > +static struct local_binding * > +local_binding_create(const char *name, const struct ovsrec_interface *iface, > + const struct sbrec_port_binding *pb, > + enum local_binding_type type) > +{ > + struct local_binding *lbinding = xzalloc(sizeof *lbinding); > + lbinding->name = xstrdup(name); > + lbinding->type = type; > + lbinding->pb = pb; > + lbinding->iface = iface; > + ovs_list_init(&lbinding->children); > + return lbinding; > +} > + > +static void > +local_binding_add(struct shash *local_bindings, struct local_binding *lbinding) > +{ > + shash_add(local_bindings, lbinding->name, lbinding); > +} > + > +static struct local_binding * > +local_binding_find(struct shash *local_bindings, const char *name) > +{ > + return shash_find_data(local_bindings, name); > +} > + > +static void > +local_binding_destroy(struct local_binding *lbinding) > +{ > + struct local_binding *c, *next; > + LIST_FOR_EACH_SAFE (c, next, node, &lbinding->children) { > + ovs_list_remove(&c->node); > + free(c->name); > + free(c); > + } > + free(lbinding->name); > + free(lbinding); > +} > + > +void > +local_bindings_destroy(struct shash *local_bindings) > +{ > + struct shash_node *node, *next; > + SHASH_FOR_EACH_SAFE (node, next, local_bindings) { > + struct local_binding *lbinding = node->data; > + struct local_binding *c, *n; > + LIST_FOR_EACH_SAFE (c, n, node, &lbinding->children) { > + ovs_list_remove(&c->node); > + free(c->name); > + free(c); > + } > + } > + > + shash_destroy(local_bindings); > +} > + > +static void > +local_binding_add_child(struct local_binding *lbinding, > + struct local_binding *child) > +{ > + struct local_binding *l; > + LIST_FOR_EACH (l, node, &lbinding->children) { > + if (l == child) { > + return; > + } > + } > + > + ovs_list_push_back(&lbinding->children, &child->node); > +} > + > +static struct local_binding * > +local_binding_find_child(struct local_binding *lbinding, > + const char *child_name) > +{ > + struct local_binding *l; > + LIST_FOR_EACH (l, node, &lbinding->children) { > + if (!strcmp(l->name, child_name)) { > + return l; > + } > + } > + > + return NULL; > +} > + > +void > +binding_add_vport_to_local_bindings(struct shash *local_bindings, > + const struct sbrec_port_binding *parent, > + const struct sbrec_port_binding *vport) > +{ > + struct local_binding *lbinding = local_binding_find(local_bindings, > + parent->logical_port); > + ovs_assert(lbinding); > + struct local_binding *vbinding = > + local_binding_find_child(lbinding, vport->logical_port); > + if (!vbinding) { > + vbinding = local_binding_create(vport->logical_port, lbinding->iface, > + vport, BT_VIRTUAL); > + local_binding_add_child(lbinding, vbinding); > + } else { > + vbinding->type = BT_VIRTUAL; > + } > +} > + > +static void > +claim_lport(const struct sbrec_port_binding *pb, > + const struct sbrec_chassis *chassis_rec, > + const struct ovsrec_interface *iface_rec) > +{ > + if (pb->chassis != chassis_rec) { > + if (pb->chassis) { > + VLOG_INFO("Changing chassis for lport %s from %s to %s.", > + pb->logical_port, pb->chassis->name, > + chassis_rec->name); > + } else { > + VLOG_INFO("Claiming lport %s for this chassis.", pb->logical_port); > + } > + for (int i = 0; i < pb->n_mac; i++) { > + VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]); > + } > + > + sbrec_port_binding_set_chassis(pb, chassis_rec); > + } > + > + /* Check if the port encap binding, if any, has changed */ > + struct sbrec_encap *encap_rec = > + sbrec_get_port_encap(chassis_rec, iface_rec); > + if (encap_rec && pb->encap != encap_rec) { > + sbrec_port_binding_set_encap(pb, encap_rec); > + } > +} > + > +static void > +release_lport(const struct sbrec_port_binding *pb) > +{ > + VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port); > + if (pb->encap) { > + sbrec_port_binding_set_encap(pb, NULL); > + } > + sbrec_port_binding_set_chassis(pb, NULL); > + > + if (pb->virtual_parent) { > + sbrec_port_binding_set_virtual_parent(pb, NULL); > + } > +} > + > +static void > +consider_port_binding_for_vif(const struct sbrec_port_binding *pb, > + struct binding_ctx_in *b_ctx_in, > + enum local_binding_type binding_type, > + struct local_binding *lbinding, > + struct binding_ctx_out *b_ctx_out, > + struct hmap *qos_map) > +{ > + const char *vif_chassis = smap_get(&pb->options, "requested-chassis"); > + bool can_bind = !vif_chassis || !vif_chassis[0] > + || !strcmp(vif_chassis, b_ctx_in->chassis_rec->name) > + || !strcmp(vif_chassis, b_ctx_in->chassis_rec->hostname); > + > + /* Ports of type "virtual" should never be explicitly bound to an OVS > + * port in the integration bridge. If that's the case, ignore the binding > + * and log a warning. > + */ > + if (!strcmp(pb->type, "virtual") && lbinding && lbinding->iface && > + binding_type == BT_VIF) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_WARN_RL(&rl, > + "Virtual port %s should not be bound to OVS port %s", > + pb->logical_port, lbinding->iface->name); > + lbinding->pb = NULL; > + return; > + } > + > + if (lbinding && lbinding->pb && can_bind) { > + claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface); > + > + switch (binding_type) { > + case BT_VIF: > + lbinding->pb = pb; > + break; > + case BT_CHILD: > + case BT_VIRTUAL: > + { I think this pair of '{}' is not needed in this project's coding style. It looks unnatural. Correct me if I am wrong. > + /* Add child logical port to the set of all local ports. */ > + sset_add(b_ctx_out->local_lports, pb->logical_port); > + struct local_binding *child = > + local_binding_find_child(lbinding, pb->logical_port); > + if (!child) { > + child = local_binding_create(pb->logical_port, lbinding->iface, > + pb, binding_type); > + local_binding_add_child(lbinding, child); > + } else { > + ovs_assert(child->type == BT_CHILD || > + child->type == BT_VIRTUAL); > + child->pb = pb; > + child->iface = lbinding->iface; > + } > + break; > + } > + default: > + OVS_NOT_REACHED(); > + } > + > + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > + b_ctx_in->sbrec_port_binding_by_datapath, > + b_ctx_in->sbrec_port_binding_by_name, > + pb->datapath, false, b_ctx_out->local_datapaths); > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > + if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { > + get_qos_params(pb, qos_map); > + } > + } else if (lbinding && lbinding->pb && !can_bind) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_INFO_RL(&rl, > + "Not claiming lport %s, chassis %s " > + "requested-chassis %s", > + pb->logical_port, > + b_ctx_in->chassis_rec->name, > + vif_chassis); > + } > + > + if (pb->chassis == b_ctx_in->chassis_rec) { > + if (!lbinding || !lbinding->pb || !can_bind) { > + release_lport(pb); > + } > + } > +} > + > +static void > +consider_port_binding(const struct sbrec_port_binding *pb, > + struct binding_ctx_in *b_ctx_in, > + struct binding_ctx_out *b_ctx_out, > + struct hmap *qos_map) > +{ > + > + bool our_chassis = is_our_chassis(b_ctx_in->chassis_rec, pb, > + b_ctx_in->active_tunnels, > + b_ctx_out->local_lports); > + > + if (!strcmp(pb->type, "l2gateway")) { > + if (our_chassis) { > + sset_add(b_ctx_out->local_lports, pb->logical_port); > + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > + b_ctx_in->sbrec_port_binding_by_datapath, > + b_ctx_in->sbrec_port_binding_by_name, > + pb->datapath, false, > + b_ctx_out->local_datapaths); > + } > + } else if (!strcmp(pb->type, "chassisredirect")) { > + if (ha_chassis_group_contains(pb->ha_chassis_group, > + b_ctx_in->chassis_rec)) { > + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > + b_ctx_in->sbrec_port_binding_by_datapath, > + b_ctx_in->sbrec_port_binding_by_name, > + pb->datapath, false, > + b_ctx_out->local_datapaths); > + } > + } else if (!strcmp(pb->type, "l3gateway")) { > + if (our_chassis) { > + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > + b_ctx_in->sbrec_port_binding_by_datapath, > + b_ctx_in->sbrec_port_binding_by_name, > + pb->datapath, true, b_ctx_out->local_datapaths); > + } > + } else if (!strcmp(pb->type, "localnet")) { > + /* Add all localnet ports to local_lports so that we allocate ct zones > + * for them. */ > + sset_add(b_ctx_out->local_lports, pb->logical_port); > + if (qos_map && b_ctx_in->ovs_idl_txn) { > + get_qos_params(pb, qos_map); > + } > + } else if (!strcmp(pb->type, "external")) { > + if (ha_chassis_group_contains(pb->ha_chassis_group, > + b_ctx_in->chassis_rec)) { > + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > + b_ctx_in->sbrec_port_binding_by_datapath, > + b_ctx_in->sbrec_port_binding_by_name, > + pb->datapath, false, > + b_ctx_out->local_datapaths); > + } > + } > + > + if (our_chassis || !strcmp(pb->type, "localnet")) { > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > + } > + > + ovs_assert(b_ctx_in->ovnsb_idl_txn); > + if (our_chassis) { > + claim_lport(pb, b_ctx_in->chassis_rec, NULL); > + } else if (pb->chassis == b_ctx_in->chassis_rec) { > + release_lport(pb); > + } > +} > + > +static void > +build_local_bindings_from_local_ifaces(struct binding_ctx_in *b_ctx_in, > + struct binding_ctx_out *b_ctx_out) > +{ > + int i; > + for (i = 0; i < b_ctx_in->br_int->n_ports; i++) { > + const struct ovsrec_port *port_rec = b_ctx_in->br_int->ports[i]; > + const char *iface_id; > + int j; > + > + if (!strcmp(port_rec->name, b_ctx_in->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; > + > + if (iface_id && ofport > 0) { > + const struct sbrec_port_binding *pb > + = lport_lookup_by_name( > + b_ctx_in->sbrec_port_binding_by_name, iface_id); > + struct local_binding *lbinding = > + local_binding_find(b_ctx_out->local_bindings, iface_id); > + if (!lbinding) { > + lbinding = local_binding_create(iface_id, iface_rec, pb, > + BT_VIF); > + local_binding_add(b_ctx_out->local_bindings, lbinding); > + } else { > + lbinding->type = BT_VIF; > + lbinding->pb = pb; > + } > + sset_add(b_ctx_out->local_lports, iface_id); > + } > + > + /* Check if this is a tunnel interface. */ > + if (smap_get(&iface_rec->options, "remote_ip")) { > + const char *tunnel_iface > + = smap_get(&iface_rec->status, "tunnel_egress_iface"); > + if (tunnel_iface) { > + sset_add(b_ctx_out->egress_ifaces, tunnel_iface); > + } > + } > + } > + } > + > + struct shash_node *node, *next; > + SHASH_FOR_EACH_SAFE (node, next, b_ctx_out->local_bindings) { > + struct local_binding *lbinding = node->data; > + if (!sset_contains(b_ctx_out->local_lports, lbinding->name)) { > + local_binding_destroy(lbinding); > + shash_delete(b_ctx_out->local_bindings, node); > + } > + } It seems this function is called only once in the binding_run() which means it is called at recomputing. So the local_bindings should get rebuild (see my another comment about it not getting reset in ovn-controller.c in runtime_data_run). If we are rebuilding it, then this last loop of deleting the non-exist local_bindings is unnecessary. > +} > + > void > binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > { > @@ -721,49 +865,62 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > > const struct sbrec_port_binding *binding_rec; > struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); > - 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 (b_ctx_in->br_int) { > - get_local_iface_ids(b_ctx_in->br_int, &lport_to_iface, > - b_ctx_out->local_lports, &egress_ifaces); > + build_local_bindings_from_local_ifaces(b_ctx_in, b_ctx_out); > } > > - /* Array to store pointers to local virtual ports. It is populated by > - * consider_local_datapath. > - */ > - const struct sbrec_port_binding **vpbs = NULL; > - size_t n_vpbs = 0; > - size_t n_allocated_vpbs = 0; > + struct hmap *qos_map_ptr = > + !sset_is_empty(b_ctx_out->egress_ifaces) ? &qos_map : NULL; > > /* 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. > - * Virtual ports are just added to vpbs array and will be processed > - * later. This is special case for virtual ports is needed in order to > - * make sure we update the virtual_parent port bindings first. > + * directly connected logical ports and children of those ports > + * (which also includes virtual ports). > */ > SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, > b_ctx_in->port_binding_table) { > - const struct ovsrec_interface *iface_rec > - = shash_find_data(&lport_to_iface, binding_rec->logical_port); > - vpbs = > - consider_local_datapath(binding_rec, > - sset_is_empty(&egress_ifaces) ? NULL : > - &qos_map, iface_rec, b_ctx_in, b_ctx_out, > - vpbs, &n_vpbs, &n_allocated_vpbs); > - } > + if (!strcmp(binding_rec->type, "patch") || > + !strcmp(binding_rec->type, "localport") || > + !strcmp(binding_rec->type, "vtep")) { > + update_local_lport_ids(b_ctx_out->local_lport_ids, binding_rec); > + continue; > + } > > - /* Now also update the virtual ports in case their parent ports were > - * updated above. > - */ > - for (size_t i = 0; i < n_vpbs; i++) { > - consider_local_virtual_port(b_ctx_in->sbrec_port_binding_by_name, > - b_ctx_in->chassis_rec, vpbs[i]); > + bool consider_for_vif = false; > + struct local_binding *lbinding = NULL; > + enum local_binding_type binding_type = BT_VIF; > + if (!binding_rec->type[0]) { > + if (!binding_rec->parent_port || !binding_rec->parent_port[0]) { > + lbinding = local_binding_find(b_ctx_out->local_bindings, > + binding_rec->logical_port); > + } else { > + lbinding = local_binding_find(b_ctx_out->local_bindings, > + binding_rec->parent_port); > + binding_type = BT_CHILD; > + } > + > + consider_for_vif = true; > + } else if (!strcmp(binding_rec->type, "virtual") && > + binding_rec->virtual_parent && > + binding_rec->virtual_parent[0]) { > + lbinding = local_binding_find(b_ctx_out->local_bindings, > + binding_rec->virtual_parent); > + consider_for_vif = true; > + binding_type = BT_VIRTUAL; > + } > + > + if (consider_for_vif) { > + consider_port_binding_for_vif(binding_rec, b_ctx_in, > + binding_type, lbinding, b_ctx_out, > + qos_map_ptr); > + } else { > + consider_port_binding(binding_rec, b_ctx_in, b_ctx_out, > + qos_map_ptr); > + } > } > - free(vpbs); > > add_ovs_bridge_mappings(b_ctx_in->ovs_table, b_ctx_in->bridge_table, > &bridge_mappings); > @@ -775,49 +932,39 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > b_ctx_in->port_binding_table) { > if (!strcmp(binding_rec->type, "localnet")) { > consider_localnet_port(binding_rec, &bridge_mappings, > - &egress_ifaces, b_ctx_out->local_datapaths); > + b_ctx_out->egress_ifaces, > + b_ctx_out->local_datapaths); > } > } > shash_destroy(&bridge_mappings); > > - if (!sset_is_empty(&egress_ifaces) > + if (!sset_is_empty(b_ctx_out->egress_ifaces) > && set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > - b_ctx_in->qos_table, &egress_ifaces)) { > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { > const char *entry; > - SSET_FOR_EACH (entry, &egress_ifaces) { > + SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > setup_qos(entry, &qos_map); > } > } > > - shash_destroy(&lport_to_iface); > - sset_destroy(&egress_ifaces); > hmap_destroy(&qos_map); > } > > /* Returns true if port-binding changes potentially require flow changes on > * the current chassis. Returns false if we are sure there is no impact. */ > bool > -binding_evaluate_port_binding_changes( > - const struct sbrec_port_binding_table *pb_table, > - const struct ovsrec_bridge *br_int, > - const struct sbrec_chassis *chassis_rec, > - struct sset *active_tunnels, > - struct sset *local_lports) > +binding_evaluate_port_binding_changes(struct binding_ctx_in *b_ctx_in, > + struct binding_ctx_out *b_ctx_out) > { > - if (!chassis_rec) { > + if (!b_ctx_in->chassis_rec) { > return true; > } > > bool changed = false; > > 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); > - if (br_int) { > - get_local_iface_ids(br_int, &lport_to_iface, local_lports, > - &egress_ifaces); > - } > - SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding_rec, pb_table) { > + SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding_rec, > + b_ctx_in->port_binding_table) { > /* XXX: currently OVSDB change tracking doesn't support getting old > * data when the operation is update, so if a port-binding moved from > * this chassis to another, there is no easy way to find out the > @@ -829,24 +976,31 @@ binding_evaluate_port_binding_changes( > * interface table will be updated, which will trigger recompute. > * > * - If the port is not a regular VIF, always trigger recompute. */ > - if (binding_rec->chassis == chassis_rec) { > + if (binding_rec->chassis == b_ctx_in->chassis_rec) { > changed = true; > break; > } > > - const struct ovsrec_interface *iface_rec > - = shash_find_data(&lport_to_iface, binding_rec->logical_port); > - if (is_our_chassis(chassis_rec, binding_rec, active_tunnels, iface_rec, > - local_lports) || > - (strcmp(binding_rec->type, "") && strcmp(binding_rec->type, > - "remote"))) { > + if (strcmp(binding_rec->type, "")) { strcmp(binding_rec->type, "remote") should be kept here. > + changed = true; > + break; > + } > + > + struct local_binding *lbinding = NULL; > + if (!binding_rec->parent_port || !binding_rec->parent_port[0]) { > + lbinding = local_binding_find(b_ctx_out->local_bindings, > + binding_rec->logical_port); > + } else { > + lbinding = local_binding_find(b_ctx_out->local_bindings, > + binding_rec->parent_port); > + } > + > + if (lbinding) { > changed = true; > break; > } > } > > - shash_destroy(&lport_to_iface); > - sset_destroy(&egress_ifaces); > return changed; > } > > diff --git a/controller/binding.h b/controller/binding.h > index d0b8331af..6bee1d12e 100644 > --- a/controller/binding.h > +++ b/controller/binding.h > @@ -31,6 +31,7 @@ struct ovsrec_open_vswitch_table; > struct sbrec_chassis; > struct sbrec_port_binding_table; > struct sset; > +struct sbrec_port_binding; > > struct binding_ctx_in { > struct ovsdb_idl_txn *ovnsb_idl_txn; > @@ -51,8 +52,10 @@ struct binding_ctx_in { > > struct binding_ctx_out { > struct hmap *local_datapaths; > + struct shash *local_bindings; > struct sset *local_lports; > struct sset *local_lport_ids; > + struct sset *egress_ifaces; > }; > > void binding_register_ovs_idl(struct ovsdb_idl *); > @@ -60,11 +63,10 @@ void binding_run(struct binding_ctx_in *, struct binding_ctx_out *); > bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, > const struct sbrec_port_binding_table *, > const struct sbrec_chassis *); > -bool binding_evaluate_port_binding_changes( > - const struct sbrec_port_binding_table *, > - const struct ovsrec_bridge *br_int, > - const struct sbrec_chassis *, > - struct sset *active_tunnels, > - struct sset *local_lports); > - > +bool binding_evaluate_port_binding_changes(struct binding_ctx_in *, > + struct binding_ctx_out *); > +void local_bindings_destroy(struct shash *local_bindings); > +void binding_add_vport_to_local_bindings( > + struct shash *local_bindings, const struct sbrec_port_binding *parent, > + const struct sbrec_port_binding *vport); > #endif /* controller/binding.h */ > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 24c89e617..fdfa60e9c 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -958,6 +958,9 @@ struct ed_type_runtime_data { > /* Contains "struct local_datapath" nodes. */ > struct hmap local_datapaths; > > + /* Contains "struct local_bindings" nodes. */ > + struct shash local_bindings; > + > /* Contains the name of each logical port resident on the local > * hypervisor. These logical ports include the VIFs (and their child > * logical ports, if any) that belong to VMs running on the hypervisor, > @@ -969,6 +972,8 @@ struct ed_type_runtime_data { > * <datapath-tunnel-key>_<port-tunnel-key> */ > struct sset local_lport_ids; > struct sset active_tunnels; > + > + struct sset egress_ifaces; > }; > > static void * > @@ -981,6 +986,8 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, > sset_init(&data->local_lports); > sset_init(&data->local_lport_ids); > sset_init(&data->active_tunnels); > + sset_init(&data->egress_ifaces); > + shash_init(&data->local_bindings); > return data; > } > > @@ -992,6 +999,7 @@ en_runtime_data_cleanup(void *data) > sset_destroy(&rt_data->local_lports); > sset_destroy(&rt_data->local_lport_ids); > sset_destroy(&rt_data->active_tunnels); > + sset_init(&rt_data->egress_ifaces); Should this be sset_destroy? > struct local_datapath *cur_node, *next_node; > HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node, > &rt_data->local_datapaths) { > @@ -1000,6 +1008,7 @@ en_runtime_data_cleanup(void *data) > free(cur_node); > } > hmap_destroy(&rt_data->local_datapaths); > + local_bindings_destroy(&rt_data->local_bindings); > } > > static void > @@ -1072,6 +1081,8 @@ init_binding_ctx(struct engine_node *node, > b_ctx_out->local_datapaths = &rt_data->local_datapaths; > b_ctx_out->local_lports = &rt_data->local_lports; > b_ctx_out->local_lport_ids = &rt_data->local_lport_ids; > + b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; > + b_ctx_out->local_bindings = &rt_data->local_bindings; > } > > static void > @@ -1098,9 +1109,11 @@ en_runtime_data_run(struct engine_node *node, void *data) > sset_destroy(local_lports); > sset_destroy(local_lport_ids); > sset_destroy(active_tunnels); > + sset_destroy(&rt_data->egress_ifaces); Should the local_bindings structure get reset here as well? > sset_init(local_lports); > sset_init(local_lport_ids); > sset_init(active_tunnels); > + sset_init(&rt_data->egress_ifaces); > } > > struct binding_ctx_in b_ctx_in; > @@ -1129,35 +1142,12 @@ static bool > runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) > { > struct ed_type_runtime_data *rt_data = data; > - struct sset *local_lports = &rt_data->local_lports; > - struct sset *active_tunnels = &rt_data->active_tunnels; > - > - struct ovsrec_open_vswitch_table *ovs_table = > - (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET( > - engine_get_input("OVS_open_vswitch", node)); > - struct ovsrec_bridge_table *bridge_table = > - (struct ovsrec_bridge_table *)EN_OVSDB_GET( > - engine_get_input("OVS_bridge", node)); > - const char *chassis_id = chassis_get_id(); > - const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table); > - > - ovs_assert(br_int && chassis_id); > - > - struct ovsdb_idl_index *sbrec_chassis_by_name = > - engine_ovsdb_node_get_index( > - engine_get_input("SB_chassis", node), > - "name"); > - > - const struct sbrec_chassis *chassis > - = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); > - ovs_assert(chassis); > - > - struct sbrec_port_binding_table *pb_table = > - (struct sbrec_port_binding_table *)EN_OVSDB_GET( > - engine_get_input("SB_port_binding", node)); > + struct binding_ctx_in b_ctx_in; > + struct binding_ctx_out b_ctx_out; > + init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); > > - bool changed = binding_evaluate_port_binding_changes( > - pb_table, br_int, chassis, active_tunnels, local_lports); > + bool changed = binding_evaluate_port_binding_changes(&b_ctx_in, > + &b_ctx_out); > > return !changed; > } > diff --git a/controller/pinctrl.c b/controller/pinctrl.c The change in pinctrl.h and pinctrl.c seem not needed. It may be left from the earlier versions. > index 3230bb386..824be5e7a 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -18,6 +18,7 @@ > > #include "pinctrl.h" > > +#include "binding.h" > #include "coverage.h" > #include "csum.h" > #include "dirs.h" > diff --git a/controller/pinctrl.h b/controller/pinctrl.h > index 8fa4baae9..12fb3b080 100644 > --- a/controller/pinctrl.h > +++ b/controller/pinctrl.h > @@ -31,6 +31,7 @@ struct sbrec_chassis; > struct sbrec_dns_table; > struct sbrec_controller_event_table; > struct sbrec_service_monitor_table; > +struct shash; > > void pinctrl_init(void); > void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > -- > 2.25.4 > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
