| Message ID | 1697513128-25990-1-git-send-email-wenx05124561@163.com |
|---|---|
| State | Under Review |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | [ovs-dev] conntrack: Short the scope of ct_lock in new conn setup. | expand |
| 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 |
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.
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.
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);