[ovs-dev] [PATCH ovn 2/4] ovn-controller: Remove ports from struct local_datapaths.
Numan Siddique
numans at ovn.org
Tue Jan 28 14:12:20 UTC 2020
On Tue, Jan 28, 2020 at 12:51 PM Han Zhou <hzhou at ovn.org> wrote:
>
> On Fri, Jan 24, 2020 at 3:03 AM <numans at ovn.org> wrote:
> >
> > From: Numan Siddique <numans at ovn.org>
> >
> > struct local_datapaths stores the array of port bindings for each
> datapath.
> > These ports are used only in the pinctrl module to check if a mac binding
> > has been learnt for the buffered packets.
> >
> > MAC bindings are always learnt in the router pipeline and so
> > logical_port column of MAC_Binding table will always refer to a
> > logical router port. run_buffered_binding() of pinctrl module can use
> > the peer ports stored in the struct local_datapaths instead.
> > This would save many calls to mac_binding_lookup().
> >
> > This patch doesn't store the array of port bindings for each local
> > datapath as it is not required at all.
> >
> > Earlier, the peer ports were stored only for patch port bindings. But we
> can have
> > peer ports even for l3gateway port bindings. This patch now considers
> l3gateway
> > ports also for storing the peer ports in struct local_datapaths.
>
> I think l3gateway port was not needed because the separate array of port
> bindings was used. Now that you remove the port bindings array, adding
> peers for l3gateway port bindings are necessary. However, there is a
> problem. Please see my comment below.
>
> >
> > Signed-off-by: Numan Siddique <numans at ovn.org>
> > ---
> > controller/binding.c | 9 +--------
> > controller/ovn-controller.c | 2 --
> > controller/ovn-controller.h | 4 ----
> > controller/pinctrl.c | 11 +++++++++--
> > 4 files changed, 10 insertions(+), 16 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index 4c107c1af..8ab23203b 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -146,7 +146,7 @@ add_local_datapath__(struct ovsdb_idl_index
> *sbrec_datapath_binding_by_key,
> > const struct sbrec_port_binding *pb;
> > SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target,
> > sbrec_port_binding_by_datapath) {
> > - if (!strcmp(pb->type, "patch")) {
> > + if (!strcmp(pb->type, "patch") || !strcmp(pb->type,
> "l3gateway")) {
>
> This change will have a side-effect. Originally, the datapaths connected by
> l3gateway ports, i.e. logical switches on the other side of gateway router,
> are not added to local datapaths. That was a desired behavior, since
> gateway router is centralized on gateway node and the HVs should not care
> about those datapaths.
>
> In particular, ovn-kubernetes uses gateway routers, one on each k8s node,
> and they are all connected by a join-switch. Without this patch, each node
> only creates a single pair of patch ports in br-int. With this patch, it
> would create N pairs of patch ports since it treats all the datapaths as
> "local", even if the datapath is only connected to a gateway router on
> another chassis.
>
> To solve this problem, we can still add this condition here so that we can
> add the peer ports, but don't call add_local_datapath__() so that the
> datapath is not added as local datapath.
Great observersion, Thanks Han.
Does the below modification looks good to you ?
diff --git a/controller/binding.c b/controller/binding.c
index 8ab23203b..502e92479 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -155,11 +155,18 @@ add_local_datapath__(struct ovsdb_idl_index
*sbrec_datapath_binding_by_key,
peer_name);
if (peer && peer->datapath) {
- add_local_datapath__(sbrec_datapath_binding_by_key,
- sbrec_port_binding_by_datapath,
- sbrec_port_binding_by_name,
- peer->datapath, false,
- depth + 1, local_datapaths);
+ if (!strcmp(pb->type, "patch")) {
+ /* Add the datapath to local datapath only for patch
+ * ports. For l3gateway ports, since gateway router
+ * port resides on one chassis, we don't need to add.
+ * Otherwise, all the chassis will create patch ports
+ * between br-int and the provider bridge. */
+ add_local_datapath__(sbrec_datapath_binding_by_key,
+ sbrec_port_binding_by_datapath,
+ sbrec_port_binding_by_name,
+ peer->datapath, false,
+ depth + 1, local_datapaths);
+ }
ld->n_peer_ports++;
if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
ld->peer_ports =
I can submit v3 if you prefer. Please let me know.
Thanks for the review.
Numan
>
> > const char *peer_name = smap_get(&pb->options, "peer");
> > if (peer_name) {
> > const struct sbrec_port_binding *peer;
> > @@ -172,13 +172,6 @@ add_local_datapath__(struct ovsdb_idl_index
> *sbrec_datapath_binding_by_key,
> > }
> > }
> > }
> > -
> > - ld->n_ports++;
> > - if (ld->n_ports > ld->n_allocated_ports) {
> > - ld->ports = x2nrealloc(ld->ports, &ld->n_allocated_ports,
> > - sizeof *ld->ports);
> > - }
> > - ld->ports[ld->n_ports - 1] = pb;
> > }
> > sbrec_port_binding_index_destroy_row(target);
> > }
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index d81c7574d..9b88f88fe 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -965,7 +965,6 @@ en_runtime_data_cleanup(void *data)
> > HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node,
> > &rt_data->local_datapaths) {
> > free(cur_node->peer_ports);
> > - free(cur_node->ports);
> > hmap_remove(&rt_data->local_datapaths, &cur_node->hmap_node);
> > free(cur_node);
> > }
> > @@ -989,7 +988,6 @@ en_runtime_data_run(struct engine_node *node, void
> *data)
> > struct local_datapath *cur_node, *next_node;
> > HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node,
> local_datapaths) {
> > free(cur_node->peer_ports);
> > - free(cur_node->ports);
> > hmap_remove(local_datapaths, &cur_node->hmap_node);
> > free(cur_node);
> > }
> > diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h
> > index 86b300e44..5d9466880 100644
> > --- a/controller/ovn-controller.h
> > +++ b/controller/ovn-controller.h
> > @@ -60,10 +60,6 @@ struct local_datapath {
> > * hypervisor. */
> > bool has_local_l3gateway;
> >
> > - const struct sbrec_port_binding **ports;
> > - size_t n_ports;
> > - size_t n_allocated_ports;
> > -
> > struct {
> > const struct sbrec_port_binding *local;
> > const struct sbrec_port_binding *remote;
> > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > index 452ca8a1c..5825bb14b 100644
> > --- a/controller/pinctrl.c
> > +++ b/controller/pinctrl.c
> > @@ -2957,10 +2957,17 @@ run_buffered_binding(struct ovsdb_idl_index
> *sbrec_mac_binding_by_lport_ip,
> > bool notify = false;
> >
> > HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
> > + /* MAC_Binding.logical_port will always belong to a
> > + * a router datapath. Hence we can skip logical switch
> > + * datapaths.
> > + * */
> > + if (datapath_is_switch(ld->datapath)) {
> > + continue;
> > + }
> >
> > - for (size_t i = 0; i < ld->n_ports; i++) {
> > + for (size_t i = 0; i < ld->n_peer_ports; i++) {
> >
> > - const struct sbrec_port_binding *pb = ld->ports[i];
> > + const struct sbrec_port_binding *pb =
> ld->peer_ports[i].local;
> > struct buffered_packets *cur_qp, *next_qp;
> > HMAP_FOR_EACH_SAFE (cur_qp, next_qp, hmap_node,
> > &buffered_packets_map) {
> > --
> > 2.24.1
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list