diff mbox series

[ovs-dev,v5,03/13] conntrack: Use cmap for expectations.

Message ID 9d8927cd4ebdd51a1d9b382f9f6463d3e73561b1.1788272618.git.felix.huettner@digits.schwarz
State New
Delegated to: aaron conole
Headers show
Series Multiple conntrack performance improvements. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_FreeBSD_Build_and_Test success github build: passed

Commit Message

Felix Huettner Sept. 1, 2026, 2:24 p.m. UTC 
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 Mpps to 12 Mpps 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.

Co-Authored-by: Florian Werner <florian.werner@digits.schwarz>
Signed-off-by: Florian Werner <florian.werner@digits.schwarz>
Co-Authored-by: Sebastian Riese <sebastian.riese@cloudandheat.com>
Signed-off-by: Sebastian Riese <sebastian.riese@cloudandheat.com>
Signed-off-by: Felix Huettner <felix.huettner@digits.schwarz>
---

Notes:
    v4->v5: reword commit message

 lib/conntrack-private.h |   9 ++--
 lib/conntrack.c         | 112 +++++++++++++++++++---------------------
 lib/conntrack.h         |   5 +-
 3 files changed, 60 insertions(+), 66 deletions(-)
diff mbox series

Patch

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 f03945a70..af874545d 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);
@@ -642,7 +644,6 @@  conntrack_destroy(struct conntrack *ct)
     }
 
     ovs_mutex_lock(&ct->ct_lock);
-
     for (unsigned i = 0; i < ARRAY_SIZE(ct->conns); i++) {
         cmap_destroy(&ct->conns[i]);
     }
@@ -652,15 +653,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);
@@ -1474,7 +1472,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));
@@ -1482,7 +1479,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);
@@ -2980,6 +2976,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;
 }
@@ -2987,31 +2984,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
@@ -3109,9 +3091,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;
@@ -3124,7 +3105,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)) {
@@ -3134,34 +3115,36 @@  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. */
 static void
-expectation_remove(struct hmap *alg_expectations,
+expectation_remove(struct conntrack *ct,
                    const struct conn_key *key, uint32_t basis)
+    OVS_REQUIRES(ct->resources_lock)
 {
     struct alg_exp_node *alg_exp_node;
 
-    HMAP_FOR_EACH_WITH_HASH (alg_exp_node, node, conn_key_hash(key, basis),
-                             alg_expectations) {
+    uint32_t hash = conn_key_hash(key, basis);
+    CMAP_FOR_EACH_WITH_HASH (alg_exp_node, node, hash,
+                             &ct->alg_expectations) {
         if (!conn_key_cmp(&alg_exp_node->key, key)) {
-            hmap_remove(alg_expectations, &alg_exp_node->node);
+            cmap_remove(&ct->alg_expectations, &alg_exp_node->node, hash);
             break;
         }
     }
 }
 
-/* This function must be called with the ct->resources read lock taken. */
+/* This function must be called with the ct->resources_lock taken. */
 static struct alg_exp_node *
-expectation_ref_lookup_unique(const struct hindex *alg_expectation_refs,
+expectation_ref_lookup_unique(const struct conntrack *ct,
                               const struct conn_key *parent_key,
                               const struct conn_key *alg_exp_key,
                               uint32_t basis)
+    OVS_REQUIRES(ct->resources_lock)
 {
     struct alg_exp_node *alg_exp_node;
 
     HINDEX_FOR_EACH_WITH_HASH (alg_exp_node, node_ref,
                                conn_key_hash(parent_key, basis),
-                               alg_expectation_refs) {
+                               &ct->alg_expectation_refs) {
         if (!conn_key_cmp(&alg_exp_node->parent_key, parent_key) &&
             !conn_key_cmp(&alg_exp_node->key, alg_exp_key)) {
             return alg_exp_node;
@@ -3170,38 +3153,51 @@  expectation_ref_lookup_unique(const struct hindex *alg_expectation_refs,
     return NULL;
 }
 
-/* This function must be called with the ct->resources write lock taken. */
 static void
-expectation_ref_create(struct hindex *alg_expectation_refs,
+expectation_ref_create(struct conntrack *ct,
                        struct alg_exp_node *alg_exp_node,
                        uint32_t basis)
+    OVS_REQUIRES(ct->resources_lock)
 {
-    if (!expectation_ref_lookup_unique(alg_expectation_refs,
+    if (!expectation_ref_lookup_unique(ct,
                                        &alg_exp_node->parent_key,
                                        &alg_exp_node->key, basis)) {
-        hindex_insert(alg_expectation_refs, &alg_exp_node->node_ref,
+        hindex_insert(&ct->alg_expectation_refs, &alg_exp_node->node_ref,
                       conn_key_hash(&alg_exp_node->parent_key, basis));
     }
 }
 
+static void
+expectation_flush(struct conntrack *ct)
+    OVS_REQUIRES(ct->resources_lock)
+{
+    struct alg_exp_node *node;
+    HINDEX_FOR_EACH_SAFE (node, node_ref, &ct->alg_expectation_refs) {
+        expectation_remove(ct, &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,
                                     conn_key_hash(parent_key, ct->hash_basis),
                                     &ct->alg_expectation_refs) {
         if (!conn_key_cmp(&node->parent_key, parent_key)) {
-            expectation_remove(&ct->alg_expectations, &node->key,
+            expectation_remove(ct, &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
@@ -3261,21 +3257,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);
+        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,
+    expectation_ref_create(ct, 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;