diff mbox series

[ovs-dev] conntrack: Short the scope of ct_lock in new conn setup.

Message ID 1697513128-25990-1-git-send-email-wenx05124561@163.com
State Changes Requested
Delegated to: aaron conole
Headers show
Series [ovs-dev] conntrack: Short the scope of ct_lock in new conn setup. | expand

Checks

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

Commit Message

wenxu Oct. 17, 2023, 3:25 a.m. UTC
From: wenxu <wenx05124561@163.com>

There is a big scope ct_lock for new conn setup. The
ct_lock should be hold only for conns map insert and
expire rculist insert.

Signed-off-by: wenxu <wenx05124561@163.com>
---
 lib/conntrack.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

Comments

Simon Horman Oct. 19, 2023, 11:46 a.m. UTC | #1
On Tue, Oct 17, 2023 at 11:25:28AM +0800, wenx05124561@163.com wrote:
> From: wenxu <wenx05124561@163.com>
> 
> There is a big scope ct_lock for new conn setup. The
> ct_lock should be hold only for conns map insert and
> expire rculist insert.
> 
> Signed-off-by: wenxu <wenx05124561@163.com>

Hi Wenxu,

thanks for your patch.

My main concern here is that while the scope of the lock is reduced (good!)
it seems that a lot of work is now done speculatively in conn_not_found()
only to be unwound if conn_lookup() is true. I'm unsure if this is a
problem or not, but it does seem worth discussing, which leads to my second
point.

I am wondering if this resolves a problem that you have observed,
or it is more of a theoretical cleanup based on the (good) idea that
critical sections should be small.
wenxu Oct. 23, 2023, 6:39 a.m. UTC | #2
At 2023-10-19 19:46:48, "Simon Horman" <horms@ovn.org> wrote:
>On Tue, Oct 17, 2023 at 11:25:28AM +0800, wenx05124561@163.com wrote:
>> From: wenxu <wenx05124561@163.com>
>> 
>> There is a big scope ct_lock for new conn setup. The
>> ct_lock should be hold only for conns map insert and
>> expire rculist insert.
>> 
>> Signed-off-by: wenxu <wenx05124561@163.com>
>
>Hi Wenxu,
>
>thanks for your patch.
>
>My main concern here is that while the scope of the lock is reduced (good!)
>it seems that a lot of work is now done speculatively in conn_not_found()
>only to be unwound if conn_lookup() is true. I'm unsure if this is a
>problem or not, but it does seem worth discussing, which leads to my second
>point.
>
>I am wondering if this resolves a problem that you have observed,
>or it is more of a theoretical cleanup based on the (good) idea that

>critical sections should be small.


Yes, It just follow the kernel datapath and base on the idea that critical sections should be small.
Eelco Chaudron Aug. 16, 2024, 1:50 p.m. UTC | #3
On 23 Oct 2023, at 8:39, wenxu wrote:

> At 2023-10-19 19:46:48, "Simon Horman" <horms@ovn.org> wrote:
>> On Tue, Oct 17, 2023 at 11:25:28AM +0800, wenx05124561@163.com wrote:
>>> From: wenxu <wenx05124561@163.com>
>>>
>>> There is a big scope ct_lock for new conn setup. The
>>> ct_lock should be hold only for conns map insert and
>>> expire rculist insert.
>>>
>>> Signed-off-by: wenxu <wenx05124561@163.com>
>>
>> Hi Wenxu,
>>
>> thanks for your patch.
>>
>> My main concern here is that while the scope of the lock is reduced (good!)
>> it seems that a lot of work is now done speculatively in conn_not_found()
>> only to be unwound if conn_lookup() is true. I'm unsure if this is a
>> problem or not, but it does seem worth discussing, which leads to my second
>> point.

Looks like the conversation on this part has gone stale. CC’ed Aaron and Paolo who are more experienced with the conntrack code. Maybe they can comment on this change?

Thanks,

Eelco


>> I am wondering if this resolves a problem that you have observed,
>> or it is more of a theoretical cleanup based on the (good) idea that
>
>> critical sections should be small.
>
>
> Yes, It just follow the kernel datapath and base on the idea that critical sections should be small.
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 47a443f..678e23b 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -50,6 +50,7 @@  COVERAGE_DEFINE(conntrack_full);
 COVERAGE_DEFINE(conntrack_l3csum_err);
 COVERAGE_DEFINE(conntrack_l4csum_err);
 COVERAGE_DEFINE(conntrack_lookup_natted_miss);
+COVERAGE_DEFINE(conntrack_duplicate);
 
 struct conn_lookup_ctx {
     struct conn_key key;
@@ -893,9 +894,9 @@  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
                const struct nat_action_info_t *nat_action_info,
                const char *helper, const struct alg_exp_node *alg_exp,
                enum ct_alg_ctl_type ct_alg_ctl, uint32_t tp_id)
-    OVS_REQUIRES(ct->ct_lock)
 {
     struct conn *nc = NULL;
+    uint32_t rev_hash;
 
     if (!valid_new(pkt, &ctx->key)) {
         pkt->md.ct_state = CS_INVALID;
@@ -961,17 +962,27 @@  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
             }
 
             nat_packet(pkt, nc, false, ctx->icmp_related);
-            uint32_t rev_hash = conn_key_hash(&rev_key_node->key,
-                                              ct->hash_basis);
-            cmap_insert(&ct->conns, &rev_key_node->cm_node, rev_hash);
+            rev_hash = conn_key_hash(&rev_key_node->key, ct->hash_basis);
         }
 
         ovs_mutex_init_adaptive(&nc->lock);
         atomic_flag_clear(&nc->reclaimed);
         fwd_key_node->dir = CT_DIR_FWD;
         rev_key_node->dir = CT_DIR_REV;
+        ovs_mutex_lock(&ct->ct_lock);
+        if (conn_lookup(ct, &ctx->key, now, NULL, NULL)) {
+            ovs_mutex_unlock(&ct->ct_lock);
+            COVERAGE_INC(conntrack_duplicate);
+            ovs_mutex_destroy(&nc->lock);
+            delete_conn__(nc);
+            return NULL;
+        }
+        if (nat_action_info) {
+            cmap_insert(&ct->conns, &rev_key_node->cm_node, rev_hash);
+        }
         cmap_insert(&ct->conns, &fwd_key_node->cm_node, ctx->hash);
         conn_expire_push_front(ct, nc);
+        ovs_mutex_unlock(&ct->ct_lock);
         atomic_count_inc(&ct->n_conn);
         ctx->conn = nc; /* For completeness. */
         if (zl) {
@@ -1290,12 +1301,8 @@  process_one(struct conntrack *ct, struct dp_packet *pkt,
         }
         ovs_rwlock_unlock(&ct->resources_lock);
 
-        ovs_mutex_lock(&ct->ct_lock);
-        if (!conn_lookup(ct, &ctx->key, now, NULL, NULL)) {
-            conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
-                                  helper, alg_exp, ct_alg_ctl, tp_id);
-        }
-        ovs_mutex_unlock(&ct->ct_lock);
+        conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
+                              helper, alg_exp, ct_alg_ctl, tp_id);
     }
 
     write_ct_md(pkt, zone, conn, &ctx->key, alg_exp);