On Mon, Dec 02, 2019 at 11:41:27AM -0800, Darrell Ball wrote: > Signed-off-by: Darrell Ball <dlu...@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. 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. I think that there are similar potential issues related to finding a specific zone versus the default zone. -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; }; 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) { + 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); +} + 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev