diff mbox series

[ovs-dev,v3,1/6] conntrack: Use a cmap to store zone limits

Message ID 164781561323.306037.16210903464835842445.stgit@fed.void
State Superseded
Headers show
Series conntrack: Introduce buckets and reduce contention. | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Paolo Valerio March 20, 2022, 10:33 p.m. UTC 
From: Gaetan Rivet <grive@u256.net>

Change the data structure from hmap to cmap for zone limits.
As they are shared amongst multiple conntrack users, multiple
readers want to check the current zone limit state before progressing in
their processing. Using a CMAP allows doing lookups without taking the
global 'ct_lock', thus reducing contention.

Signed-off-by: Gaetan Rivet <grive@u256.net>
Reviewed-by: Eli Britstein <elibr@nvidia.com>
Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
---
 lib/conntrack-private.h |    2 +
 lib/conntrack.c         |   70 ++++++++++++++++++++++++++++++++---------------
 lib/conntrack.h         |    2 +
 lib/dpif-netdev.c       |    5 ++-
 4 files changed, 53 insertions(+), 26 deletions(-)
diff mbox series

Patch

diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index dfdf4e676..d9461b811 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -192,7 +192,7 @@  struct conntrack {
     struct ovs_mutex ct_lock; /* Protects 2 following fields. */
     struct cmap conns OVS_GUARDED;
     struct ovs_list exp_lists[N_CT_TM] OVS_GUARDED;
-    struct hmap zone_limits OVS_GUARDED;
+    struct cmap zone_limits OVS_GUARDED;
     struct hmap timeout_policies OVS_GUARDED;
     uint32_t hash_basis; /* Salt for hashing a connection key. */
     pthread_t clean_thread; /* Periodically cleans up connection tracker. */
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 40690e5f0..bb605bac9 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -81,7 +81,7 @@  enum ct_alg_ctl_type {
 };
 
 struct zone_limit {
-    struct hmap_node node;
+    struct cmap_node node;
     struct conntrack_zone_limit czl;
 };
 
@@ -311,7 +311,7 @@  conntrack_init(void)
     for (unsigned i = 0; i < ARRAY_SIZE(ct->exp_lists); i++) {
         ovs_list_init(&ct->exp_lists[i]);
     }
-    hmap_init(&ct->zone_limits);
+    cmap_init(&ct->zone_limits);
     ct->zone_limit_seq = 0;
     timeout_policy_init(ct);
     ovs_mutex_unlock(&ct->ct_lock);
@@ -346,12 +346,25 @@  zone_key_hash(int32_t zone, uint32_t basis)
 }
 
 static struct zone_limit *
