On Thu, May 7, 2020 at 12:54 PM Numan Siddique <[email protected]> wrote:
> > > On Thu, May 7, 2020 at 12:27 PM Han Zhou <[email protected]> wrote: > >> 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. >> > > I think when we create local variables inside switch case, we need {}, but > I think I should have used as .. case BT_VIRTUAL: { > ... > } > > I'll fix this in the next version. > > >> > + /* 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. >> > > I'm not intentionally rebuilding local_bindings during recompute. > My idea was to keep in sync rather than recreating it. Do you see any > issues with that ? > > I know that you would prefer recreating the resources at every recompute. > I'm fine > doing as you suggest, but I think it should be OK not to recreate as long > as we maintain correctness. > > Right now it would seem that the local_binding_find() will always fail. > But with the next patch which handles > ovs interface and port binding changes incrementally, that will not be the > case. > > Let me know what you think. > > I rethought about this and to be consistent with the other engine data, I changed to as per you suggested and I submitted v5. Kindly take a look - https://patchwork.ozlabs.org/project/openvswitch/list/?series=176066 I would like to revisit this later and see if it's really required to clean up all the runtime data in every recompute or it can be preserved. Thanks Numan Thanks > Numan > > >> >> > +} >> > + >> > 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. >> > > That's right. Thanks for pointing it out. I'll fix it in v5. > > Thanks > Numan > > >> > 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 >> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
