[ovs-dev] [PATCH ovn] lflow.c: Avoid adding redundant resource refs for port-bindings.
Han Zhou
hzhou at ovn.org
Tue Sep 15 08:15:57 UTC 2020
When a lport is referenced by a logical flow where port-binding refs
needs to be added, currently it can add the same reference pair multiple
times in below situations (introduced in commit ade4e77):
1) In add_matches_to_flow_table(), different matches from same lflow
can have same inport/outport.
2) In is_chassis_resident_cb(), a lflow can have multiple is_chassis_resident
check for same lport (although not very common), and at the same time
the lport used in is_chassis_resident can overlap with the inport/
outport of the same flow.
Now because of the redundant entries added, it results in unexpected behavior
such as same lflow being processed multiple times as a waste of processing.
More severely, after commit 580aea72e it can result in orphaned pointer leading
to crash, as reported in [0].
This patch fixes the problems by checking existance of same reference before
adding in lflow_resource_add(). To do this check efficiently, hmap is used to
replace the list struct for the resource-to-lflow index.
This patch also handles freeing the hmap entries in
lflow_resource_ref.ref_lflow_table, which was not addressed before. Without
this it could result in memory growth continuously until a recompute is
triggered.
[0] https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374991.html
Reported-by: Dumitru Ceara <dceara at redhat.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374991.html
Fixes: ade4e779d3fb ("ovn-controller: Use the tracked runtime data changes for flow calculation.")
Fixes: 580aea72e26f ("ovn-controller: Fix conjunction handling with incremental processing.")
Signed-off-by: Han Zhou <hzhou at ovn.org>
---
controller/lflow.c | 62 +++++++++++++++++++++++++++++++++++++-----------------
controller/lflow.h | 27 +++++++++++++-----------
tests/ovn.at | 49 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 107 insertions(+), 31 deletions(-)
diff --git a/controller/lflow.c b/controller/lflow.c
index e785866..2d2bbee 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -156,20 +156,27 @@ lflow_resource_init(struct lflow_resource_ref *lfrr)
hmap_init(&lfrr->lflow_ref_table);
}
+static void
+ref_lflow_node_destroy(struct ref_lflow_node *rlfn)
+{
+ free(rlfn->ref_name);
+ hmap_destroy(&rlfn->lflow_uuids);
+ free(rlfn);
+}
+
void
lflow_resource_destroy(struct lflow_resource_ref *lfrr)
{
struct ref_lflow_node *rlfn, *rlfn_next;
HMAP_FOR_EACH_SAFE (rlfn, rlfn_next, node, &lfrr->ref_lflow_table) {
- free(rlfn->ref_name);
struct lflow_ref_list_node *lrln, *next;
- LIST_FOR_EACH_SAFE (lrln, next, ref_list, &rlfn->ref_lflow_head) {
- ovs_list_remove(&lrln->ref_list);
- ovs_list_remove(&lrln->lflow_list);
+ HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids) {
+ ovs_list_remove(&lrln->list_node);
+ hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node);
free(lrln);
}
hmap_remove(&lfrr->ref_lflow_table, &rlfn->node);
- free(rlfn);
+ ref_lflow_node_destroy(rlfn);
}
hmap_destroy(&lfrr->ref_lflow_table);
@@ -222,6 +229,7 @@ static void
lflow_resource_add(struct lflow_resource_ref *lfrr, enum ref_type type,
const char *ref_name, const struct uuid *lflow_uuid)
{
+ bool is_new = false;
struct ref_lflow_node *rlfn = ref_lflow_lookup(&lfrr->ref_lflow_table,
type, ref_name);
if (!rlfn) {
@@ -229,8 +237,9 @@ lflow_resource_add(struct lflow_resource_ref *lfrr, enum ref_type type,
rlfn->node.hash = hash_string(ref_name, type);
rlfn->type = type;
rlfn->ref_name = xstrdup(ref_name);
- ovs_list_init(&rlfn->ref_lflow_head);
+ hmap_init(&rlfn->lflow_uuids);
hmap_insert(&lfrr->ref_lflow_table, &rlfn->node, rlfn->node.hash);
+ is_new = true;
}
struct lflow_ref_node *lfrn = lflow_ref_lookup(&lfrr->lflow_ref_table,
@@ -241,12 +250,24 @@ lflow_resource_add(struct lflow_resource_ref *lfrr, enum ref_type type,
lfrn->lflow_uuid = *lflow_uuid;
ovs_list_init(&lfrn->lflow_ref_head);
hmap_insert(&lfrr->lflow_ref_table, &lfrn->node, lfrn->node.hash);
+ is_new = true;
}
+ if (!is_new) {
+ struct lflow_ref_list_node *n;
+ HMAP_FOR_EACH_WITH_HASH(n, hmap_node, uuid_hash(lflow_uuid),
+ &rlfn->lflow_uuids) {
+ if (uuid_equals(&n->lflow_uuid, lflow_uuid)) {
+ /* The mapping already existed */
+ return;
+ }
+ }
+ }
struct lflow_ref_list_node *lrln = xzalloc(sizeof *lrln);
lrln->lflow_uuid = *lflow_uuid;
- ovs_list_push_back(&rlfn->ref_lflow_head, &lrln->ref_list);
- ovs_list_push_back(&lfrn->lflow_ref_head, &lrln->lflow_list);
+ lrln->rlfn = rlfn;
+ hmap_insert(&rlfn->lflow_uuids, &lrln->hmap_node, uuid_hash(lflow_uuid));
+ ovs_list_push_back(&lfrn->lflow_ref_head, &lrln->list_node);
}
static void
@@ -261,9 +282,13 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr,
hmap_remove(&lfrr->lflow_ref_table, &lfrn->node);
struct lflow_ref_list_node *lrln, *next;
- LIST_FOR_EACH_SAFE (lrln, next, lflow_list, &lfrn->lflow_ref_head) {
- ovs_list_remove(&lrln->ref_list);
- ovs_list_remove(&lrln->lflow_list);
+ LIST_FOR_EACH_SAFE (lrln, next, list_node, &lfrn->lflow_ref_head) {
+ ovs_list_remove(&lrln->list_node);
+ hmap_remove(&lrln->rlfn->lflow_uuids, &lrln->hmap_node);
+ if (hmap_is_empty(&lrln->rlfn->lflow_uuids)) {
+ hmap_remove(&lfrr->ref_lflow_table, &lrln->rlfn->node);
+ ref_lflow_node_destroy(lrln->rlfn);
+ }
free(lrln);
}
free(lfrn);
@@ -531,12 +556,12 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
hmap_remove(&l_ctx_out->lfrr->ref_lflow_table, &rlfn->node);
struct lflow_ref_list_node *lrln, *next;
- /* Detach the rlfn->ref_lflow_head nodes from the lfrr table and clean
+ /* Detach the rlfn->lflow_uuids nodes from the lfrr table and clean
* up all other nodes related to the lflows that uses the resource,
* so that the old nodes won't interfere with updating the lfrr table
* when reparsing the lflows. */
- LIST_FOR_EACH (lrln, ref_list, &rlfn->ref_lflow_head) {
- ovs_list_remove(&lrln->lflow_list);
+ HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
+ ovs_list_remove(&lrln->list_node);
}
struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
@@ -565,7 +590,7 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
/* Firstly, flood remove the flows from desired flow table. */
struct hmap flood_remove_nodes = HMAP_INITIALIZER(&flood_remove_nodes);
struct ofctrl_flood_remove_node *ofrn, *ofrn_next;
- LIST_FOR_EACH (lrln, ref_list, &rlfn->ref_lflow_head) {
+ HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) {
VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d,"
" name: %s.",
UUID_ARGS(&lrln->lflow_uuid),
@@ -604,12 +629,11 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
}
hmap_destroy(&flood_remove_nodes);
- LIST_FOR_EACH_SAFE (lrln, next, ref_list, &rlfn->ref_lflow_head) {
- ovs_list_remove(&lrln->ref_list);
+ HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids) {
+ hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node);
free(lrln);
}
- free(rlfn->ref_name);
- free(rlfn);
+ ref_lflow_node_destroy(rlfn);
dhcp_opts_destroy(&dhcp_opts);
dhcp_opts_destroy(&dhcpv6_opts);
diff --git a/controller/lflow.h b/controller/lflow.h
index c66b318..1251fb0 100644
--- a/controller/lflow.h
+++ b/controller/lflow.h
@@ -78,25 +78,28 @@ enum ref_type {
REF_TYPE_PORTBINDING
};
-/* Maintains the relationship for a pair of named resource and
- * a lflow, indexed by both ref_lflow_table and lflow_ref_table. */
-struct lflow_ref_list_node {
- struct ovs_list lflow_list; /* list for same lflow */
- struct ovs_list ref_list; /* list for same ref */
- struct uuid lflow_uuid;
-};
-
struct ref_lflow_node {
- struct hmap_node node;
+ struct hmap_node node; /* node in lflow_resource_ref.ref_lflow_table. */
enum ref_type type; /* key */
char *ref_name; /* key */
- struct ovs_list ref_lflow_head;
+ struct hmap lflow_uuids; /* Contains lflow_ref_list_node. Use hmap instead
+ of list so that lflow_resource_add() can check
+ and avoid adding redundant entires in O(1). */
};
struct lflow_ref_node {
- struct hmap_node node;
+ struct hmap_node node; /* node in lflow_resource_ref.lflow_ref_table. */
struct uuid lflow_uuid; /* key */
- struct ovs_list lflow_ref_head;
+ struct ovs_list lflow_ref_head; /* Contains lflow_ref_list_node. */
+};
+
+/* Maintains the relationship for a pair of named resource and
+ * a lflow, indexed by both ref_lflow_table and lflow_ref_table. */
+struct lflow_ref_list_node {
+ struct ovs_list list_node; /* node in lflow_ref_node.lflow_ref_head. */
+ struct hmap_node hmap_node; /* node in ref_lflow_node.lflow_uuids. */
+ struct uuid lflow_uuid;
+ struct ref_lflow_node *rlfn;
};
struct lflow_resource_ref {
diff --git a/tests/ovn.at b/tests/ovn.at
index 41fe577..d368fb9 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -22117,3 +22117,52 @@ OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected])
OVN_CLEANUP([hv1])
AT_CLEANUP
+
+AT_SETUP([ovn -- port bind/unbind change handling with conj flows - IPv6])
+ovn_start
+
+ovn-nbctl ls-add ls1
+
+ovn-nbctl lsp-add ls1 lsp1 \
+ -- lsp-set-addresses lsp1 "f0:00:00:00:00:01 2001::1" \
+ -- lsp-set-port-security lsp1 "f0:00:00:00:00:01 2001::1"
+
+net_add n1
+sim_add hv1
+
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+ovs-vsctl -- add-port br-int hv1-vif1 -- \
+ set interface hv1-vif1 external-ids:iface-id=lsp1 \
+ options:tx_pcap=hv1/vif1-tx.pcap \
+ options:rxq_pcap=hv1/vif1-rx.pcap \
+ ofport-request=1
+
+ovn-nbctl --wait=hv sync
+
+# Expected conjunction flows:
+# ... nd_tll=00:00:00:00:00:00 actions=conjunction(2,2/2)
+# ... nd_tll=f0:00:00:00:00:01 actions=conjunction(2,2/2)
+# ... nd_target=fe80::f200:ff:fe00:1 actions=conjunction(2,1/2)
+# ... nd_target=2001::1 actions=conjunction(2,1/2)
+OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction | wc -l`])
+
+# unbind the port
+ovs-vsctl set interface hv1-vif1 external_ids:iface-id=foo
+OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction | wc -l`])
+
+# bind the port again
+ovs-vsctl set interface hv1-vif1 external_ids:iface-id=lsp1
+OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction | wc -l`])
+
+# unbind the port again
+ovs-vsctl set interface hv1-vif1 external_ids:iface-id=foo
+OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \
+grep conjunction | wc -l`])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
--
2.1.0
More information about the dev
mailing list