[ovs-dev] [PATCH v3 ovn 1/2] controller: incrementally create ipv6 prefix delegation port_binding list
Mark Michelson
mmichels at redhat.com
Mon Jul 12 22:06:13 UTC 2021
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 at redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at 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);
>
More information about the dev
mailing list