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,
> > >
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
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.",
+
[ovs-dev] [PATCH ovn] ovn-controller: Consider non-virtual ports first when updating bindings.
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.",
+ binding_rec->logical_port);
+if (binding_rec->encap) {
+
