[ovs-dev] [PATCH] conntrack: replace conntrack lock form mutex to spinlock

Aaron Conole aconole at redhat.com
Thu Jul 8 20:05:27 UTC 2021


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)?

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

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