[ovs-dev,v3,1/2] controller: incrementally create ipv6 prefix delegation port_binding list

Incrementally manage local_active_ports_ipv6_pd map for interfaces
where IPv6 prefix-delegation has been enabled. This patch allows to
avoid looping over all local interfaces to check if prefix-delegation
is running on the current port binding.

Acked-by: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 controller/binding.c        |  32 +++++++++++
 controller/binding.h        |   1 +
 controller/ovn-controller.c |  11 +++-
 controller/ovn-controller.h |   6 ++
 controller/pinctrl.c        | 107 +++++++++++++++++-------------------
 controller/pinctrl.h        |   4 +-
 6 files changed, 103 insertions(+), 58 deletions(-)

Sorry Lorenzo but I found one more issue. Sorry for not noticing it 
during an earlier review.

On 7/12/21 2:18 PM, Lorenzo Bianconi wrote:
> Incrementally manage local_active_ports_ipv6_pd map for interfaces
> where IPv6 prefix-delegation has been enabled. This patch allows to
> avoid looping over all local interfaces to check if prefix-delegation
> is running on the current port binding.
> 
> Acked-by: Mark Michelson <mmichels@redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>   controller/binding.c        |  32 +++++++++++
>   controller/binding.h        |   1 +
>   controller/ovn-controller.c |  11 +++-
>   controller/ovn-controller.h |   6 ++
>   controller/pinctrl.c        | 107 +++++++++++++++++-------------------
>   controller/pinctrl.h        |   4 +-
>   6 files changed, 103 insertions(+), 58 deletions(-)
> 
> diff --git a/controller/binding.c b/controller/binding.c
> index 594babc98..9711ac850 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -574,6 +574,30 @@ remove_related_lport(const struct sbrec_port_binding *pb,
>       }
>   }
>   
> +static void
> +update_active_pb_ras_pd(const struct sbrec_port_binding *pb,
> +                        struct hmap *local_datapaths,
> +                        struct shash *map, const char *conf)
> +{
> +    bool ras_pd_conf = smap_get_bool(&pb->options, conf, false);
> +    struct shash_node *iter = shash_find(map, pb->logical_port);
> +
> +    if (iter && !ras_pd_conf) {
> +        shash_delete(map, iter);

There's a memory leak here. iter->data needs to be freed.

> +        return;
> +    }
> +    struct pb_ld_binding *ras_pd = NULL;
> +    if (!iter && ras_pd_conf) {
> +        ras_pd = xzalloc(sizeof *ras_pd);
> +        ras_pd->pb = pb;
> +        shash_add(map, pb->logical_port, ras_pd);
> +    }
> +    if (ras_pd) {

The logic here has changed since the first version of the patch, and I 
think it's wrong now. This now will only update ras_pd->ld if ras_pd was 
allocated during this function call. Previously, ld would be updated 
when the pb_ld_binding was found in the map. I think this is a bit 
confusing since you're dealing both with shash_node and pb_ld_binding 
types in this function. I think you can do something like this:

if (iter && !ras_pd_conf) {
    /* delete iter from map */
    return;
}
struct pb_ld_binding *ras_pd = NULL;
if (ras_pd_conf) {
     if (iter) {
         ras_pd = iter->data;
     } else {
         /* allocate ras_pd and add it to map */
     }
     ovs_assert(ras_pd);
     ras_pd->ld = get_local_datapath(...);
}

> +        ras_pd->ld = get_local_datapath(local_datapaths,
> +                                        pb->datapath->tunnel_key);
> +    }
> +}
> +
>   /* Corresponds to each Port_Binding.type. */
>   enum en_lport_type {
>       LP_UNKNOWN,
> @@ -1645,6 +1669,10 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out)
>       const struct sbrec_port_binding *pb;
>       SBREC_PORT_BINDING_TABLE_FOR_EACH (pb,
>                                          b_ctx_in->port_binding_table) {
> +        update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths,
> +                                b_ctx_out->local_active_ports_ipv6_pd,
> +                                "ipv6_prefix_delegation");
> +
>           enum en_lport_type lport_type = get_lport_type(pb);
>   
>           switch (lport_type) {
> @@ -2482,6 +2510,10 @@ delete_done:
>               continue;
>           }
>   
> +        update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths,
> +                                b_ctx_out->local_active_ports_ipv6_pd,
> +                                "ipv6_prefix_delegation");
> +
>           enum en_lport_type lport_type = get_lport_type(pb);
>   
>           struct binding_lport *b_lport =
> diff --git a/controller/binding.h b/controller/binding.h
> index a08011ae2..60ad49da0 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -72,6 +72,7 @@ void related_lports_destroy(struct related_lports *);
>   
>   struct binding_ctx_out {
>       struct hmap *local_datapaths;
> +    struct shash *local_active_ports_ipv6_pd;
>       struct local_binding_data *lbinding_data;
>   
>       /* sset of (potential) local lports. */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 6a9c25f28..c4eb54755 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1029,6 +1029,8 @@ struct ed_type_runtime_data {
>       bool tracked;
>       bool local_lports_changed;
>       struct hmap tracked_dp_bindings;
> +
> +    struct shash local_active_ports_ipv6_pd;
>   };
>   
>   /* struct ed_type_runtime_data has the below members for tracking the
> @@ -1116,6 +1118,7 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED,
>       sset_init(&data->egress_ifaces);
>       smap_init(&data->local_iface_ids);
>       local_binding_data_init(&data->lbinding_data);
> +    shash_init(&data->local_active_ports_ipv6_pd);
>   
>       /* Init the tracked data. */
>       hmap_init(&data->tracked_dp_bindings);
> @@ -1141,6 +1144,7 @@ en_runtime_data_cleanup(void *data)
>           free(cur_node);
>       }
>       hmap_destroy(&rt_data->local_datapaths);
> +    shash_destroy(&rt_data->local_active_ports_ipv6_pd);

