On Tue, Jan 28, 2020 at 12:51 PM Han Zhou <hz...@ovn.org> wrote: > > On Fri, Jan 24, 2020 at 3:03 AM <num...@ovn.org> wrote: > > > > From: Numan Siddique <num...@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 <num...@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 > > 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 > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev