On Sat, Jul 10, 2021, at 16:55, wenxu wrote:
> 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.
>
Hi!
I think you are right. I was going too fast removing those locks.
When doing looking before insertion to avoid double-insertion, the
initial lookup must be part of the insertion critical section,
otherwise it will race.
I have fixed it. It has an impact on performance of course, but
there is no way to avoid it. I will send a new version soon.
Thanks for the review!
> >- 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
>
--
Gaetan Rivet
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
