| Message ID | 1574432545-31875-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn] ovn-controller: Consider non-virtual ports first when updating bindings. | expand |
Acked-by: Mark Michelson <mmichels@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 <numans@ovn.org> > Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > 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); > >
On Sat, Nov 23, 2019 at 3:51 AM Mark Michelson <mmichels@redhat.com> wrote: > > Acked-by: Mark Michelson <mmichels@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 <numans@ovn.org> > > Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'") > > Signed-off-by: Dumitru Ceara <dceara@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Thu, Nov 28, 2019 at 2:36 PM Numan Siddique <numans@ovn.org> wrote: > > On Sat, Nov 23, 2019 at 3:51 AM Mark Michelson <mmichels@redhat.com> wrote: > > > > Acked-by: Mark Michelson <mmichels@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 <numans@ovn.org> > > > Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'") > > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > Thanks. I applied this patch to master. > > Numan Thanks Mark and 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
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);
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 <numans@ovn.org> Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- controller/binding.c | 97 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 69 insertions(+), 28 deletions(-)