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 <mmich...@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@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);
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
