[ovs-dev] [PATCH] conntrack: replace conntrack lock form mutex to spinlock
wenxu
wenxu at ucloud.cn
Fri Jul 9 04:17:48 UTC 2021
From: Aaron Conole <aconole at redhat.com>
Date: 2021-07-09 04:05:27
To: wenxu at ucloud.cn
Cc: blp at ovn.org,dlu998 at gmail.com,i.maximets at ovn.org,dev at openvswitch.org
Subject: Re: [ovs-dev] [PATCH] conntrack: replace conntrack lock form mutex to spinlock>wenxu at ucloud.cn writes:
>
>> From: wenxu <wenxu at ucloud.cn>
>>
>> The ct_lock of conntrack is a global lock to protect the
>> conntrack table insert. Mutex lock will add more latency
>> for lock conflict and the poor performance for CPS of new
>> connections creating.
>>
>> With our benchmark four pmd thread with simple conntrack
>> actions, The CPS is 300k. With this patch replace the
>> ct_lock to spinlock, the CPS improves to 500K.
>>
>> Signed-off-by: wenxu <wenxu at ucloud.cn>
>> ---
>
>Can you also include the cpu utilization changes you observed (if any -
>were you using DPDK ports)?
Yes I using the dpdk port. Original I show the cpu utilitization with top
There are so many sys cpu usgae(I think for mutex reason). with the spinlock
There are only little sys cpu usage
>
>> lib/conntrack-private.h | 2 +-
>> lib/conntrack-tp.c | 22 +++++++++++-----------
>> lib/conntrack.c | 48 ++++++++++++++++++++++++------------------------
>> 3 files changed, 36 insertions(+), 36 deletions(-)
>>
>> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
>> index e8332bd..e72615f 100644
>> --- a/lib/conntrack-private.h
>> +++ b/lib/conntrack-private.h
>> @@ -156,7 +156,7 @@ enum ct_timeout {
>> };
>>
>> struct conntrack {
>> - struct ovs_mutex ct_lock; /* Protects 2 following fields. */
>> + struct ovs_spin ct_lock; /* Protects 2 following fields. */
>
>You probably see that ovs_spin isn't supported on all platforms.
>We might be able to build a usable shim by making the ovs_spin datatype
>implemented in terms of ovs_mutex_trylock() in a loop.
Yes, will do.
>
>WDYT?
>
>> struct cmap conns OVS_GUARDED;
>> struct ovs_list exp_lists[N_CT_TM] OVS_GUARDED;
>> struct hmap zone_limits OVS_GUARDED;
>> diff --git a/lib/conntrack-tp.c b/lib/conntrack-tp.c
>> index a586d3a..3f15064 100644
>> --- a/lib/conntrack-tp.c
>> +++ b/lib/conntrack-tp.c
>> @@ -67,14 +67,14 @@ timeout_policy_get(struct conntrack *ct, int32_t tp_id)
>> {
>> struct timeout_policy *tp;
>>
>> - ovs_mutex_lock(&ct->ct_lock);
>> + ovs_spin_lock(&ct->ct_lock);
>> tp = timeout_policy_lookup(ct, tp_id);
>> if (!tp) {
>> - ovs_mutex_unlock(&ct->ct_lock);
>> + ovs_spin_unlock(&ct->ct_lock);
>> return NULL;
>> }
>>
>> - ovs_mutex_unlock(&ct->ct_lock);
>> + ovs_spin_unlock(&ct->ct_lock);
>> return tp;
>> }
>>
>> @@ -158,9 +158,9 @@ timeout_policy_delete(struct conntrack *ct, uint32_t tp_id)
>> {
>> int err;
>>
>> - ovs_mutex_lock(&ct->ct_lock);
>> + ovs_spin_lock(&ct->ct_lock);
>> err = timeout_policy_delete__(ct, tp_id);
>> - ovs_mutex_unlock(&ct->ct_lock);
>> + ovs_spin_unlock(&ct->ct_lock);
>> return err;
>> }
>>
>> @@ -185,13 +185,13 @@ timeout_policy_update(struct conntrack *ct,
>> int err = 0;
>> uint32_t tp_id = new_tp->policy.id;
>>
>> - ovs_mutex_lock(&ct->ct_lock);
>> + ovs_spin_lock(&ct->ct_lock);
>> struct timeout_policy *tp = timeout_policy_lookup(ct, tp_id);
>> if (tp) {
>> err = timeout_policy_delete__(ct, tp_id);
>> }
>> timeout_policy_create(ct, new_tp);
>> - ovs_mutex_unlock(&ct->ct_lock);
>> + ovs_spin_unlock(&ct->ct_lock);
>> return err;
>> }
>>
>> @@ -238,7 +238,7 @@ conn_update_expiration__(struct conntrack *ct, struct conn *conn,
>> {
>> ovs_mutex_unlock(&conn->lock);
>>
>> - ovs_mutex_lock(&ct->ct_lock);
>> + ovs_spin_lock(&ct->ct_lock);
>> ovs_mutex_lock(&conn->lock);
>> if (!conn->cleaned) {
>> conn->expiration = now + tp_value * 1000;
>> @@ -246,7 +246,7 @@ conn_update_expiration__(struct conntrack *ct, struct conn *conn,
>> ovs_list_push_back(&ct->exp_lists[tm], &conn->exp_node);
>> }
>> ovs_mutex_unlock(&conn->lock);
>> - ovs_mutex_unlock(&ct->ct_lock);
>> + ovs_spin_unlock(&ct->ct_lock);
>>
>> ovs_mutex_lock(&conn->lock);
>> }
>> @@ -262,7 +262,7 @@ conn_update_expiration(struct conntrack *ct, struct conn *conn,
>>
>> ovs_mutex_unlock(&conn->lock);
>>
>> - ovs_mutex_lock(&ct->ct_lock);
>> + ovs_spin_lock(&ct->ct_lock);
>> ovs_mutex_lock(&conn->lock);
>> tp = timeout_policy_lookup(ct, conn->tp_id);
>> if (tp) {
>> @@ -271,7 +271,7 @@ conn_update_expiration(struct conntrack *ct, struct conn *conn,
>> val = ct_dpif_netdev_tp_def[tm_to_ct_dpif_tp(tm)];
>> }
>> ovs_mutex_unlock(&conn->lock);
>> - ovs_mutex_unlock(&ct->ct_lock);
>> + ovs_spin_unlock(&ct->ct_lock);
>>
>> ovs_mutex_lock(&conn->lock);
>> VLOG_DBG_RL(&rl, "Update timeout %s zone=%u with policy id=%d "
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index 2e803ca..639ddf4 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -298,8 +298,8 @@ conntrack_init(void)
>> hindex_init(&ct->alg_expectation_refs);
>> ovs_rwlock_unlock(&ct->resources_lock);
>>
>> - ovs_mutex_init_adaptive(&ct->ct_lock);
>> - ovs_mutex_lock(&ct->ct_lock);
>> + ovs_spin_init(&ct->ct_lock);
>> + ovs_spin_lock(&ct->ct_lock);
>> cmap_init(&ct->conns);
>> for (unsigned i = 0; i < ARRAY_SIZE(ct->exp_lists); i++) {
>> ovs_list_init(&ct->exp_lists[i]);
>> @@ -307,7 +307,7 @@ conntrack_init(void)
>> hmap_init(&ct->zone_limits);
>> ct->zone_limit_seq = 0;
>> timeout_policy_init(ct);
>> - ovs_mutex_unlock(&ct->ct_lock);
>> + ovs_spin_unlock(&ct->ct_lock);
>>
>> ct->hash_basis = random_uint32();
>> atomic_count_init(&ct->n_conn, 0);
>> @@ -364,13 +364,13 @@ zone_limit_lookup_or_default(struct conntrack *ct, int32_t zone)
>> struct conntrack_zone_limit
>> zone_limit_get(struct conntrack *ct, int32_t zone)
>> {
>> - ovs_mutex_lock(&ct->ct_lock);
>> + ovs_spin_lock(&ct->ct_lock);
>> struct conntrack_zone_limit czl = {DEFAULT_ZONE, 0, 0, 0};
>> struct zone_limit *zl = zone_limit_lookup_or_default(ct, zone);
>> if (zl) {
>> czl = zl->czl;
>> }
>> - ovs_mutex_unlock(&ct->ct_lock);
>> + ovs_spin_unlock(&ct->ct_lock);
>> return czl;
>> }
>>
>> @@ -395,7 +395,7 @@ int
>> zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
>> {
>> int err = 0;
>> - ovs_mutex_lock(&ct->ct_lock);
>> + ovs_spin_lock(&ct->ct_lock);
>> struct zone_limit *zl = zone_limit_lookup(ct, zone);
>> if (zl) {
>> zl->czl.limit = limit;
>> @@ -409,7 +409,7 @@ zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
>> zone);
>> }
>> }
>> - ovs_mutex_unlock(&ct->ct_lock);
>> + ovs_spin_unlock(&ct->ct_lock);
>> return err;
>> }
>>
>> @@ -424,7 +424,7 @@ zone_limit_clean(struct conntrack *ct, struct zone_limit *zl)
>> int
>> zone_limit_delete(struct conntrack *ct, uint16_t zone)
>> {
>> - ovs_mutex_lock(&ct->ct_lock);
>> + ovs_spin_lock(&ct->ct_lock);
>> struct zone_limit *zl = zone_limit_lookup(ct, zone);
>> if (zl) {
>> zone_limit_clean(ct, zl);
>> @@ -433,7 +433,7 @@ zone_limit_delete(struct conntrack *ct, uint16_t zone)
>> VLOG_INFO("Attempted delete of non-existent zone limit: zone %d",
>> zone);
>> }
>> - ovs_mutex_unlock(&ct->ct_lock);
>> + ovs_spin_unlock(&ct->ct_lock);
>> return 0;
>> }
>>
>> @@ -497,7 +497,7 @@ conntrack_destroy(struct conntrack *ct)
>> pthread_join(ct->clean_thread, NULL);
>> latch_destroy(&ct->clean_thread_exit);
>>
>> - ovs_mutex_lock(&ct->ct_lock);
>> + ovs_spin_lock(&ct->ct_lock);
>> CMAP_FOR_EACH (conn, cm_node, &ct->conns) {
>> conn_clean_one(ct, conn);
>> }
>> @@ -515,8 +515,8 @@ conntrack_destroy(struct conntrack *ct)
>> }
>> hmap_destroy(&ct->timeout_policies);
>>
>> - ovs_mutex_unlock(&ct->ct_lock);
>> - ovs_mutex_destroy(&ct->ct_lock);
>> + ovs_spin_unlock(&ct->ct_lock);
>> + ovs_spin_destroy(&ct->ct_lock);
>>
>> ovs_rwlock_wrlock(&ct->resources_lock);
>> struct alg_exp_node *alg_exp_node;
>> @@ -1116,11 +1116,11 @@ conn_update_state(struct conntrack *ct, struct dp_packet *pkt,
>> pkt->md.ct_state = CS_INVALID;
>> break;
>> case CT_UPDATE_NEW:
>> - ovs_mutex_lock(&ct->ct_lock);
>> + ovs_spin_lock(&ct->ct_lock);
>> if (conn_lookup(ct, &conn->key, now, NULL, NULL)) {
>> conn_clean(ct, conn);
>> }
>> - ovs_mutex_unlock(&ct->ct_lock);
>> + ovs_spin_unlock(&ct->ct_lock);
>> create_new_conn = true;
>> break;
>> case CT_UPDATE_VALID_NEW:
>> @@ -1302,11 +1302,11 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
>>
>> /* Delete found entry if in wrong direction. 'force' implies commit. */
>> if (OVS_UNLIKELY(force && ctx->reply && conn)) {
>> - ovs_mutex_lock(&ct->ct_lock);
>> + ovs_spin_lock(&ct->ct_lock);
>> if (conn_lookup(ct, &conn->key, now, NULL, NULL)) {
>> conn_clean(ct, conn);
>> }
>> - ovs_mutex_unlock(&ct->ct_lock);
>> + ovs_spin_unlock(&ct->ct_lock);
>> conn = NULL;
>> }
>>
>> @@ -1370,12 +1370,12 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
>> }
>> ovs_rwlock_unlock(&ct->resources_lock);
>>
>> - ovs_mutex_lock(&ct->ct_lock);
>> + ovs_spin_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);
>> + ovs_spin_unlock(&ct->ct_lock);
>> }
>>
>> write_ct_md(pkt, zone, conn, &ctx->key, alg_exp);
>> @@ -1498,7 +1498,7 @@ ct_sweep(struct conntrack *ct, long long now, size_t limit)
>> long long min_expiration = LLONG_MAX;
>> size_t count = 0;
>>
>> - ovs_mutex_lock(&ct->ct_lock);
>> + ovs_spin_lock(&ct->ct_lock);
>>
>> for (unsigned i = 0; i < N_CT_TM; i++) {
>> LIST_FOR_EACH_SAFE (conn, next, exp_node, &ct->exp_lists[i]) {
>> @@ -1523,7 +1523,7 @@ 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);
>> + ovs_spin_unlock(&ct->ct_lock);
>> return min_expiration;
>> }
>>
>> @@ -2583,13 +2583,13 @@ conntrack_flush(struct conntrack *ct, const uint16_t *zone)
>> {
>> struct conn *conn;
>>
>> - ovs_mutex_lock(&ct->ct_lock);
>> + ovs_spin_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);
>> + ovs_spin_unlock(&ct->ct_lock);
>>
>> return 0;
>> }
>> @@ -2604,7 +2604,7 @@ 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);
>> + ovs_spin_lock(&ct->ct_lock);
>> conn_lookup(ct, &key, time_msec(), &conn, NULL);
>>
>> if (conn && conn->conn_type == CT_CONN_TYPE_DEFAULT) {
>> @@ -2614,7 +2614,7 @@ conntrack_flush_tuple(struct conntrack *ct, const struct ct_dpif_tuple *tuple,
>> error = ENOENT;
>> }
>>
>> - ovs_mutex_unlock(&ct->ct_lock);
>> + ovs_spin_unlock(&ct->ct_lock);
>> return error;
>> }
>
More information about the dev
mailing list