Change this to shash_destroy_free_data(). Otherwise, the data portions 
of all of the shash_nodes will be leaked.

>       local_binding_data_destroy(&rt_data->lbinding_data);
>   }
>   
> @@ -1219,6 +1223,8 @@ init_binding_ctx(struct engine_node *node,
>       b_ctx_in->ovs_table = ovs_table;
>   
>       b_ctx_out->local_datapaths = &rt_data->local_datapaths;
> +    b_ctx_out->local_active_ports_ipv6_pd =
> +        &rt_data->local_active_ports_ipv6_pd;
>       b_ctx_out->local_lports = &rt_data->local_lports;
>       b_ctx_out->local_lports_changed = false;
>       b_ctx_out->related_lports = &rt_data->related_lports;
> @@ -1236,6 +1242,7 @@ en_runtime_data_run(struct engine_node *node, void *data)
>   {
>       struct ed_type_runtime_data *rt_data = data;
>       struct hmap *local_datapaths = &rt_data->local_datapaths;
> +    struct shash *local_active_ipv6_pd = &rt_data->local_active_ports_ipv6_pd;
>       struct sset *local_lports = &rt_data->local_lports;
>       struct sset *active_tunnels = &rt_data->active_tunnels;
>   
> @@ -1251,6 +1258,7 @@ en_runtime_data_run(struct engine_node *node, void *data)
>               free(cur_node);
>           }
>           hmap_clear(local_datapaths);
> +        shash_clear(local_active_ipv6_pd);
>           local_binding_data_destroy(&rt_data->lbinding_data);
>           sset_destroy(local_lports);
>           related_lports_destroy(&rt_data->related_lports);
> @@ -3265,7 +3273,8 @@ main(int argc, char *argv[])
>                                       sbrec_bfd_table_get(ovnsb_idl_loop.idl),
>                                       br_int, chassis,
>                                       &runtime_data->local_datapaths,
> -                                    &runtime_data->active_tunnels);
> +                                    &runtime_data->active_tunnels,
> +                                    &runtime_data->local_active_ports_ipv6_pd);
>                           /* Updating monitor conditions if runtime data or
>                            * logical datapath goups changed. */
>                           if (engine_node_changed(&en_runtime_data)
> diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h
> index 5d9466880..417c7aacb 100644
> --- a/controller/ovn-controller.h
> +++ b/controller/ovn-controller.h
> @@ -87,4 +87,10 @@ enum chassis_tunnel_type {
>   
>   uint32_t get_tunnel_type(const char *name);
>   
> +struct pb_ld_binding {
> +    const struct sbrec_port_binding *pb;
> +    const struct local_datapath *ld;
> +    struct hmap_node hmap_node;
> +};
> +
>   #endif /* controller/ovn-controller.h */
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 78ecfed84..5f3eca4a0 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -1249,83 +1249,76 @@ fill_ipv6_prefix_state(struct ovsdb_idl_txn *ovnsb_idl_txn,
>   static void
>   prepare_ipv6_prefixd(struct ovsdb_idl_txn *ovnsb_idl_txn,
>                        struct ovsdb_idl_index *sbrec_port_binding_by_name,
> -                     const struct hmap *local_datapaths,
> +                     const struct shash *local_active_ports_ipv6_pd,
>                        const struct sbrec_chassis *chassis,
>                        const struct sset *active_tunnels)
>       OVS_REQUIRES(pinctrl_mutex)
>   {
> -    const struct local_datapath *ld;
>       bool changed = false;
>   
> -    HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
> -        if (datapath_is_switch(ld->datapath)) {
> -            /* logical switch */
> +    struct shash_node *iter;
> +    SHASH_FOR_EACH (iter, local_active_ports_ipv6_pd) {
> +        const struct pb_ld_binding *pb_ipv6 = iter->data;
> +        const struct sbrec_port_binding *pb = pb_ipv6->pb;
> +        int j;
> +
> +        if (!pb_ipv6->ld) {
>               continue;
>           }
>   
> -        for (size_t i = 0; i < ld->n_peer_ports; i++) {
> -            const struct sbrec_port_binding *pb = ld->peer_ports[i].local;
> -            int j;
> +        const char *peer_s = smap_get(&pb->options, "peer");
> +        if (!peer_s) {
> +            continue;
> +        }
>   
> -            if (!smap_get_bool(&pb->options, "ipv6_prefix_delegation",
> -                               false)) {
> -                continue;
> -            }
> +        const struct sbrec_port_binding *peer
> +            = lport_lookup_by_name(sbrec_port_binding_by_name, peer_s);
> +        if (!peer) {
> +            continue;
> +        }
>   
> -            const char *peer_s = smap_get(&pb->options, "peer");
> -            if (!peer_s) {
> -                continue;
> -            }
> +        char *redirect_name = xasprintf("cr-%s", pb->logical_port);
> +        bool resident = lport_is_chassis_resident(
> +                sbrec_port_binding_by_name, chassis, active_tunnels,
> +                redirect_name);
> +        free(redirect_name);
> +        if (!resident && strcmp(pb->type, "l3gateway")) {
> +            continue;
> +        }
>   
> -            const struct sbrec_port_binding *peer
> -                = lport_lookup_by_name(sbrec_port_binding_by_name, peer_s);
> -            if (!peer) {
> -                continue;
> -            }
> +        struct in6_addr ip6_addr;
> +        struct eth_addr ea = eth_addr_zero;
> +        for (j = 0; j < pb->n_mac; j++) {
> +            struct lport_addresses laddrs;
>   
> -            char *redirect_name = xasprintf("cr-%s", pb->logical_port);
> -            bool resident = lport_is_chassis_resident(
> -                    sbrec_port_binding_by_name, chassis, active_tunnels,
> -                    redirect_name);
> -            free(redirect_name);
> -            if (!resident && strcmp(pb->type, "l3gateway")) {
> +            if (!extract_lsp_addresses(pb->mac[j], &laddrs)) {
>                   continue;
>               }
>   
> -            struct in6_addr ip6_addr;
> -            struct eth_addr ea = eth_addr_zero;
> -            for (j = 0; j < pb->n_mac; j++) {
> -                struct lport_addresses laddrs;
> -
> -                if (!extract_lsp_addresses(pb->mac[j], &laddrs)) {
> -                    continue;
> -                }
> -
> -                ea = laddrs.ea;
> -                if (laddrs.n_ipv6_addrs > 0) {
> -                    ip6_addr = laddrs.ipv6_addrs[0].addr;
> -                    destroy_lport_addresses(&laddrs);
> -                    break;
> -                }
> +            ea = laddrs.ea;
> +            if (laddrs.n_ipv6_addrs > 0) {
> +                ip6_addr = laddrs.ipv6_addrs[0].addr;
>                   destroy_lport_addresses(&laddrs);
> +                break;
>               }
> +            destroy_lport_addresses(&laddrs);
> +        }
>   
> -            if (eth_addr_is_zero(ea)) {
> -                continue;
> -            }
> -
> -            if (j == pb->n_mac) {
> -                in6_generate_lla(ea, &ip6_addr);
> -            }
> +        if (eth_addr_is_zero(ea)) {
> +            continue;
> +        }
>   
> -            changed |= fill_ipv6_prefix_state(ovnsb_idl_txn, ld,
> -                                              ea, ip6_addr,
> -                                              peer->tunnel_key,
> -                                              peer->datapath->tunnel_key);
> +        if (j == pb->n_mac) {
> +            in6_generate_lla(ea, &ip6_addr);
>           }
> +
> +        changed |= fill_ipv6_prefix_state(ovnsb_idl_txn, pb_ipv6->ld,
> +                                          ea, ip6_addr,
> +                                          peer->tunnel_key,
> +                                          peer->datapath->tunnel_key);
>       }
>   
> -    struct shash_node *iter, *next;
> +    struct shash_node *next;
>       SHASH_FOR_EACH_SAFE (iter, next, &ipv6_prefixd) {
>           struct ipv6_prefixd_state *pfd = iter->data;
>           if (pfd->last_used + IPV6_PREFIXD_STALE_TIMEOUT < time_msec()) {
> @@ -3411,7 +3404,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>               const struct ovsrec_bridge *br_int,
>               const struct sbrec_chassis *chassis,
>               const struct hmap *local_datapaths,
> -            const struct sset *active_tunnels)
> +            const struct sset *active_tunnels,
> +            const struct shash *local_active_ports_ipv6_pd)
>   {
>       ovs_mutex_lock(&pinctrl_mutex);
>       pinctrl_set_br_int_name_(br_int->name);
> @@ -3426,7 +3420,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>                              local_datapaths, active_tunnels);
>       prepare_ipv6_ras(local_datapaths);
>       prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name,
> -                         local_datapaths, chassis, active_tunnels);
> +                         local_active_ports_ipv6_pd, chassis,
> +                         active_tunnels);
>       sync_dns_cache(dns_table);
>       controller_event_run(ovnsb_idl_txn, ce_table, chassis);
>       ip_mcast_sync(ovnsb_idl_txn, chassis, local_datapaths,
> diff --git a/controller/pinctrl.h b/controller/pinctrl.h
> index cc0a51984..0b9a57bdd 100644
> --- a/controller/pinctrl.h
> +++ b/controller/pinctrl.h
> @@ -23,6 +23,7 @@
>   #include "openvswitch/meta-flow.h"
>   
>   struct hmap;
> +struct shash;
>   struct lport_index;
>   struct ovsdb_idl_index;
>   struct ovsdb_idl_txn;
> @@ -49,7 +50,8 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>                    const struct sbrec_bfd_table *,
>                    const struct ovsrec_bridge *, const struct sbrec_chassis *,
>                    const struct hmap *local_datapaths,
> -                 const struct sset *active_tunnels);
> +                 const struct sset *active_tunnels,
> +                 const struct shash *local_active_ports_ipv6_pd);
>   void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn);
>   void pinctrl_destroy(void);
>   void pinctrl_set_br_int_name(char *br_int_name);
>