[ovs-dev] [PATCH ovn] lflow.c: Avoid adding redundant resource refs for port-bindings.
Dumitru Ceara
dceara at redhat.com
Tue Sep 15 15:19:34 UTC 2020
On 9/15/20 10:15 AM, Han Zhou wrote:
> 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>
Hi Han,
Thanks for the fix. This is not a complete review yet but I do have some
preliminary comments.
> ---
> 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);
> +}
> +
In my opinion it would be cleaner if we also have a function to do the
init/lookup part. Actually, I think this comment is valid for all
structures: ref_lflow_node, lflow_ref_node, lflow_resource_ref. It might
be a bit of additional code but now it's quite hard to follow what gets
created/destroyed where.
> 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;
I would avoid this variable because in my opinion it makes the code more
complicated to follow.
> 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);
> }
I think rewriting this function in a different way might make it more
readable. E.g., pseudocode:
rlfn = ref_lflow_lookup(&lfrr->ref_lflow_table, type, ref_name);
lfrn = lflow_ref_lookup(&lfrr->lflow_ref_table, lflow_uuid);
if (rlfn && lfrn) {
/* Check for duplicates here. */
} else {
if (!rlfn) {
/* Allocate, init, insert rlfn. */
/* Maybe we can have a function for this? */
}
if (!lfrn) {
/* Allocate, init, insert lfrn. */
/* Maybe we can have a function for this? */
}
}
/* Allocate, init, insert lrln. */
[...]
Also, it might be me but these variable names are so close to each other
that it's quite confusing. Maybe we can find some more explicit names?
>
> 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);
I think a comment here explaining why we can't remove the node would be
helpful.
Thanks,
Dumitru
> + 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
>
More information about the dev
mailing list