Thanks, I have one question inline.
On Wed, Feb 17, 2021 at 8:34 AM Gaetan Rivet <gr...@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 <gr...@u256.net>
> Reviewed-by: Eli Britstein <el...@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?
> if (zl) {
> nc->admit_zone = zl->czl.zone;
> nc->zone_limit_seq = zl->czl.zone_limit_seq;
> - zl->czl.count++;
> + atomic_count_inc(&zl->czl.count);
> } else {
> nc->admit_zone = INVALID_ZONE;
> }
> diff --git a/lib/conntrack.h b/lib/conntrack.h
> index 9553b188a..58b181834 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -108,7 +108,7 @@ struct conntrack_dump {
> struct conntrack_zone_limit {
> int32_t zone;
> uint32_t limit;
> - uint32_t count;
> + atomic_count count;
> uint32_t zone_limit_seq; /* Used to disambiguate zone limit counts. */
> };
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 4381c618f..9d9767b2c 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -8193,7 +8193,8 @@ dpif_netdev_ct_get_limits(struct dpif *dpif OVS_UNUSED,
> czl = zone_limit_get(dp->conntrack, zone_limit->zone);
> if (czl.zone == zone_limit->zone || czl.zone == DEFAULT_ZONE) {
> ct_dpif_push_zone_limit(zone_limits_reply, zone_limit->zone,
> - czl.limit, czl.count);
> + czl.limit,
> + atomic_count_get(&czl.count));
> } else {
> return EINVAL;
> }
> @@ -8203,7 +8204,7 @@ dpif_netdev_ct_get_limits(struct dpif *dpif OVS_UNUSED,
> czl = zone_limit_get(dp->conntrack, z);
> if (czl.zone == z) {
> ct_dpif_push_zone_limit(zone_limits_reply, z, czl.limit,
> - czl.count);
> + atomic_count_get(&czl.count));
> }
> }
> }
> --
> 2.30.0
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
