| Message ID | 20200124110329.1555571-1-numans@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn,1/4] Make is_switch() in lflow.c a util function | expand |
On Fri, Jan 24, 2020 at 3:03 AM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@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@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. > 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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Tue, Jan 28, 2020 at 12:51 PM Han Zhou <hzhou@ovn.org> wrote: > > On Fri, Jan 24, 2020 at 3:03 AM <numans@ovn.org> wrote: > > > > From: Numan Siddique <numans@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@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@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Tue, Jan 28, 2020 at 7:42 PM Numan Siddique <numans@ovn.org> wrote: > > On Tue, Jan 28, 2020 at 12:51 PM Han Zhou <hzhou@ovn.org> wrote: > > > > On Fri, Jan 24, 2020 at 3:03 AM <numans@ovn.org> wrote: > > > > > > From: Numan Siddique <numans@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@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 > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Tue, Jan 28, 2020 at 6:14 AM Numan Siddique <numans@ovn.org> wrote: > > On Tue, Jan 28, 2020 at 7:42 PM Numan Siddique <numans@ovn.org> wrote: > > > > On Tue, Jan 28, 2020 at 12:51 PM Han Zhou <hzhou@ovn.org> wrote: > > > > > > On Fri, Jan 24, 2020 at 3:03 AM <numans@ovn.org> wrote: > > > > > > > > From: Numan Siddique <numans@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@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 > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > Looks good to me. Thanks! Acked-by: Han Zhou <hzhou@ovn.org>
On Tue, Jan 28, 2020 at 11:25 PM Han Zhou <hzhou@ovn.org> wrote: > > On Tue, Jan 28, 2020 at 6:14 AM Numan Siddique <numans@ovn.org> wrote: > > > > On Tue, Jan 28, 2020 at 7:42 PM Numan Siddique <numans@ovn.org> wrote: > > > > > > On Tue, Jan 28, 2020 at 12:51 PM Han Zhou <hzhou@ovn.org> wrote: > > > > > > > > On Fri, Jan 24, 2020 at 3:03 AM <numans@ovn.org> wrote: > > > > > > > > > > From: Numan Siddique <numans@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@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 > > > > > dev@openvswitch.org > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > > > > dev mailing list > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > Looks good to me. Thanks! > > Acked-by: Han Zhou <hzhou@ovn.org> Thanks. I applied this patch to master. Numan > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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")) { 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) {