[ovs-dev] [PATCH v3 7/8] conntrack: Inverse conn and ct lock precedence

[wenxu]

Hi Gaetan,


First, Thanks for your patch. This is very useful for us. But maybe 
there are some question need to be checked.


> 
>     ovs_mutex_unlock(&ct->ct_lock);
>@@ -1034,7 +1057,6 @@ 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;
>     struct conn *nat_conn = NULL;
>@@ -1113,13 +1135,18 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
>             nat_conn->alg = NULL;
>             nat_conn->nat_conn = NULL;
>             uint32_t nat_hash = conn_key_hash(&nat_conn->key, ct->hash_basis);
>+            ovs_mutex_lock(&ct->ct_lock);
>             cmap_insert(&ct->conns, &nat_conn->cm_node, nat_hash);
>+            ovs_mutex_unlock(&ct->ct_lock);
>         }
I think for nat case the port and ip selection function also should contain in the ct_lock
For snat example 10.0.0.2 and 10.0.0.3 both snat to 1.1.1.7, if both the 10.0.0.2 and
10.0.0.3 using the same port to access the same  client. In default both of them will 
select the original port. 
> 

>@@ -1434,12 +1457,10 @@ 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);
>         }
I think the conn_lookup should contained in the ct_lock.  If there are two packet create same conntrack
in different pmd,  At the same time both of them find no conntrack through lookup_conn. Then there will
be two conntracks insert to the systems.


>-        ovs_mutex_unlock(&ct->ct_lock);
>     }
> 
>     write_ct_md(pkt, zone, conn, &ctx->key, alg_exp);
>@@ -1564,8 +1585,6 @@ ct_sweep(struct conntrack *ct, long long now, size_t limit)
>     struct mpsc_queue_node *node;
>     size_t count = 0;
> 
>-    ovs_mutex_lock(&ct->ct_lock);
>-
>     for (unsigned i = 0; i < N_CT_TM; i++) {
>         struct conn *end_of_queue = NULL;
> 
>@@ -1630,7 +1649,6 @@ ct_sweep(struct conntrack *ct, long long now, size_t limit)
> out:
>     VLOG_DBG("conntrack cleanup %"PRIuSIZE" entries in %lld msec", count,
>              time_msec() - now);
>-    ovs_mutex_unlock(&ct->ct_lock);
>     return min_expiration;
> }
> 
>@@ -2688,13 +2706,11 @@ conntrack_flush(struct conntrack *ct, const uint16_t *zone)
> {
>     struct conn *conn;
> 
>-    ovs_mutex_lock(&ct->ct_lock);
>     CMAP_FOR_EACH (conn, cm_node, &ct->conns) {
>         if (!zone || *zone == conn->key.zone) {
>             conn_clean_one(ct, conn);
>         }
>     }
>-    ovs_mutex_unlock(&ct->ct_lock);
> 
>     return 0;
> }
>@@ -2709,7 +2725,6 @@ conntrack_flush_tuple(struct conntrack *ct, const struct ct_dpif_tuple *tuple,
> 
>     memset(&key, 0, sizeof(key));
>     tuple_to_conn_key(tuple, zone, &key);
>-    ovs_mutex_lock(&ct->ct_lock);
>     conn_lookup(ct, &key, time_msec(), &conn, NULL);
> 
>     if (conn && conn->conn_type == CT_CONN_TYPE_DEFAULT) {
>@@ -2719,7 +2734,6 @@ conntrack_flush_tuple(struct conntrack *ct, const struct ct_dpif_tuple *tuple,
>         error = ENOENT;
>     }
> 
>-    ovs_mutex_unlock(&ct->ct_lock);
>     return error;
> }
> 
>-- 
>2.31.1
>
>_______________________________________________
>dev mailing list
>dev at openvswitch.org
>https://mail.openvswitch.org/mailman/listinfo/ovs-dev