[ovs-dev] [patch v1] conntrack: Support zone limits.

[Darrell Ball]

Thanks for the review Ben

On Mon, Dec 2, 2019 at 12:19 PM Ben Pfaff <blp at ovn.org> wrote:

> On Mon, Dec 02, 2019 at 11:41:27AM -0800, Darrell Ball wrote:
> > Signed-off-by: Darrell Ball <dlu998 at gmail.com>
>
> Thanks.  I'm glad to see this code growing closer to parity with the
> kernel implementation.  The implementation also looks pretty clean.
>
> I'm appending some style suggestions.  They should not change behavior.
>

those are all fine - thanks


>
> I do have one concern about correctness.  It looks to me that there is a
> separate lookup for the zone at the time that a connection is created
> (to increment the counter) and at the time it is destroyed (to decrement
> the counter).  I believe that this can lead to inconsistencies.  For
> example, suppose that initially a connection has no associated zone, but
> at time of destruction a zone does exist.  In that case, I believe that
> a counter would get decremented that was never incremented.


Good catch !
I agree; I created a conn 'admit zone' to keep track of the zone limit zone
used at creation time.


> I think
> that there are similar potential issues related to finding a specific
> zone versus the default zone.
>

IIUC, when a zone is looked up otherwise, we specify whether we are looking
for the default zone or other zone explicitly. If I missed your meaning,
pls comment on
this aspect in V2. Thanks


>
> -8<--------------------------cut here-------------------------->8--
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 59e1c51c0389..c90da2b4e32e 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -78,9 +78,7 @@ enum ct_alg_ctl_type {
>
>  struct zone_limit {
>      struct hmap_node node;
> -    int32_t zone;
> -    uint32_t limit;
> -    uint32_t count;
> +    struct conntrack_zone_limit czl;
>

might as well use the struct created


>  };
>
>  static bool conn_key_extract(struct conntrack *, struct dp_packet *,
> @@ -339,31 +337,30 @@ zone_limit_lookup(struct conntrack *ct, int32_t zone)
>  {
>      uint32_t hash = zone_key_hash(zone, ct->hash_basis);
>      struct zone_limit *zl;
> -    HMAP_FOR_EACH_WITH_HASH (zl, node, hash, &ct->zone_limits) {
> -        if (zl->zone == zone) {
> +    HMAP_FOR_EACH_IN_BUCKET (zl, node, hash, &ct->zone_limits) {
>

sure, slightly faster


> +        if (zl->czl.zone == zone) {
>              return zl;
>          }
>      }
>      return NULL;
>  }
>
> +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);
> +}
> +
>

this is useful


>  struct conntrack_zone_limit
>  zone_limit_get(struct conntrack *ct, int32_t zone)
>  {
>      struct conntrack_zone_limit czl = {INVALID_ZONE, 0, 0};
>      ovs_mutex_lock(&ct->ct_lock);
> -    struct zone_limit *zl = zone_limit_lookup(ct, zone);
> +    struct zone_limit *zl = zone_limit_lookup_or_default(ct, zone);
>      if (zl) {
> -        czl.zone = zl->zone;
> -        czl.limit = zl->limit;
> -        czl.count = zl->count;
> -    } else {
> -        zl = zone_limit_lookup(ct, DEFAULT_ZONE);
> -        if (zl) {
> -            czl.zone = zl->zone;
> -            czl.limit = zl->limit;
> -            czl.count = zl->count;
> -        }
> +        czl = zl->czl;
>      }
>      ovs_mutex_unlock(&ct->ct_lock);
>      return czl;
> @@ -375,8 +372,8 @@ zone_limit_create(struct conntrack *ct, int32_t zone,
> uint32_t limit)
>  {
>      if (zone >= DEFAULT_ZONE && zone <= MAX_ZONE) {
>          struct zone_limit *zl = xzalloc(sizeof *zl);
> -        zl->limit = limit;
> -        zl->zone = zone;
> +        zl->czl.limit = limit;
> +        zl->czl.zone = zone;
>          uint32_t hash = zone_key_hash(zone, ct->hash_basis);
>          hmap_insert(&ct->zone_limits, &zl->node, hash);
>          return 0;
> @@ -392,7 +389,7 @@ zone_limit_update(struct conntrack *ct, int32_t zone,
> uint32_t limit)
>      ovs_mutex_lock(&ct->ct_lock);
>      struct zone_limit *zl = zone_limit_lookup(ct, zone);
>      if (zl) {
> -        zl->limit = limit;
> +        zl->czl.limit = limit;
>          VLOG_INFO("Changed zone limit of %u for zone %d", limit, zone);
>      } else {
>          err = zone_limit_create(ct, zone, limit);
> @@ -444,7 +441,7 @@ conn_clean_cmn(struct conntrack *ct, struct conn *conn)
>
>      struct zone_limit *zl = zone_limit_lookup(ct, conn->key.zone);
>      if (zl) {
> -        zl->count--;
> +        zl->czl.count--;
>      }
>  }
>
> @@ -984,16 +981,9 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
>              return nc;
>          }
>
> -        struct zone_limit *zl = zone_limit_lookup(ct, ctx->key.zone);
> -        if (zl) {
> -            if (zl->count >= zl->limit) {
> -                return nc;
> -            }
> -        } else {
> -            zl = zone_limit_lookup(ct, DEFAULT_ZONE);
> -                if (zl && zl->count >= zl->limit) {
> -                    return nc;
> -                }
> +        struct zone_limit *zl = zone_limit_lookup_or_default(ct,
> ctx->key.zone);
> +        if (zl && zl->czl.count >= zl->czl.limit) {
> +            return nc;
>          }
>
>          nc = new_conn(ct, pkt, &ctx->key, now);
> @@ -1055,7 +1045,7 @@ conn_not_found(struct conntrack *ct, struct
> dp_packet *pkt,
>          atomic_count_inc(&ct->n_conn);
>          ctx->conn = nc; /* For completeness. */
>          if (zl) {
> -            zl->count++;
> +            zl->czl.count++;
>          }
>      }
>
> diff --git a/lib/conntrack.h b/lib/conntrack.h
> index e407228054a3..71272371c7c7 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -110,7 +110,7 @@ struct conntrack_zone_limit {
>      uint32_t count;
>  };
>
> -enum zone_limits_e {
> +enum {
>      DEFAULT_ZONE = -1,
>      INVALID_ZONE = -2,
>      MIN_ZONE = 0,
>