On Mon, Jan 08, 2018 at 12:54:26PM -0800, Darrell Ball wrote:
> Presently, alg expectations are removed by being time expired.
> This was intended to happen before the control connections and
> was intended to minimize the extra work involved for tracking and
> removing the expectations. This is not the best option since it
> should be possible to remove expectations when a control connection
> is removed and a new api is in the works to do this. Also, conceptually
> an expectation should not exist without a control connection context
> and it can be argued that this should be a strict requirement.
>
> The approach is changed to remove the expectations when the control
> connections are removed. The previous code to expire the expectations
> is removed at the same time.
>
> Fixes: bd5e81a0e ("Userspace Datapath: Add ALG infra and FTP.")
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-December/341683.html
>
> Signed-off-by: Darrell Ball <dlu...@gmail.com>
> ---
>
> v2->v3: Use hindex map in lieu of hmap for efficiency: Ben P.
I think that the open-coded iteration in expectation_clean() can be more
simply rewritten, like this:
diff --git a/lib/conntrack.c b/lib/conntrack.c
index a14b66c16f56..c5ef42c585cf 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -2622,50 +2622,19 @@ expectation_ref_create(struct hindex
*alg_expectation_refs,
}
}
-/* This function must be called with the ct->resources read lock taken. */
-static struct alg_exp_node *
-expectation_ref_lookup(const struct hindex *alg_expectation_refs,
- const struct conn_key *master_key,
- uint32_t basis)
-{
- uint32_t alg_exp_conn_key_hash = conn_key_hash(master_key, basis);
- struct hindex_node *hindex_node =
- hindex_node_with_hash(alg_expectation_refs, alg_exp_conn_key_hash);
- if (hindex_node) {
- struct alg_exp_node *alg_exp_node =
- CONTAINER_OF(hindex_node, struct alg_exp_node, node_ref);
- return alg_exp_node;
- }
-
- return NULL;
-}
-
static void
expectation_clean(struct conntrack *ct, const struct conn_key *master_key,
uint32_t basis)
{
ct_rwlock_wrlock(&ct->resources_lock);
- struct alg_exp_node *alg_exp_node =
- expectation_ref_lookup(&ct->alg_expectation_refs, master_key, basis);
- if (alg_exp_node) {
- expectation_remove(&ct->alg_expectations, &alg_exp_node->key, basis);
- hindex_remove(&ct->alg_expectation_refs, &alg_exp_node->node_ref);
- struct hindex_node *next_hindex_node =
- hindex_next_node_with_hash(&alg_exp_node->node_ref);
- free(alg_exp_node);
- struct hindex_node *hindex_node = next_hindex_node;
-
- while (hindex_node) {
- next_hindex_node = hindex_next_node_with_hash(hindex_node);
- struct alg_exp_node *alg_exp_node =
- CONTAINER_OF(hindex_node, struct alg_exp_node, node_ref);
- expectation_remove(&ct->alg_expectations, &alg_exp_node->key,
- basis);
- hindex_remove(&ct->alg_expectation_refs, hindex_node);
- free(alg_exp_node);
- hindex_node = next_hindex_node;
- }
+ struct alg_exp_node *node, *next;
+ HINDEX_FOR_EACH_WITH_HASH_SAFE (node, next, node_ref,
+ conn_key_hash(master_key, basis),
+ &ct->alg_expectation_refs) {
+ expectation_remove(&ct->alg_expectations, &node->key, basis);
+ hindex_remove(&ct->alg_expectation_refs, &node->node_ref);
+ free(node);
}
ct_rwlock_unlock(&ct->resources_lock);
}
With this rewrite, patch 1 adding hindex_next_node_with_hash isn't
needed.
However, this loop (in either version) doesn't do any kind of comparison
of a full key; it only matches on the hash value. That seems wrong. Is
the lack of a key comparison intentional and correct?
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
