| 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 |
| 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.
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 --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);