On Tue, Jan 28, 2020 at 11:25 PM Han Zhou <hz...@ovn.org> wrote:
>
> On Tue, Jan 28, 2020 at 6:14 AM Numan Siddique <num...@ovn.org> wrote:
> >
> > On Tue, Jan 28, 2020 at 7:42 PM Numan Siddique <num...@ovn.org> wrote:
> > >
> > > 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 =
> > >
> > >
> >
> > There's a mistake in the comments. Below is the right one
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index 8ab23203b..6e752dbcb 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
> > + * resides on one chassis, we don't need to add.
> > + * Otherwise, all other chassis might 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
> > > >
>
> Looks good to me. Thanks!
>
> Acked-by: Han Zhou <hz...@ovn.org>
Thanks. I applied this patch to master.
Numan
> _______________________________________________
> 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
