Thanks for the review Ben On Mon, Dec 2, 2019 at 12:19 PM Ben Pfaff <[email protected]> wrote:
> On Mon, Dec 02, 2019 at 11:41:27AM -0800, Darrell Ball wrote: > > Signed-off-by: Darrell Ball <[email protected]> > > 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, > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
