[ovs-dev] [PATCH v1 2/9] conntrack: Use a cmap to store zone limits
Gaëtan Rivet
grive at u256.net
Wed Feb 24 00:34:31 UTC 2021
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 at 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 at u256.net>
> > Reviewed-by: Eli Britstein <elibr at 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!
--
Gaetan
More information about the dev
mailing list