[ovs-dev] [PATCH ovn 2/4] ovn-controller: Remove ports from struct local_datapaths.
Han Zhou
hzhou at ovn.org
Tue Jan 28 17:54:04 UTC 2020
On Tue, Jan 28, 2020 at 6:14 AM Numan Siddique <numans at ovn.org> wrote:
>
> On Tue, Jan 28, 2020 at 7:42 PM Numan Siddique <numans at ovn.org> wrote:
> >
> > 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 =
> >
> >
>
> 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 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
> > >
Looks good to me. Thanks!
Acked-by: Han Zhou <hzhou at ovn.org>
More information about the dev
mailing list