[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