-zone_limit_lookup(struct conntrack *ct, int32_t zone)
+zone_limit_lookup_protected(struct conntrack *ct, int32_t zone)
     OVS_REQUIRES(ct->ct_lock)
 {
     uint32_t hash = zone_key_hash(zone, ct->hash_basis);
     struct zone_limit *zl;
-    HMAP_FOR_EACH_IN_BUCKET (zl, node, hash, &ct->zone_limits) {
+    CMAP_FOR_EACH_WITH_HASH_PROTECTED (zl, node, hash, &ct->zone_limits) {
+        if (zl->czl.zone == zone) {
+            return zl;
+        }
+    }
+    return NULL;
+}
+
+static struct zone_limit *
+zone_limit_lookup(struct conntrack *ct, int32_t zone)
+{
+    uint32_t hash = zone_key_hash(zone, ct->hash_basis);
+    struct zone_limit *zl;
+    CMAP_FOR_EACH_WITH_HASH (zl, node, hash, &ct->zone_limits) {
         if (zl->czl.zone == zone) {
             return zl;
         }
@@ -361,7 +374,6 @@  zone_limit_lookup(struct conntrack *ct, int32_t zone)
 
 static struct zone_limit *
 zone_limit_lookup_or_default(struct conntrack *ct, int32_t zone)
-    OVS_REQUIRES(ct->ct_lock)
 {
     struct zone_limit *zl = zone_limit_lookup(ct, zone);
     return zl ? zl : zone_limit_lookup(ct, DEFAULT_ZONE);
@@ -370,13 +382,16 @@  zone_limit_lookup_or_default(struct conntrack *ct, int32_t zone)
 struct conntrack_zone_limit
 zone_limit_get(struct conntrack *ct, int32_t zone)
 {
-    ovs_mutex_lock(&ct->ct_lock);
-    struct conntrack_zone_limit czl = {DEFAULT_ZONE, 0, 0, 0};
+    struct conntrack_zone_limit czl = {
+        .zone = DEFAULT_ZONE,
+        .limit = 0,
+        .count = ATOMIC_COUNT_INIT(0),
+        .zone_limit_seq = 0,
+    };
     struct zone_limit *zl = zone_limit_lookup_or_default(ct, zone);
     if (zl) {
         czl = zl->czl;
     }
-    ovs_mutex_unlock(&ct->ct_lock);
     return czl;
 }
 
@@ -384,13 +399,19 @@  static int
 zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit)
     OVS_REQUIRES(ct->ct_lock)
 {
+    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
+
+    if (zl) {
+        return 0;
+    }
+
     if (zone >= DEFAULT_ZONE && zone <= MAX_ZONE) {
-        struct zone_limit *zl = xzalloc(sizeof *zl);
+        zl = xzalloc(sizeof *zl);
         zl->czl.limit = limit;
         zl->czl.zone = zone;
         zl->czl.zone_limit_seq = ct->zone_limit_seq++;
         uint32_t hash = zone_key_hash(zone, ct->hash_basis);
-        hmap_insert(&ct->zone_limits, &zl->node, hash);
+        cmap_insert(&ct->zone_limits, &zl->node, hash);
         return 0;
     } else {
         return EINVAL;
@@ -401,13 +422,14 @@  int
 zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
 {
     int err = 0;
-    ovs_mutex_lock(&ct->ct_lock);
     struct zone_limit *zl = zone_limit_lookup(ct, zone);
     if (zl) {
         zl->czl.limit = limit;
         VLOG_INFO("Changed zone limit of %u for zone %d", limit, zone);
     } else {
+        ovs_mutex_lock(&ct->ct_lock);
         err = zone_limit_create(ct, zone, limit);
+        ovs_mutex_unlock(&ct->ct_lock);
         if (!err) {
             VLOG_INFO("Created zone limit of %u for zone %d", limit, zone);
         } else {
@@ -415,7 +437,6 @@  zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
                       zone);
         }
     }
-    ovs_mutex_unlock(&ct->ct_lock);
     return err;
 }
 
@@ -423,23 +444,25 @@  static void
 zone_limit_clean(struct conntrack *ct, struct zone_limit *zl)
     OVS_REQUIRES(ct->ct_lock)
 {
-    hmap_remove(&ct->zone_limits, &zl->node);
-    free(zl);
+    uint32_t hash = zone_key_hash(zl->czl.zone, ct->hash_basis);
+    cmap_remove(&ct->zone_limits, &zl->node, hash);
+    ovsrcu_postpone(free, zl);
 }
 
 int
 zone_limit_delete(struct conntrack *ct, uint16_t zone)
 {
     ovs_mutex_lock(&ct->ct_lock);
-    struct zone_limit *zl = zone_limit_lookup(ct, zone);
+    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
     if (zl) {
         zone_limit_clean(ct, zl);
+        ovs_mutex_unlock(&ct->ct_lock);
         VLOG_INFO("Deleted zone limit for zone %d", zone);
     } else {
+        ovs_mutex_unlock(&ct->ct_lock);
         VLOG_INFO("Attempted delete of non-existent zone limit: zone %d",
                   zone);
     }
-    ovs_mutex_unlock(&ct->ct_lock);
     return 0;
 }
 
@@ -456,7 +479,7 @@  conn_clean_cmn(struct conntrack *ct, struct conn *conn)
 
     struct zone_limit *zl = zone_limit_lookup(ct, conn->admit_zone);
     if (zl && zl->czl.zone_limit_seq == conn->zone_limit_seq) {
-        zl->czl.count--;
+        atomic_count_dec(&zl->czl.count);
     }
 }
 
@@ -510,10 +533,13 @@  conntrack_destroy(struct conntrack *ct)
     cmap_destroy(&ct->conns);
 
     struct zone_limit *zl;
-    HMAP_FOR_EACH_POP (zl, node, &ct->zone_limits) {
-        free(zl);
+    CMAP_FOR_EACH (zl, node, &ct->zone_limits) {
+        uint32_t hash = zone_key_hash(zl->czl.zone, ct->hash_basis);
+
+        cmap_remove(&ct->zone_limits, &zl->node, hash);
+        ovsrcu_postpone(free, zl);
     }
-    hmap_destroy(&ct->zone_limits);
+    cmap_destroy(&ct->zone_limits);
 
     struct timeout_policy *tp;
     HMAP_FOR_EACH_POP (tp, node, &ct->timeout_policies) {
@@ -991,7 +1017,7 @@  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
     if (commit) {
         struct zone_limit *zl = zone_limit_lookup_or_default(ct,
                                                              ctx->key.zone);
-        if (zl && zl->czl.count >= zl->czl.limit) {
+        if (zl && atomic_count_get(&zl->czl.count) >= zl->czl.limit) {
             return nc;
         }
 
@@ -1064,7 +1090,7 @@  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
         if (zl) {
             nc->admit_zone = zl->czl.zone;
             nc->zone_limit_seq = zl->czl.zone_limit_seq;
-            zl->czl.count++;
+            atomic_count_inc(&zl->czl.count);
         } else {
             nc->admit_zone = INVALID_ZONE;
         }
diff --git a/lib/conntrack.h b/lib/conntrack.h
index 9553b188a..58b181834 100644
--- a/lib/conntrack.h
+++ b/lib/conntrack.h
@@ -108,7 +108,7 @@  struct conntrack_dump {
 struct conntrack_zone_limit {
     int32_t zone;
     uint32_t limit;
-    uint32_t count;
+    atomic_count count;
     uint32_t zone_limit_seq; /* Used to disambiguate zone limit counts. */
 };
 
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 9f35713ef..313ddb5ec 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -9244,7 +9244,8 @@  dpif_netdev_ct_get_limits(struct dpif *dpif,
             czl = zone_limit_get(dp->conntrack, zone_limit->zone);
             if (czl.zone == zone_limit->zone || czl.zone == DEFAULT_ZONE) {
                 ct_dpif_push_zone_limit(zone_limits_reply, zone_limit->zone,
-                                        czl.limit, czl.count);
+                                        czl.limit,
+                                        atomic_count_get(&czl.count));
             } else {
                 return EINVAL;
             }
@@ -9254,7 +9255,7 @@  dpif_netdev_ct_get_limits(struct dpif *dpif,
             czl = zone_limit_get(dp->conntrack, z);
             if (czl.zone == z) {
                 ct_dpif_push_zone_limit(zone_limits_reply, z, czl.limit,
-                                        czl.count);
+                                        atomic_count_get(&czl.count));
             }
         }
     }