Previously we guarded expectations using an rwlock. While there are generally only a few or none expectation entries, taking and releasing the rwlock in read mode still takes more time than using a cmap.
When doing a loadtest with OVN and natting across two LRs we increased our packet rate from 2.5 Mio pps to 12 Mio pps with only 40% of the previous load. This mainly comes from the UNSNAT and UNDNAT stages of OVN that generally do not match anything as long as we are in not specific cases. When using the same testcases as in the previous commits Below lists the total time for the testrun with these various configs. | | Without this patch | With this patch | | Config 1 | 70.8 s | 70.5 s | | Config 2 | 73.1 s | 80.7 s | | Config 3 | 68.1 s | 65.9 s | | Config 4 | 78.5 s | 71.4 s | | Config 5 | 45.5 s | 43.3 s | | Config 6 | 47.3 s | 46.6 s | Co-Authored-by: Florian Werner <[email protected]> Signed-off-by: Florian Werner <[email protected]> Co-Authored-by: Sebastian Riese <[email protected]> Signed-off-by: Sebastian Riese <[email protected]> Signed-off-by: Felix Huettner <[email protected]> --- lib/conntrack-private.h | 9 ++-- lib/conntrack.c | 94 ++++++++++++++++++++--------------------- lib/conntrack.h | 5 +-- 3 files changed, 51 insertions(+), 57 deletions(-) diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h index f1132e8aa..0141afd4a 100644 --- a/lib/conntrack-private.h +++ b/lib/conntrack-private.h @@ -75,7 +75,7 @@ BUILD_ASSERT_DECL(MEMBER_SIZEOF(struct conn_key, nw_proto) == sizeof(uint8_t)); * connection. */ struct alg_exp_node { /* Node in alg_expectations. */ - struct hmap_node node; + struct cmap_node node; /* Node in alg_expectation_refs. */ struct hindex_node node_ref; /* Key of data connection to be created. */ @@ -231,9 +231,10 @@ struct conntrack { /* Expectations for application level gateways (created by control * connections to help create data connections, e.g. for FTP). */ - struct ovs_rwlock resources_lock; /* Protects fields below. */ - struct hmap alg_expectations OVS_GUARDED; /* Holds struct - * alg_exp_nodes. */ + struct ovs_mutex resources_lock; /* Protects fields below. */ + struct cmap alg_expectations; /* Holds struct alg_exp_nodes. + * resources_lock must be held for mutating + * operations. */ struct hindex alg_expectation_refs OVS_GUARDED; /* For lookup from * control context. */ diff --git a/lib/conntrack.c b/lib/conntrack.c index 4661e803f..fd1be21df 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -131,7 +131,7 @@ static inline bool extract_l3_ipv6(struct conn_key *key, const void *data, size_t size, const char **new_data); static struct alg_exp_node * -expectation_lookup(struct hmap *alg_expectations, const struct conn_key *key, +expectation_lookup(struct cmap *alg_expectations, const struct conn_key *key, uint32_t basis, bool src_ip_wc); static int @@ -154,6 +154,8 @@ detect_ftp_ctl_type(const struct conn_lookup_ctx *ctx, static void expectation_clean(struct conntrack *ct, const struct conn_key *parent_key); +static void +expectation_flush(struct conntrack *ct) OVS_REQUIRES(ct->resources_lock); static struct ct_l4_proto *l4_protos[UINT8_MAX + 1]; @@ -254,11 +256,11 @@ conntrack_init(void) */ ct->hash_basis = random_uint32(); - ovs_rwlock_init(&ct->resources_lock); - ovs_rwlock_wrlock(&ct->resources_lock); - hmap_init(&ct->alg_expectations); + ovs_mutex_init(&ct->resources_lock); + ovs_mutex_lock(&ct->resources_lock); + cmap_init(&ct->alg_expectations); hindex_init(&ct->alg_expectation_refs); - ovs_rwlock_unlock(&ct->resources_lock); + ovs_mutex_unlock(&ct->resources_lock); ovs_mutex_init_adaptive(&ct->ct_lock); ovs_mutex_lock(&ct->ct_lock); @@ -641,8 +643,9 @@ conntrack_destroy(struct conntrack *ct) ovsrcu_postpone(free, tp); } - ovs_mutex_lock(&ct->ct_lock); + conntrack_flush(ct, NULL); + ovs_mutex_lock(&ct->ct_lock); for (unsigned i = 0; i < ARRAY_SIZE(ct->conns); i++) { cmap_destroy(&ct->conns[i]); } @@ -652,15 +655,12 @@ conntrack_destroy(struct conntrack *ct) ovs_mutex_unlock(&ct->ct_lock); ovs_mutex_destroy(&ct->ct_lock); - ovs_rwlock_wrlock(&ct->resources_lock); - struct alg_exp_node *alg_exp_node; - HMAP_FOR_EACH_POP (alg_exp_node, node, &ct->alg_expectations) { - free(alg_exp_node); - } - hmap_destroy(&ct->alg_expectations); + ovs_mutex_lock(&ct->resources_lock); + expectation_flush(ct); + cmap_destroy(&ct->alg_expectations); hindex_destroy(&ct->alg_expectation_refs); - ovs_rwlock_unlock(&ct->resources_lock); - ovs_rwlock_destroy(&ct->resources_lock); + ovs_mutex_unlock(&ct->resources_lock); + ovs_mutex_destroy(&ct->resources_lock); ipf_destroy(ct->ipf); free(ct); @@ -1473,7 +1473,6 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, if (OVS_UNLIKELY(create_new_conn)) { - ovs_rwlock_rdlock(&ct->resources_lock); alg_exp = expectation_lookup(&ct->alg_expectations, &ctx->key, ct->hash_basis, alg_src_ip_wc(ct_alg_ctl)); @@ -1481,7 +1480,6 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, memcpy(&alg_exp_entry, alg_exp, sizeof alg_exp_entry); alg_exp = &alg_exp_entry; } - ovs_rwlock_unlock(&ct->resources_lock); conn = conn_maybe_not_found(ct, pkt, ctx, commit, now, nat_action_info, helper, alg_exp, ct_alg_ctl, tp_id); @@ -2979,6 +2977,7 @@ conntrack_exp_dump_start(struct conntrack *ct, struct conntrack_dump *dump, } dump->ct = ct; + dump->cursor = cmap_cursor_start(&dump->ct->alg_expectations); return 0; } @@ -2986,31 +2985,16 @@ conntrack_exp_dump_start(struct conntrack *ct, struct conntrack_dump *dump, int conntrack_exp_dump_next(struct conntrack_dump *dump, struct ct_dpif_exp *entry) { - struct conntrack *ct = dump->ct; struct alg_exp_node *enode; - int ret = EOF; - - ovs_rwlock_rdlock(&ct->resources_lock); - - for (;;) { - struct hmap_node *node = hmap_at_position(&ct->alg_expectations, - &dump->hmap_pos); - if (!node) { - break; - } - - enode = CONTAINER_OF(node, struct alg_exp_node, node); + CMAP_CURSOR_FOR_EACH_CONTINUE (enode, node, &dump->cursor) { if (!dump->filter_zone || enode->key.zone == dump->zone) { - ret = 0; exp_node_to_ct_dpif_exp(enode, entry); - break; + return 0; } } - ovs_rwlock_unlock(&ct->resources_lock); - - return ret; + return EOF; } int @@ -3108,9 +3092,8 @@ conntrack_get_tcp_seq_chk(struct conntrack *ct) return enabled; } -/* This function must be called with the ct->resources read lock taken. */ static struct alg_exp_node * -expectation_lookup(struct hmap *alg_expectations, const struct conn_key *key, +expectation_lookup(struct cmap *alg_expectations, const struct conn_key *key, uint32_t basis, bool src_ip_wc) { struct conn_key check_key; @@ -3123,7 +3106,7 @@ expectation_lookup(struct hmap *alg_expectations, const struct conn_key *key, struct alg_exp_node *alg_exp_node; - HMAP_FOR_EACH_WITH_HASH (alg_exp_node, node, + CMAP_FOR_EACH_WITH_HASH (alg_exp_node, node, conn_key_hash(&check_key, basis), alg_expectations) { if (!conn_key_cmp(&alg_exp_node->key, &check_key)) { @@ -3133,17 +3116,18 @@ expectation_lookup(struct hmap *alg_expectations, const struct conn_key *key, return NULL; } -/* This function must be called with the ct->resources write lock taken. */ +/* This function must be called with the ct->resources_lock taken. */ static void -expectation_remove(struct hmap *alg_expectations, +expectation_remove(struct cmap *alg_expectations, const struct conn_key *key, uint32_t basis) { struct alg_exp_node *alg_exp_node; - HMAP_FOR_EACH_WITH_HASH (alg_exp_node, node, conn_key_hash(key, basis), + uint32_t hash = conn_key_hash(key, basis); + CMAP_FOR_EACH_WITH_HASH (alg_exp_node, node, hash, alg_expectations) { if (!conn_key_cmp(&alg_exp_node->key, key)) { - hmap_remove(alg_expectations, &alg_exp_node->node); + cmap_remove(alg_expectations, &alg_exp_node->node, hash); break; } } @@ -3183,10 +3167,22 @@ expectation_ref_create(struct hindex *alg_expectation_refs, } } +static void +expectation_flush(struct conntrack *ct) +{ + struct alg_exp_node *node; + HINDEX_FOR_EACH_SAFE (node, node_ref, &ct->alg_expectation_refs) { + expectation_remove(&ct->alg_expectations, &node->key, + ct->hash_basis); + hindex_remove(&ct->alg_expectation_refs, &node->node_ref); + ovsrcu_postpone(free, node); + } +} + static void expectation_clean(struct conntrack *ct, const struct conn_key *parent_key) { - ovs_rwlock_wrlock(&ct->resources_lock); + ovs_mutex_lock(&ct->resources_lock); struct alg_exp_node *node; HINDEX_FOR_EACH_WITH_HASH_SAFE (node, node_ref, @@ -3196,11 +3192,11 @@ expectation_clean(struct conntrack *ct, const struct conn_key *parent_key) expectation_remove(&ct->alg_expectations, &node->key, ct->hash_basis); hindex_remove(&ct->alg_expectation_refs, &node->node_ref); - free(node); + ovsrcu_postpone(free, node); } } - ovs_rwlock_unlock(&ct->resources_lock); + ovs_mutex_unlock(&ct->resources_lock); } static void @@ -3260,21 +3256,21 @@ expectation_create(struct conntrack *ct, ovs_be16 dst_port, /* Take the write lock here because it is almost 100% * likely that the lookup will fail and * expectation_create() will be called below. */ - ovs_rwlock_wrlock(&ct->resources_lock); + ovs_mutex_lock(&ct->resources_lock); struct alg_exp_node *alg_exp = expectation_lookup( &ct->alg_expectations, &alg_exp_node->key, ct->hash_basis, src_ip_wc); if (alg_exp) { - free(alg_exp_node); - ovs_rwlock_unlock(&ct->resources_lock); + ovsrcu_postpone(free, alg_exp_node); + ovs_mutex_unlock(&ct->resources_lock); return; } alg_exp_node->alg_nat_repl_addr = alg_nat_repl_addr; - hmap_insert(&ct->alg_expectations, &alg_exp_node->node, + cmap_insert(&ct->alg_expectations, &alg_exp_node->node, conn_key_hash(&alg_exp_node->key, ct->hash_basis)); expectation_ref_create(&ct->alg_expectation_refs, alg_exp_node, ct->hash_basis); - ovs_rwlock_unlock(&ct->resources_lock); + ovs_mutex_unlock(&ct->resources_lock); } static void diff --git a/lib/conntrack.h b/lib/conntrack.h index c3136e955..abc859ef6 100644 --- a/lib/conntrack.h +++ b/lib/conntrack.h @@ -106,10 +106,7 @@ void conntrack_clear(struct dp_packet *packet); struct conntrack_dump { struct conntrack *ct; unsigned bucket; - union { - struct hmap_position hmap_pos; - struct cmap_cursor cursor; - }; + struct cmap_cursor cursor; bool filter_zone; uint16_t zone; uint16_t current_zone; -- 2.43.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
