diff mbox series

[ovs-dev,v2,5/6] conntrack: Use an atomic conn expiration value

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

Checks

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

Commit Message

Paolo Valerio March 7, 2022, 12:40 p.m. UTC 
From: Gaetan Rivet <grive@u256.net>

A lock is taken during conn_lookup() to check whether a connection is
expired before returning it. This lock can have some contention.

Even though this lock ensures a consistent sequence of writes, it does
not imply a specific order. A ct_clean thread taking the lock first
could read a value that would be updated immediately after by a PMD
waiting on the same lock, just as well as the inverse order.

As such, the expiration time can be stale anytime it is read. In this
context, using an atomic will ensure the same guarantees for either
writes or reads, i.e. writes are consistent and reads are not undefined
behaviour. Reading an atomic is however less costly than taking and
releasing a lock.

Signed-off-by: Gaetan Rivet <grive@u256.net>
Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
Reviewed-by: Eli Britstein <elibr@nvidia.com>
Acked-by: William Tu <u9012063@gmail.com>
---
 lib/conntrack-private.h |    2 +-
 lib/conntrack-tp.c      |    2 +-
 lib/conntrack.c         |   27 +++++++++++++++------------
 3 files changed, 17 insertions(+), 14 deletions(-)
diff mbox series

Patch

diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index a89ff96fa..a7abe158a 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -110,7 +110,7 @@  struct conn {
     /* Mutable data. */
     struct ovs_mutex lock; /* Guards all mutable fields. */
     ovs_u128 label;
-    long long expiration;
+    atomic_llong expiration;
     uint32_t mark;
     int seq_skew;
 
diff --git a/lib/conntrack-tp.c b/lib/conntrack-tp.c
index 117810528..cdb3639de 100644
--- a/lib/conntrack-tp.c
+++ b/lib/conntrack-tp.c
@@ -257,7 +257,7 @@  conn_update_expiration(struct conntrack *ct, struct conn *conn,
                 ct_timeout_str[tm], conn->key_node[CT_DIR_FWD].key.zone,
                 conn->tp_id, val);
 
-    conn->expiration = now + val * 1000;
+    atomic_store_relaxed(&conn->expiration, now + val * 1000);
 }
 
 /* ct_lock must be held. */
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 14f7d7ac7..f2dad8158 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -102,6 +102,7 @@  static enum ct_update_res conn_update(struct conntrack *ct, struct conn *conn,
                                       struct dp_packet *pkt,
                                       struct conn_lookup_ctx *ctx,
                                       long long now);
+static long long int conn_expiration(const struct conn *conn);
 static bool conn_expired(struct conn *, long long now);
 static void set_mark(struct dp_packet *, struct conn *,
                      uint32_t val, uint32_t mask);
@@ -582,7 +583,6 @@  conn_key_lookup__(struct conntrack *ct, unsigned bucket,
             conn_clean(ct, conn);
             continue;
         }
-
         for (int i = CT_DIR_FWD; i < CT_DIR_MAX; i++) {
             if (!conn_key_cmp(&conn->key_node[i].key, key)) {
                 found = true;
@@ -1063,13 +1063,10 @@  un_nat_packet(struct dp_packet *pkt, const struct conn *conn,
 static void
 conn_seq_skew_set(struct conntrack *ct, const struct conn *conn_in,
                   long long now, int seq_skew, bool seq_skew_dir)
-    OVS_NO_THREAD_SAFETY_ANALYSIS
 {
     struct conn *conn;
-    ovs_mutex_unlock(&conn_in->lock);
-    conn_lookup_gc(ct, &conn_in->key_node[CT_DIR_FWD].key, now, &conn, NULL);
-    ovs_mutex_lock(&conn_in->lock);
 
+    conn_lookup_gc(ct, &conn_in->key_node[CT_DIR_FWD].key, now, &conn, NULL);
     if (conn && seq_skew) {
         conn->seq_skew = seq_skew;
         conn->seq_skew_dir = seq_skew_dir;
@@ -1661,9 +1658,7 @@  sweep_bucket(struct conntrack *ct, struct ct_bucket *bucket,
         }
 
         conn = CONTAINER_OF(keyn, struct conn, key_node[keyn->key.dir]);
-        ovs_mutex_lock(&conn->lock);
-        expiration = conn->expiration;
-        ovs_mutex_unlock(&conn->lock);
+        expiration = conn_expiration(conn);
 
         if (now >= expiration) {
             conn_clean(ct, conn);
@@ -2671,12 +2666,20 @@  conn_update(struct conntrack *ct, struct conn *conn, struct dp_packet *pkt,
     return update_res;
 }
 
+static long long int
+conn_expiration(const struct conn *conn)
+{
+    long long int expiration;
+
+    atomic_read_relaxed(&CONST_CAST(struct conn *, conn)->expiration,
+                        &expiration);
+    return expiration;
+}
+
 static bool
 conn_expired(struct conn *conn, long long now)
 {
-    ovs_mutex_lock(&conn->lock);
-    bool expired = now >= conn->expiration ? true : false;
-    ovs_mutex_unlock(&conn->lock);
+    bool expired = now >= conn_expiration(conn) ? true : false;
     return expired;
 }
 
@@ -2808,7 +2811,7 @@  conn_to_ct_dpif_entry(const struct conn *conn, struct ct_dpif_entry *entry,
     entry->mark = conn->mark;
     memcpy(&entry->labels, &conn->label, sizeof entry->labels);
 
-    long long expiration = conn->expiration - now;
+    long long expiration = conn_expiration(conn) - now;
 
     struct ct_l4_proto *class = l4_protos[key->nw_proto];
     if (class->conn_get_protoinfo) {