On Sat, Nov 23, 2019 at 3:51 AM Mark Michelson <mmich...@redhat.com> wrote: > > Acked-by: Mark Michelson <mmich...@redhat.com> > > On 11/22/19 9:22 AM, Dumitru Ceara wrote: > > There's no guarantee SBREC_PORT_BINDING_TABLE_FOR_EACH will first > > return the non-virtual ports and then virtual ports. In the case when a > > virtual port is processed before its virtual_parent, > > consider_local_datapath might not release it in the current > > ovn-controller iteration even though the virtual_parent gets released. > > > > Right now this doesn't trigger any functionality issue because releasing > > the virtual_parent triggers a change in the SB DB which will wake up > > ovn-controller and trigger a new computation which will also update the > > virtual port. > > > > However, this is suboptimal, and we can notice that often ovn-controller > > gets the SB update notification before the "transaction successful" > > notification. In such cases the incremental engine doesn't run > > (ovnsb_idl_txn == NULL) and a full recompute is scheduled for the next > > run. By batching the two SB updates in a single transaction we > > lower the risk of this situation happening. > > > > CC: Numan Siddique <num...@ovn.org> > > Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'") > > Signed-off-by: Dumitru Ceara <dce...@redhat.com>
Thanks. I applied this patch to master. Numan > > --- > > controller/binding.c | 97 > > +++++++++++++++++++++++++++++++++++++--------------- > > 1 file changed, 69 insertions(+), 28 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index aad9d39..4c107c1 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -472,7 +472,12 @@ is_our_chassis(const struct sbrec_chassis *chassis_rec, > > return our_chassis; > > } > > > > -static void > > +/* 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(struct ovsdb_idl_txn *ovnsb_idl_txn, > > struct ovsdb_idl_txn *ovs_idl_txn, > > struct ovsdb_idl_index > > *sbrec_datapath_binding_by_key, > > @@ -485,7 +490,9 @@ consider_local_datapath(struct ovsdb_idl_txn > > *ovnsb_idl_txn, > > struct hmap *local_datapaths, > > struct shash *lport_to_iface, > > struct sset *local_lports, > > - struct sset *local_lport_ids) > > + struct sset *local_lport_ids, > > + const struct sbrec_port_binding **vpbs, > > + size_t *n_vpbs, size_t *n_allocated_vpbs) > > { > > const struct ovsrec_interface *iface_rec > > = shash_find_data(lport_to_iface, binding_rec->logical_port); > > @@ -587,22 +594,11 @@ consider_local_datapath(struct ovsdb_idl_txn > > *ovnsb_idl_txn, > > } > > } else if (binding_rec->chassis == chassis_rec) { > > if (!strcmp(binding_rec->type, "virtual")) { > > - /* 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. */ > > - 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) { > > - 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); > > + 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); > > @@ -621,6 +617,30 @@ consider_local_datapath(struct ovsdb_idl_txn > > *ovnsb_idl_txn, > > 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) > > +{ > > + /* 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. > > + */ > > + 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) { > > + 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 > > @@ -718,20 +738,41 @@ binding_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > &egress_ifaces); > > } > > > > + /* 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; > > + > > /* 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. */ > > + * 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. > > + */ > > SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, port_binding_table) { > > - consider_local_datapath(ovnsb_idl_txn, ovs_idl_txn, > > - sbrec_datapath_binding_by_key, > > - sbrec_port_binding_by_datapath, > > - sbrec_port_binding_by_name, > > - active_tunnels, chassis_rec, binding_rec, > > - sset_is_empty(&egress_ifaces) ? NULL : > > - &qos_map, local_datapaths, &lport_to_iface, > > - local_lports, local_lport_ids); > > - > > - } > > + vpbs = > > + consider_local_datapath(ovnsb_idl_txn, ovs_idl_txn, > > + sbrec_datapath_binding_by_key, > > + sbrec_port_binding_by_datapath, > > + sbrec_port_binding_by_name, > > + active_tunnels, chassis_rec, > > binding_rec, > > + sset_is_empty(&egress_ifaces) ? NULL : > > + &qos_map, local_datapaths, > > &lport_to_iface, > > + local_lports, local_lport_ids, > > + vpbs, &n_vpbs, &n_allocated_vpbs); > > + } > > + > > + /* 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(sbrec_port_binding_by_name, > > chassis_rec, > > + vpbs[i]); > > + } > > + free(vpbs); > > > > add_ovs_bridge_mappings(ovs_table, bridge_table, &bridge_mappings); > > > > > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev