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
>[email protected]
>https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
