Felix Huettner via dev <[email protected]> writes:
> Future patches will move more resources to a per zone scope.
> To make it easier to review we just split out the conns to a separate
> struct in here.
>
> Signed-off-by: Felix Huettner <[email protected]>
> ---
> lib/conntrack-private.h | 6 +++-
> lib/conntrack.c | 74 +++++++++++++++++++++++++++++------------
> 2 files changed, 57 insertions(+), 23 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 0141afd4a..a0aaf5bc3 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -207,9 +207,13 @@ struct conntrack_zone_limit {
> uint32_t zone_limit_seq; /* Used to disambiguate zone limit counts. */
> };
>
> +struct conntrack_zone {
> + struct cmap conns;
> +};
> +
> struct conntrack {
> struct ovs_mutex ct_lock; /* Protects the following fields. */
> - struct cmap conns[UINT16_MAX + 1];
> + struct conntrack_zone zones[UINT16_MAX + 1];
> struct rculist exp_lists[N_EXP_LISTS];
> struct cmap zone_limits;
> struct cmap timeout_policies;
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 1a3e87c1e..f313eaa15 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -246,6 +246,15 @@ conn_key_cmp(const struct conn_key *key1, const struct
> conn_key *key2)
> return 1;
> }
>
> +static struct conntrack_zone *
> +zone_lookup(struct conntrack *ct, int32_t zone)
> +{
> + if (zone < MIN_ZONE || zone > MAX_ZONE) {
> + return NULL;
> + }
I don't think we actually even need this check. At least by the time
the final patch lands, I can't find a way for most of the lookup paths
to find this, and we also sometimes completely bypass the lookup in the
series (by just using &ct->zones[idx] directly), and in many places we
just simply trust the return value. If you really want a safety valve
here (could be useful going forward) we should have a single commit that
introduces this and switches all the accessors from `&ct->zones[..]` to
this call, and have a NULL check against it.
But I actually think it could be okay to just drop this helper
completely.
> + return &ct->zones[zone];
> +}
> +
> /* Initializes the connection tracker 'ct'. The caller is responsible for
> * calling 'conntrack_destroy()', when the instance is not needed anymore */
> struct conntrack *
> @@ -267,8 +276,8 @@ conntrack_init(void)
>
> ovs_mutex_init_adaptive(&ct->ct_lock);
> ovs_mutex_lock(&ct->ct_lock);
> - for (unsigned i = 0; i < ARRAY_SIZE(ct->conns); i++) {
> - cmap_init(&ct->conns[i]);
> + for (unsigned i = 0; i < ARRAY_SIZE(ct->zones); i++) {
> + cmap_init(&ct->zones[i].conns);
> }
> for (unsigned i = 0; i < ARRAY_SIZE(ct->exp_lists); i++) {
> rculist_init(&ct->exp_lists[i]);
> @@ -563,20 +572,26 @@ static void
> conn_clean__(struct conntrack *ct, struct conn *conn)
> OVS_REQUIRES(ct->ct_lock)
> {
> + struct conntrack_zone *cz;
> + uint16_t fwd_zone;
> uint32_t hash;
>
> if (conn->alg) {
> expectation_clean(ct, &conn->key_node[CT_DIR_FWD].key);
> }
>
> + fwd_zone = conn->key_node[CT_DIR_FWD].key.zone;
> + cz = zone_lookup(ct, fwd_zone);
> +
> hash = conn_key_hash(&conn->key_node[CT_DIR_FWD].key, ct->hash_basis);
> - cmap_remove(&ct->conns[conn->key_node[CT_DIR_FWD].key.zone],
> + cmap_remove(&cz->conns,
> &conn->key_node[CT_DIR_FWD].cm_node, hash);
>
> if (conn->nat_action) {
> + ovs_assert(fwd_zone == conn->key_node[CT_DIR_REV].key.zone);
> hash = conn_key_hash(&conn->key_node[CT_DIR_REV].key,
> ct->hash_basis);
> - cmap_remove(&ct->conns[conn->key_node[CT_DIR_REV].key.zone],
> + cmap_remove(&cz->conns,
> &conn->key_node[CT_DIR_REV].cm_node, hash);
> }
>
> @@ -648,8 +663,8 @@ conntrack_destroy(struct conntrack *ct)
> }
>
> ovs_mutex_lock(&ct->ct_lock);
> - for (unsigned i = 0; i < ARRAY_SIZE(ct->conns); i++) {
> - cmap_destroy(&ct->conns[i]);
> + for (unsigned i = 0; i < ARRAY_SIZE(ct->zones); i++) {
> + cmap_destroy(&ct->zones[i].conns);
> }
> cmap_destroy(&ct->zone_limits);
> cmap_destroy(&ct->timeout_policies);
> @@ -670,7 +685,7 @@ conntrack_destroy(struct conntrack *ct)
>
>
> static bool
> -conn_key_lookup(struct conntrack *ct, const struct conn_key *key,
> +conn_key_lookup(struct conntrack_zone *cz, const struct conn_key *key,
> uint32_t hash, long long now, struct conn **conn_out,
> bool *reply)
> {
> @@ -678,7 +693,7 @@ conn_key_lookup(struct conntrack *ct, const struct
> conn_key *key,
> struct conn *conn = NULL;
> bool found = false;
>
> - CMAP_FOR_EACH_WITH_HASH (keyn, cm_node, hash, &ct->conns[key->zone]) {
> + CMAP_FOR_EACH_WITH_HASH (keyn, cm_node, hash, &cz->conns) {
> if (keyn->dir == CT_DIR_FWD) {
> conn = CONTAINER_OF(keyn, struct conn, key_node[CT_DIR_FWD]);
> } else {
> @@ -710,12 +725,21 @@ out_found:
> return found;
> }
>
> +static bool
> +conn_lookup_zone(struct conntrack *ct, struct conntrack_zone *cz,
> + const struct conn_key *key, long long now,
> + struct conn **conn_out, bool *reply)
> +{
> + uint32_t hash = conn_key_hash(key, ct->hash_basis);
> + return conn_key_lookup(cz, key, hash, now, conn_out, reply);
> +}
> +
> static bool
> conn_lookup(struct conntrack *ct, const struct conn_key *key,
> long long now, struct conn **conn_out, bool *reply)
> {
> - uint32_t hash = conn_key_hash(key, ct->hash_basis);
> - return conn_key_lookup(ct, key, hash, now, conn_out, reply);
> + struct conntrack_zone *cz = &ct->zones[key->zone];
> + return conn_lookup_zone(ct, cz, key, now, conn_out, reply);
> }
>
> static void
> @@ -1024,7 +1048,8 @@ ct_verify_helper(const char *helper, enum
> ct_alg_ctl_type ct_alg_ctl)
> }
>
> static struct conn *
> -conn_insert(struct conntrack *ct, struct dp_packet *pkt,
> +conn_insert(struct conntrack *ct, struct conntrack_zone *cz,
> + struct dp_packet *pkt,
> struct conn_lookup_ctx *ctx, long long now,
> const struct nat_action_info_t *nat_action_info,
> const char *helper, const struct alg_exp_node *alg_exp,
> @@ -1107,11 +1132,11 @@ conn_insert(struct conntrack *ct, struct dp_packet
> *pkt,
> nat_packet(pkt, nc, false, ctx->icmp_related);
> uint32_t rev_hash = conn_key_hash(&rev_key_node->key,
> ct->hash_basis);
> - cmap_insert(&ct->conns[ctx->key.zone],
> + cmap_insert(&cz->conns,
> &rev_key_node->cm_node, rev_hash);
> }
>
> - cmap_insert(&ct->conns[ctx->key.zone],
> + cmap_insert(&cz->conns,
> &fwd_key_node->cm_node, ctx->hash);
> conn_expire_push_front(ct, nc);
> atomic_count_inc(&ct->n_conn);
> @@ -1163,6 +1188,7 @@ conn_maybe_not_found(struct conntrack *ct, struct
> dp_packet *pkt,
> const char *helper, const struct alg_exp_node *alg_exp,
> enum ct_alg_ctl_type ct_alg_ctl, uint32_t tp_id)
> {
> + struct conntrack_zone *cz = zone_lookup(ct, ctx->key.zone);
> struct conn *nc = NULL;
>
> COVERAGE_INC(conntrack_maybe_not_found);
> @@ -1177,18 +1203,18 @@ conn_maybe_not_found(struct conntrack *ct, struct
> dp_packet *pkt,
> * analysis. */
> if (commit) {
> ovs_mutex_lock(&ct->ct_lock);
> - bool found = conn_lookup(ct, &ctx->key, now, NULL, NULL);
> + bool found = conn_lookup_zone(ct, cz, &ctx->key, now, NULL, NULL);
> if (!found) {
> if (!pkt_validate_and_set_new_ct_state(pkt, ctx, alg_exp)) {
> ovs_mutex_unlock(&ct->ct_lock);
> return nc;
> }
> - nc = conn_insert(ct, pkt, ctx, now, nat_action_info,
> + nc = conn_insert(ct, cz, pkt, ctx, now, nat_action_info,
> helper, alg_exp, ct_alg_ctl, tp_id);
> }
> ovs_mutex_unlock(&ct->ct_lock);
> } else {
> - bool found = conn_lookup(ct, &ctx->key, now, NULL, NULL);
> + bool found = conn_lookup_zone(ct, cz, &ctx->key, now, NULL, NULL);
> if (!found) {
> pkt_validate_and_set_new_ct_state(pkt, ctx, alg_exp);
> }
> @@ -1399,8 +1425,8 @@ initial_conn_lookup(struct conntrack *ct, struct
> conn_lookup_ctx *ctx,
> * reverse key. */
> conn_key_reverse(&ctx->key);
> }
> -
> - conn_key_lookup(ct, &ctx->key, ctx->hash, now, &ctx->conn, &ctx->reply);
> + struct conntrack_zone *cz = &ct->zones[ctx->key.zone];
> + conn_key_lookup(cz, &ctx->key, ctx->hash, now, &ctx->conn, &ctx->reply);
>
> if (natted) {
> if (OVS_LIKELY(ctx->conn)) {
> @@ -1427,6 +1453,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
> const struct nat_action_info_t *nat_action_info,
> const char *helper, uint32_t tp_id)
> {
> + ovs_assert(ctx->key.zone == zone);
> +
> /* Reset ct_state whenever entering a new zone. */
> if (pkt->md.ct_state && pkt->md.ct_zone != zone) {
> pkt->md.ct_state = 0;
> @@ -2915,7 +2943,8 @@ conntrack_dump_start(struct conntrack *ct, struct
> conntrack_dump *dump,
>
> dump->ct = ct;
> *ptot_bkts = 1; /* Need to clean up the callers. */
> - dump->cursor = cmap_cursor_start(&dump->ct->conns[dump->current_zone]);
> + dump->cursor = cmap_cursor_start(
> + &dump->ct->zones[dump->current_zone].conns);
> return 0;
> }
>
> @@ -2946,7 +2975,8 @@ conntrack_dump_next(struct conntrack_dump *dump, struct
> ct_dpif_entry *entry)
> break;
> }
> dump->current_zone++;
> - dump->cursor =
> cmap_cursor_start(&dump->ct->conns[dump->current_zone]);
> + dump->cursor = cmap_cursor_start(
> + &dump->ct->zones[dump->current_zone].conns);
> }
>
> return EOF;
> @@ -3016,7 +3046,7 @@ conntrack_flush_zone(struct conntrack *ct, const
> uint16_t zone)
> struct conn_key_node *keyn;
> struct conn *conn;
>
> - CMAP_FOR_EACH (keyn, cm_node, &ct->conns[zone]) {
> + CMAP_FOR_EACH (keyn, cm_node, &ct->zones[zone].conns) {
> if (keyn->dir != CT_DIR_FWD) {
> continue;
> }
> @@ -3034,7 +3064,7 @@ conntrack_flush(struct conntrack *ct, const uint16_t
> *zone)
> return conntrack_flush_zone(ct, *zone);
> }
>
> - for (unsigned i = 0; i < ARRAY_SIZE(ct->conns); i++) {
> + for (unsigned i = 0; i < ARRAY_SIZE(ct->zones); i++) {
> conntrack_flush_zone(ct, i);
> }
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
