diff mbox series

[ovs-dev,v1,2/9] conntrack: Use a cmap to store zone limits

Message ID f6d6286396b99306a2e5aefbcdb1c56cce7e5d02.1613557616.git.grive@u256.net
State Changes Requested
Headers show
Series conntrack: improve multithread scalability | expand

Commit Message

Gaetan Rivet Feb. 17, 2021, 4:34 p.m. UTC 
Change the data structure from hmap to cmap for zone limits.
As they are shared amongst multiple conntrack users, multiple
readers want to check the current zone limit state before progressing in
their processing. Using a CMAP allows doing lookups without taking the
global 'ct_lock', thus reducing contention.

Signed-off-by: Gaetan Rivet <grive@u256.net>
Reviewed-by: Eli Britstein <elibr@nvidia.com>
---
 lib/conntrack-private.h |  2 +-
 lib/conntrack.c         | 72 ++++++++++++++++++++++++++++-------------
 lib/conntrack.h         |  2 +-
 lib/dpif-netdev.c       |  5 +--
 4 files changed, 55 insertions(+), 26 deletions(-)

Comments

William Tu Feb. 23, 2021, 10:55 p.m. UTC | #1 
Thanks, I have one question inline.

On Wed, Feb 17, 2021 at 8:34 AM Gaetan Rivet <grive@u256.net> wrote:
>
> Change the data structure from hmap to cmap for zone limits.
> As they are shared amongst multiple conntrack users, multiple
> readers want to check the current zone limit state before progressing in
> their processing. Using a CMAP allows doing lookups without taking the
> global 'ct_lock', thus reducing contention.
>
> Signed-off-by: Gaetan Rivet <grive@u256.net>
> Reviewed-by: Eli Britstein <elibr@nvidia.com>
> ---
>  lib/conntrack-private.h |  2 +-
>  lib/conntrack.c         | 72 ++++++++++++++++++++++++++++-------------
>  lib/conntrack.h         |  2 +-
>  lib/dpif-netdev.c       |  5 +--
>  4 files changed, 55 insertions(+), 26 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 4b6f9eae3..f2cbf657e 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -176,7 +176,7 @@ struct conntrack {
>      struct ovs_mutex ct_lock; /* Protects 2 following fields. */
>      struct cmap conns OVS_GUARDED;
>      struct rculist exp_lists[N_CT_TM] OVS_GUARDED;
> -    struct hmap zone_limits OVS_GUARDED;
> +    struct cmap zone_limits OVS_GUARDED;
>      struct hmap timeout_policies OVS_GUARDED;
>      uint32_t hash_basis; /* Salt for hashing a connection key. */
>      pthread_t clean_thread; /* Periodically cleans up connection tracker. */
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index ac12f9196..c218200dc 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -79,7 +79,7 @@ enum ct_alg_ctl_type {
>  };
>
>  struct zone_limit {
> -    struct hmap_node node;
> +    struct cmap_node node;
>      struct conntrack_zone_limit czl;
>  };
>
> @@ -303,7 +303,7 @@ conntrack_init(void)
>      for (unsigned i = 0; i < ARRAY_SIZE(ct->exp_lists); i++) {
>          rculist_init(&ct->exp_lists[i]);
>      }
> -    hmap_init(&ct->zone_limits);
> +    cmap_init(&ct->zone_limits);
>      ct->zone_limit_seq = 0;
>      timeout_policy_init(ct);
>      ovs_mutex_unlock(&ct->ct_lock);
> @@ -339,12 +339,25 @@ zone_key_hash(int32_t zone, uint32_t basis)
>  }
>
>  static struct zone_limit *
> -zone_limit_lookup(struct conntrack *ct, int32_t zone)
> +zone_limit_lookup_protected(struct conntrack *ct, int32_t zone)
>      OVS_REQUIRES(ct->ct_lock)
>  {
>      uint32_t hash = zone_key_hash(zone, ct->hash_basis);
>      struct zone_limit *zl;
> -    HMAP_FOR_EACH_IN_BUCKET (zl, node, hash, &ct->zone_limits) {
> +    CMAP_FOR_EACH_WITH_HASH_PROTECTED (zl, node, hash, &ct->zone_limits) {
> +        if (zl->czl.zone == zone) {
> +            return zl;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static struct zone_limit *
> +zone_limit_lookup(struct conntrack *ct, int32_t zone)
> +{
> +    uint32_t hash = zone_key_hash(zone, ct->hash_basis);
> +    struct zone_limit *zl;
> +    CMAP_FOR_EACH_WITH_HASH (zl, node, hash, &ct->zone_limits) {
>          if (zl->czl.zone == zone) {
>              return zl;
>          }
> @@ -354,7 +367,6 @@ zone_limit_lookup(struct conntrack *ct, int32_t zone)
>
>  static struct zone_limit *
>  zone_limit_lookup_or_default(struct conntrack *ct, int32_t zone)
> -    OVS_REQUIRES(ct->ct_lock)
>  {
>      struct zone_limit *zl = zone_limit_lookup(ct, zone);
>      return zl ? zl : zone_limit_lookup(ct, DEFAULT_ZONE);
> @@ -363,13 +375,16 @@ 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);
> -    struct conntrack_zone_limit czl = {DEFAULT_ZONE, 0, 0, 0};
> +    struct conntrack_zone_limit czl = {
> +        .zone = DEFAULT_ZONE,
> +        .limit = 0,
> +        .count = ATOMIC_COUNT_INIT(0),
> +        .zone_limit_seq = 0,
> +    };
>      struct zone_limit *zl = zone_limit_lookup_or_default(ct, zone);
>      if (zl) {
>          czl = zl->czl;
>      }
> -    ovs_mutex_unlock(&ct->ct_lock);
>      return czl;
>  }
>
> @@ -377,13 +392,19 @@ static int
>  zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit)
>      OVS_REQUIRES(ct->ct_lock)
>  {
> +    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
> +
> +    if (zl) {
> +        return 0;
> +    }
> +
>      if (zone >= DEFAULT_ZONE && zone <= MAX_ZONE) {
> -        struct zone_limit *zl = xzalloc(sizeof *zl);
> +        zl = xzalloc(sizeof *zl);
>          zl->czl.limit = limit;
>          zl->czl.zone = zone;
>          zl->czl.zone_limit_seq = ct->zone_limit_seq++;
>          uint32_t hash = zone_key_hash(zone, ct->hash_basis);
> -        hmap_insert(&ct->zone_limits, &zl->node, hash);
> +        cmap_insert(&ct->zone_limits, &zl->node, hash);
>          return 0;
>      } else {
>          return EINVAL;
> @@ -394,13 +415,14 @@ int
>  zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
>  {
>      int err = 0;
> -    ovs_mutex_lock(&ct->ct_lock);
>      struct zone_limit *zl = zone_limit_lookup(ct, zone);
>      if (zl) {
>          zl->czl.limit = limit;
>          VLOG_INFO("Changed zone limit of %u for zone %d", limit, zone);
>      } else {
> +        ovs_mutex_lock(&ct->ct_lock);
>          err = zone_limit_create(ct, zone, limit);
> +        ovs_mutex_unlock(&ct->ct_lock);
>          if (!err) {
>              VLOG_INFO("Created zone limit of %u for zone %d", limit, zone);
>          } else {
> @@ -408,7 +430,6 @@ zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
>                        zone);
>          }
>      }
> -    ovs_mutex_unlock(&ct->ct_lock);
>      return err;
>  }
>
> @@ -416,23 +437,25 @@ static void
>  zone_limit_clean(struct conntrack *ct, struct zone_limit *zl)
>      OVS_REQUIRES(ct->ct_lock)
>  {
> -    hmap_remove(&ct->zone_limits, &zl->node);
> -    free(zl);
> +    uint32_t hash = zone_key_hash(zl->czl.zone, ct->hash_basis);
> +    cmap_remove(&ct->zone_limits, &zl->node, hash);
> +    ovsrcu_postpone(free, zl);
>  }
>
>  int
>  zone_limit_delete(struct conntrack *ct, uint16_t zone)
>  {
>      ovs_mutex_lock(&ct->ct_lock);
> -    struct zone_limit *zl = zone_limit_lookup(ct, zone);
> +    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
>      if (zl) {
>          zone_limit_clean(ct, zl);
> +        ovs_mutex_unlock(&ct->ct_lock);
>          VLOG_INFO("Deleted zone limit for zone %d", zone);
>      } else {
> +        ovs_mutex_unlock(&ct->ct_lock);
>          VLOG_INFO("Attempted delete of non-existent zone limit: zone %d",
>                    zone);
>      }
> -    ovs_mutex_unlock(&ct->ct_lock);
>      return 0;
>  }
>
> @@ -449,7 +472,7 @@ conn_clean_cmn(struct conntrack *ct, struct conn *conn)
>
>      struct zone_limit *zl = zone_limit_lookup(ct, conn->admit_zone);
>      if (zl && zl->czl.zone_limit_seq == conn->zone_limit_seq) {
> -        zl->czl.count--;
> +        atomic_count_dec(&zl->czl.count);
>      }
>  }
>
> @@ -503,10 +526,13 @@ conntrack_destroy(struct conntrack *ct)
>      cmap_destroy(&ct->conns);
>
>      struct zone_limit *zl;
> -    HMAP_FOR_EACH_POP (zl, node, &ct->zone_limits) {
> -        free(zl);
> +    CMAP_FOR_EACH (zl, node, &ct->zone_limits) {
> +        uint32_t hash = zone_key_hash(zl->czl.zone, ct->hash_basis);
> +
> +        cmap_remove(&ct->zone_limits, &zl->node, hash);
> +        ovsrcu_postpone(free, zl);
>      }
> -    hmap_destroy(&ct->zone_limits);
> +    cmap_destroy(&ct->zone_limits);
>
>      struct timeout_policy *tp;
>      HMAP_FOR_EACH_POP (tp, node, &ct->timeout_policies) {
> @@ -988,7 +1014,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
>      if (commit) {
>          struct zone_limit *zl = zone_limit_lookup_or_default(ct,
>                                                               ctx->key.zone);
> -        if (zl && zl->czl.count >= zl->czl.limit) {
> +        if (zl && atomic_count_get(&zl->czl.count) >= zl->czl.limit) {
>              return nc;
>          }
>
> @@ -1057,10 +1083,12 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
>          cmap_insert(&ct->conns, &nc->cm_node, ctx->hash);
>          atomic_count_inc(&ct->n_conn);
>          ctx->conn = nc; /* For completeness. */
> +
> +        zl = zone_limit_lookup_or_default(ct, ctx->key.zone);

why calling lookup again here?


>          if (zl) {
>              nc->admit_zone = zl->czl.zone;
>              nc->zone_limit_seq = zl->czl.zone_limit_seq;
> -            zl->czl.count++;
> +            atomic_count_inc(&zl->czl.count);
>          } else {
>              nc->admit_zone = INVALID_ZONE;
>          }
> diff --git a/lib/conntrack.h b/lib/conntrack.h
> index 9553b188a..58b181834 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -108,7 +108,7 @@ struct conntrack_dump {
>  struct conntrack_zone_limit {
>      int32_t zone;
>      uint32_t limit;
> -    uint32_t count;
> +    atomic_count count;
>      uint32_t zone_limit_seq; /* Used to disambiguate zone limit counts. */
>  };
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 4381c618f..9d9767b2c 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -8193,7 +8193,8 @@ dpif_netdev_ct_get_limits(struct dpif *dpif OVS_UNUSED,
>              czl = zone_limit_get(dp->conntrack, zone_limit->zone);
>              if (czl.zone == zone_limit->zone || czl.zone == DEFAULT_ZONE) {
>                  ct_dpif_push_zone_limit(zone_limits_reply, zone_limit->zone,
> -                                        czl.limit, czl.count);
> +                                        czl.limit,
> +                                        atomic_count_get(&czl.count));
>              } else {
>                  return EINVAL;
>              }
> @@ -8203,7 +8204,7 @@ dpif_netdev_ct_get_limits(struct dpif *dpif OVS_UNUSED,
>              czl = zone_limit_get(dp->conntrack, z);
>              if (czl.zone == z) {
>                  ct_dpif_push_zone_limit(zone_limits_reply, z, czl.limit,
> -                                        czl.count);
> +                                        atomic_count_get(&czl.count));
>              }
>          }
>      }
> --
> 2.30.0
>
Gaetan Rivet Feb. 24, 2021, 12:34 a.m. UTC | #2 
On Tue, Feb 23, 2021, at 22:55, William Tu wrote:
> Thanks, I have one question inline.
> 
> On Wed, Feb 17, 2021 at 8:34 AM Gaetan Rivet <grive@u256.net> wrote:
> >
> > Change the data structure from hmap to cmap for zone limits.
> > As they are shared amongst multiple conntrack users, multiple
> > readers want to check the current zone limit state before progressing in
> > their processing. Using a CMAP allows doing lookups without taking the
> > global 'ct_lock', thus reducing contention.
> >
> > Signed-off-by: Gaetan Rivet <grive@u256.net>
> > Reviewed-by: Eli Britstein <elibr@nvidia.com>
> > ---
> >  lib/conntrack-private.h |  2 +-
> >  lib/conntrack.c         | 72 ++++++++++++++++++++++++++++-------------
> >  lib/conntrack.h         |  2 +-
> >  lib/dpif-netdev.c       |  5 +--
> >  4 files changed, 55 insertions(+), 26 deletions(-)
> >
> > diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> > index 4b6f9eae3..f2cbf657e 100644
> > --- a/lib/conntrack-private.h
> > +++ b/lib/conntrack-private.h
> > @@ -176,7 +176,7 @@ struct conntrack {
> >      struct ovs_mutex ct_lock; /* Protects 2 following fields. */
> >      struct cmap conns OVS_GUARDED;
> >      struct rculist exp_lists[N_CT_TM] OVS_GUARDED;
> > -    struct hmap zone_limits OVS_GUARDED;
> > +    struct cmap zone_limits OVS_GUARDED;
> >      struct hmap timeout_policies OVS_GUARDED;
> >      uint32_t hash_basis; /* Salt for hashing a connection key. */
> >      pthread_t clean_thread; /* Periodically cleans up connection tracker. */
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index ac12f9196..c218200dc 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -79,7 +79,7 @@ enum ct_alg_ctl_type {
> >  };
> >
> >  struct zone_limit {
> > -    struct hmap_node node;
> > +    struct cmap_node node;
> >      struct conntrack_zone_limit czl;
> >  };
> >
> > @@ -303,7 +303,7 @@ conntrack_init(void)
> >      for (unsigned i = 0; i < ARRAY_SIZE(ct->exp_lists); i++) {
> >          rculist_init(&ct->exp_lists[i]);
> >      }
> > -    hmap_init(&ct->zone_limits);
> > +    cmap_init(&ct->zone_limits);
> >      ct->zone_limit_seq = 0;
> >      timeout_policy_init(ct);
> >      ovs_mutex_unlock(&ct->ct_lock);
> > @@ -339,12 +339,25 @@ zone_key_hash(int32_t zone, uint32_t basis)
> >  }
> >
> >  static struct zone_limit *
> > -zone_limit_lookup(struct conntrack *ct, int32_t zone)
> > +zone_limit_lookup_protected(struct conntrack *ct, int32_t zone)
> >      OVS_REQUIRES(ct->ct_lock)
> >  {
> >      uint32_t hash = zone_key_hash(zone, ct->hash_basis);
> >      struct zone_limit *zl;
> > -    HMAP_FOR_EACH_IN_BUCKET (zl, node, hash, &ct->zone_limits) {
> > +    CMAP_FOR_EACH_WITH_HASH_PROTECTED (zl, node, hash, &ct->zone_limits) {
> > +        if (zl->czl.zone == zone) {
> > +            return zl;
> > +        }
> > +    }
> > +    return NULL;
> > +}
> > +
> > +static struct zone_limit *
> > +zone_limit_lookup(struct conntrack *ct, int32_t zone)
> > +{
> > +    uint32_t hash = zone_key_hash(zone, ct->hash_basis);
> > +    struct zone_limit *zl;
> > +    CMAP_FOR_EACH_WITH_HASH (zl, node, hash, &ct->zone_limits) {
> >          if (zl->czl.zone == zone) {
> >              return zl;
> >          }
> > @@ -354,7 +367,6 @@ zone_limit_lookup(struct conntrack *ct, int32_t zone)
> >
> >  static struct zone_limit *
> >  zone_limit_lookup_or_default(struct conntrack *ct, int32_t zone)
> > -    OVS_REQUIRES(ct->ct_lock)
> >  {
> >      struct zone_limit *zl = zone_limit_lookup(ct, zone);
> >      return zl ? zl : zone_limit_lookup(ct, DEFAULT_ZONE);
> > @@ -363,13 +375,16 @@ 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);
> > -    struct conntrack_zone_limit czl = {DEFAULT_ZONE, 0, 0, 0};
> > +    struct conntrack_zone_limit czl = {
> > +        .zone = DEFAULT_ZONE,
> > +        .limit = 0,
> > +        .count = ATOMIC_COUNT_INIT(0),
> > +        .zone_limit_seq = 0,
> > +    };
> >      struct zone_limit *zl = zone_limit_lookup_or_default(ct, zone);
> >      if (zl) {
> >          czl = zl->czl;
> >      }
> > -    ovs_mutex_unlock(&ct->ct_lock);
> >      return czl;
> >  }
> >
> > @@ -377,13 +392,19 @@ static int
> >  zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit)
> >      OVS_REQUIRES(ct->ct_lock)
> >  {
> > +    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
> > +
> > +    if (zl) {
> > +        return 0;
> > +    }
> > +
> >      if (zone >= DEFAULT_ZONE && zone <= MAX_ZONE) {
> > -        struct zone_limit *zl = xzalloc(sizeof *zl);
> > +        zl = xzalloc(sizeof *zl);
> >          zl->czl.limit = limit;
> >          zl->czl.zone = zone;
> >          zl->czl.zone_limit_seq = ct->zone_limit_seq++;
> >          uint32_t hash = zone_key_hash(zone, ct->hash_basis);
> > -        hmap_insert(&ct->zone_limits, &zl->node, hash);
> > +        cmap_insert(&ct->zone_limits, &zl->node, hash);
> >          return 0;
> >      } else {
> >          return EINVAL;
> > @@ -394,13 +415,14 @@ int
> >  zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
> >  {
> >      int err = 0;
> > -    ovs_mutex_lock(&ct->ct_lock);
> >      struct zone_limit *zl = zone_limit_lookup(ct, zone);
> >      if (zl) {
> >          zl->czl.limit = limit;
> >          VLOG_INFO("Changed zone limit of %u for zone %d", limit, zone);
> >      } else {
> > +        ovs_mutex_lock(&ct->ct_lock);
> >          err = zone_limit_create(ct, zone, limit);
> > +        ovs_mutex_unlock(&ct->ct_lock);
> >          if (!err) {
> >              VLOG_INFO("Created zone limit of %u for zone %d", limit, zone);
> >          } else {
> > @@ -408,7 +430,6 @@ zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
> >                        zone);
> >          }
> >      }
> > -    ovs_mutex_unlock(&ct->ct_lock);
> >      return err;
> >  }
> >
> > @@ -416,23 +437,25 @@ static void
> >  zone_limit_clean(struct conntrack *ct, struct zone_limit *zl)
> >      OVS_REQUIRES(ct->ct_lock)
> >  {
> > -    hmap_remove(&ct->zone_limits, &zl->node);
> > -    free(zl);
> > +    uint32_t hash = zone_key_hash(zl->czl.zone, ct->hash_basis);
> > +    cmap_remove(&ct->zone_limits, &zl->node, hash);
> > +    ovsrcu_postpone(free, zl);
> >  }
> >
> >  int
> >  zone_limit_delete(struct conntrack *ct, uint16_t zone)
> >  {
> >      ovs_mutex_lock(&ct->ct_lock);
> > -    struct zone_limit *zl = zone_limit_lookup(ct, zone);
> > +    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
> >      if (zl) {
> >          zone_limit_clean(ct, zl);
> > +        ovs_mutex_unlock(&ct->ct_lock);
> >          VLOG_INFO("Deleted zone limit for zone %d", zone);
> >      } else {
> > +        ovs_mutex_unlock(&ct->ct_lock);
> >          VLOG_INFO("Attempted delete of non-existent zone limit: zone %d",
> >                    zone);
> >      }
> > -    ovs_mutex_unlock(&ct->ct_lock);
> >      return 0;
> >  }
> >
> > @@ -449,7 +472,7 @@ conn_clean_cmn(struct conntrack *ct, struct conn *conn)
> >
> >      struct zone_limit *zl = zone_limit_lookup(ct, conn->admit_zone);
> >      if (zl && zl->czl.zone_limit_seq == conn->zone_limit_seq) {
> > -        zl->czl.count--;
> > +        atomic_count_dec(&zl->czl.count);
> >      }
> >  }
> >
> > @@ -503,10 +526,13 @@ conntrack_destroy(struct conntrack *ct)
> >      cmap_destroy(&ct->conns);
> >
> >      struct zone_limit *zl;
> > -    HMAP_FOR_EACH_POP (zl, node, &ct->zone_limits) {
> > -        free(zl);
> > +    CMAP_FOR_EACH (zl, node, &ct->zone_limits) {
> > +        uint32_t hash = zone_key_hash(zl->czl.zone, ct->hash_basis);
> > +
> > +        cmap_remove(&ct->zone_limits, &zl->node, hash);
> > +        ovsrcu_postpone(free, zl);
> >      }
> > -    hmap_destroy(&ct->zone_limits);
> > +    cmap_destroy(&ct->zone_limits);
> >
> >      struct timeout_policy *tp;
> >      HMAP_FOR_EACH_POP (tp, node, &ct->timeout_policies) {
> > @@ -988,7 +1014,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
> >      if (commit) {
> >          struct zone_limit *zl = zone_limit_lookup_or_default(ct,
> >                                                               ctx->key.zone);
> > -        if (zl && zl->czl.count >= zl->czl.limit) {
> > +        if (zl && atomic_count_get(&zl->czl.count) >= zl->czl.limit) {
> >              return nc;
> >          }
> >
> > @@ -1057,10 +1083,12 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
> >          cmap_insert(&ct->conns, &nc->cm_node, ctx->hash);
> >          atomic_count_inc(&ct->n_conn);
> >          ctx->conn = nc; /* For completeness. */
> > +
> > +        zl = zone_limit_lookup_or_default(ct, ctx->key.zone);
> 
> why calling lookup again here?
> 
> 

It seems to be a mistake actually! It's a remain from a previous version, that slipped past rebase as it has no effect.
I've re-checked, I do not see a potential quiescing point between the lookups, so no reason to do it.
Thanks for pointing it out!
diff mbox series

Patch

diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index 4b6f9eae3..f2cbf657e 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -176,7 +176,7 @@  struct conntrack {
     struct ovs_mutex ct_lock; /* Protects 2 following fields. */
     struct cmap conns OVS_GUARDED;
     struct rculist exp_lists[N_CT_TM] OVS_GUARDED;
-    struct hmap zone_limits OVS_GUARDED;
+    struct cmap zone_limits OVS_GUARDED;
     struct hmap timeout_policies OVS_GUARDED;
     uint32_t hash_basis; /* Salt for hashing a connection key. */
     pthread_t clean_thread; /* Periodically cleans up connection tracker. */
diff --git a/lib/conntrack.c b/lib/conntrack.c
index ac12f9196..c218200dc 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -79,7 +79,7 @@  enum ct_alg_ctl_type {
 };
 
 struct zone_limit {
-    struct hmap_node node;
+    struct cmap_node node;
     struct conntrack_zone_limit czl;
 };
 
@@ -303,7 +303,7 @@  conntrack_init(void)
     for (unsigned i = 0; i < ARRAY_SIZE(ct->exp_lists); i++) {
         rculist_init(&ct->exp_lists[i]);
     }
-    hmap_init(&ct->zone_limits);
+    cmap_init(&ct->zone_limits);
     ct->zone_limit_seq = 0;
     timeout_policy_init(ct);
     ovs_mutex_unlock(&ct->ct_lock);
@@ -339,12 +339,25 @@  zone_key_hash(int32_t zone, uint32_t basis)
 }
 
 static struct zone_limit *
-zone_limit_lookup(struct conntrack *ct, int32_t zone)
+zone_limit_lookup_protected(struct conntrack *ct, int32_t zone)
     OVS_REQUIRES(ct->ct_lock)
 {
     uint32_t hash = zone_key_hash(zone, ct->hash_basis);
     struct zone_limit *zl;
-    HMAP_FOR_EACH_IN_BUCKET (zl, node, hash, &ct->zone_limits) {
+    CMAP_FOR_EACH_WITH_HASH_PROTECTED (zl, node, hash, &ct->zone_limits) {
+        if (zl->czl.zone == zone) {
+            return zl;
+        }
+    }
+    return NULL;
+}
+
+static struct zone_limit *
+zone_limit_lookup(struct conntrack *ct, int32_t zone)
+{
+    uint32_t hash = zone_key_hash(zone, ct->hash_basis);
+    struct zone_limit *zl;
+    CMAP_FOR_EACH_WITH_HASH (zl, node, hash, &ct->zone_limits) {
         if (zl->czl.zone == zone) {
             return zl;
         }
@@ -354,7 +367,6 @@  zone_limit_lookup(struct conntrack *ct, int32_t zone)
 
 static struct zone_limit *
 zone_limit_lookup_or_default(struct conntrack *ct, int32_t zone)
-    OVS_REQUIRES(ct->ct_lock)
 {
     struct zone_limit *zl = zone_limit_lookup(ct, zone);
     return zl ? zl : zone_limit_lookup(ct, DEFAULT_ZONE);
@@ -363,13 +375,16 @@  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);
-    struct conntrack_zone_limit czl = {DEFAULT_ZONE, 0, 0, 0};
+    struct conntrack_zone_limit czl = {
+        .zone = DEFAULT_ZONE,
+        .limit = 0,
+        .count = ATOMIC_COUNT_INIT(0),
+        .zone_limit_seq = 0,
+    };
     struct zone_limit *zl = zone_limit_lookup_or_default(ct, zone);
     if (zl) {
         czl = zl->czl;
     }
-    ovs_mutex_unlock(&ct->ct_lock);
     return czl;
 }
 
@@ -377,13 +392,19 @@  static int
 zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit)
     OVS_REQUIRES(ct->ct_lock)
 {
+    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
+
+    if (zl) {
+        return 0;
+    }
+
     if (zone >= DEFAULT_ZONE && zone <= MAX_ZONE) {
-        struct zone_limit *zl = xzalloc(sizeof *zl);
+        zl = xzalloc(sizeof *zl);
         zl->czl.limit = limit;
         zl->czl.zone = zone;
         zl->czl.zone_limit_seq = ct->zone_limit_seq++;
         uint32_t hash = zone_key_hash(zone, ct->hash_basis);
-        hmap_insert(&ct->zone_limits, &zl->node, hash);
+        cmap_insert(&ct->zone_limits, &zl->node, hash);
         return 0;
     } else {
         return EINVAL;
@@ -394,13 +415,14 @@  int
 zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
 {
     int err = 0;
-    ovs_mutex_lock(&ct->ct_lock);
     struct zone_limit *zl = zone_limit_lookup(ct, zone);
     if (zl) {
         zl->czl.limit = limit;
         VLOG_INFO("Changed zone limit of %u for zone %d", limit, zone);
     } else {
+        ovs_mutex_lock(&ct->ct_lock);
         err = zone_limit_create(ct, zone, limit);
+        ovs_mutex_unlock(&ct->ct_lock);
         if (!err) {
             VLOG_INFO("Created zone limit of %u for zone %d", limit, zone);
         } else {
@@ -408,7 +430,6 @@  zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
                       zone);
         }
     }
-    ovs_mutex_unlock(&ct->ct_lock);
     return err;
 }
 
@@ -416,23 +437,25 @@  static void
 zone_limit_clean(struct conntrack *ct, struct zone_limit *zl)
     OVS_REQUIRES(ct->ct_lock)
 {
-    hmap_remove(&ct->zone_limits, &zl->node);
-    free(zl);
+    uint32_t hash = zone_key_hash(zl->czl.zone, ct->hash_basis);
+    cmap_remove(&ct->zone_limits, &zl->node, hash);
+    ovsrcu_postpone(free, zl);
 }
 
 int
 zone_limit_delete(struct conntrack *ct, uint16_t zone)
 {
     ovs_mutex_lock(&ct->ct_lock);
-    struct zone_limit *zl = zone_limit_lookup(ct, zone);
+    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
     if (zl) {
         zone_limit_clean(ct, zl);
+        ovs_mutex_unlock(&ct->ct_lock);
         VLOG_INFO("Deleted zone limit for zone %d", zone);
     } else {
+        ovs_mutex_unlock(&ct->ct_lock);
         VLOG_INFO("Attempted delete of non-existent zone limit: zone %d",
                   zone);
     }
-    ovs_mutex_unlock(&ct->ct_lock);
     return 0;
 }
 
@@ -449,7 +472,7 @@  conn_clean_cmn(struct conntrack *ct, struct conn *conn)
 
     struct zone_limit *zl = zone_limit_lookup(ct, conn->admit_zone);
     if (zl && zl->czl.zone_limit_seq == conn->zone_limit_seq) {
-        zl->czl.count--;
+        atomic_count_dec(&zl->czl.count);
     }
 }
 
@@ -503,10 +526,13 @@  conntrack_destroy(struct conntrack *ct)
     cmap_destroy(&ct->conns);
 
     struct zone_limit *zl;
-    HMAP_FOR_EACH_POP (zl, node, &ct->zone_limits) {
-        free(zl);
+    CMAP_FOR_EACH (zl, node, &ct->zone_limits) {
+        uint32_t hash = zone_key_hash(zl->czl.zone, ct->hash_basis);
+
+        cmap_remove(&ct->zone_limits, &zl->node, hash);
+        ovsrcu_postpone(free, zl);
     }
-    hmap_destroy(&ct->zone_limits);
+    cmap_destroy(&ct->zone_limits);
 
     struct timeout_policy *tp;
     HMAP_FOR_EACH_POP (tp, node, &ct->timeout_policies) {
@@ -988,7 +1014,7 @@  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
     if (commit) {
         struct zone_limit *zl = zone_limit_lookup_or_default(ct,
                                                              ctx->key.zone);
-        if (zl && zl->czl.count >= zl->czl.limit) {
+        if (zl && atomic_count_get(&zl->czl.count) >= zl->czl.limit) {
             return nc;
         }
 
@@ -1057,10 +1083,12 @@  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
         cmap_insert(&ct->conns, &nc->cm_node, ctx->hash);
         atomic_count_inc(&ct->n_conn);
         ctx->conn = nc; /* For completeness. */
+
+        zl = zone_limit_lookup_or_default(ct, ctx->key.zone);
         if (zl) {
             nc->admit_zone = zl->czl.zone;
             nc->zone_limit_seq = zl->czl.zone_limit_seq;
-            zl->czl.count++;
+            atomic_count_inc(&zl->czl.count);
         } else {
             nc->admit_zone = INVALID_ZONE;
         }
diff --git a/lib/conntrack.h b/lib/conntrack.h
index 9553b188a..58b181834 100644
--- a/lib/conntrack.h
+++ b/lib/conntrack.h
@@ -108,7 +108,7 @@  struct conntrack_dump {
 struct conntrack_zone_limit {
     int32_t zone;
     uint32_t limit;
-    uint32_t count;
+    atomic_count count;
     uint32_t zone_limit_seq; /* Used to disambiguate zone limit counts. */
 };
 
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 4381c618f..9d9767b2c 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -8193,7 +8193,8 @@  dpif_netdev_ct_get_limits(struct dpif *dpif OVS_UNUSED,
             czl = zone_limit_get(dp->conntrack, zone_limit->zone);
             if (czl.zone == zone_limit->zone || czl.zone == DEFAULT_ZONE) {
                 ct_dpif_push_zone_limit(zone_limits_reply, zone_limit->zone,
-                                        czl.limit, czl.count);
+                                        czl.limit,
+                                        atomic_count_get(&czl.count));
             } else {
                 return EINVAL;
             }
@@ -8203,7 +8204,7 @@  dpif_netdev_ct_get_limits(struct dpif *dpif OVS_UNUSED,
             czl = zone_limit_get(dp->conntrack, z);
             if (czl.zone == z) {
                 ct_dpif_push_zone_limit(zone_limits_reply, z, czl.limit,
-                                        czl.count);
+                                        atomic_count_get(&czl.count));
             }
         }
     }