Re: [ovs-dev] [PATCH ovn] ovn-controller: Consider non-virtual ports first when updating bindings.
On Thu, Nov 28, 2019 at 2:36 PM Numan Siddique wrote: > > On Sat, Nov 23, 2019 at 3:51 AM Mark Michelson wrote: > > > > Acked-by: Mark Michelson > > > > 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 > > > Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'") > > > Signed-off-by: Dumitru Ceara > > 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_chass
Re: [ovs-dev] [PATCH ovn] ovn-controller: Consider non-virtual ports first when updating bindings.
On Sat, Nov 23, 2019 at 3:51 AM Mark Michelson wrote: > > Acked-by: Mark Michelson > > 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 > > Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'") > > Signed-off-by: Dumitru Ceara 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 struc
Re: [ovs-dev] [PATCH ovn] ovn-controller: Consider non-virtual ports first when updating bindings.
Acked-by: Mark Michelson 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 Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'") Signed-off-by: Dumitru Ceara --- 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.", +