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.
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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
