[ovs-dev] [PATCH ovn 2/4] ovn-controller: Remove ports from struct local_datapaths.

Han Zhou hzhou at ovn.org
Tue Jan 28 07:20:50 UTC 2020


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.

>              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


More information about the dev mailing list