[ovs-dev] [PATCH v3 5/8] conntrack: Use a cmap to store zone limits

Gaetan Rivet grive at u256.net
Tue Jun 15 23:22:50 UTC 2021


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 at u256.net>
Reviewed-by: Eli Britstein <elibr at nvidia.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 --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index 537e56534..7eb3ca297 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -172,7 +172,7 @@ struct conntrack {
     struct ovs_mutex ct_lock; /* Protects 2 following fields. */
     struct cmap conns OVS_GUARDED;
     struct mpsc_queue exp_lists[N_CT_TM];
-    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 45de13ebf..094367733 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -79,7 +79,7 @@ enum ct_alg_ctl_type {
 };
 
 struct zone_limit {
-    struct hmap_node node;
+    struct cmap_node node;
     struct conntrack_zone_limit czl;
 };
 
@@ -308,7 +308,7 @@ conntrack_init(void)
     for (unsigned i = 0; i < ARRAY_SIZE(ct->exp_lists); i++) {
         mpsc_queue_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);
@@ -343,12 +343,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;
         }
@@ -358,7 +371,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);
@@ -367,13 +379,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;
 }
 
@@ -381,13 +396,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;
@@ -398,13 +419,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 {
@@ -412,7 +434,6 @@ zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
                       zone);
         }
     }
-    ovs_mutex_unlock(&ct->ct_lock);
     return err;
 }
 
@@ -420,23 +441,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;
 }
 
@@ -453,7 +476,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);
     }
 }
 
@@ -539,10 +562,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) {
@@ -1024,7 +1050,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;
         }
 
@@ -1097,7 +1123,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 8fa7eb6d4..e93720e20 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -8215,7 +8215,8 @@ dpif_netdev_ct_get_limits(struct dpif *dpif OVS_UNUSED,
             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;
             }
@@ -8225,7 +8226,7 @@ dpif_netdev_ct_get_limits(struct dpif *dpif OVS_UNUSED,
             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));
             }
         }
     }
-- 
2.31.1



More information about the dev mailing list