Felix Huettner <[email protected]> writes:
> If a lot of conn entries expired in a single zone we previously took the
> zone_lock for each conn entry we deleted. This caused a lot of churn on
> the lock, especially if there are parallel inserts.
>
> We fix this by batching the deletions on 100 entries and taking a lock
> for all of them at once.
>
> Signed-off-by: Felix Huettner <[email protected]>
> ---
> lib/conntrack.c | 66 ++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 52 insertions(+), 14 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 12109f03b..34f6dbc3f 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -436,34 +436,34 @@ zone_limit_update(struct conntrack *ct, int32_t zone,
> uint32_t limit)
> }
>
> static void
> -conn_clean__(struct conntrack *ct, struct conn *conn)
> +conn_clean_protected__(struct conntrack *ct, struct conntrack_zone *cz,
> + struct conn *conn)
> + OVS_REQUIRES(cz->zone_lock)
> {
> - struct conntrack_zone *cz;
> - uint16_t fwd_zone;
> uint32_t hash;
>
> + COVERAGE_INC(conntrack_remove);
> +
> 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);
> - ovs_mutex_lock(&cz->zone_lock);
> cmap_remove(&cz->conns,
> &conn->key_node[CT_DIR_FWD].cm_node, hash);
> atomic_count_dec(&cz->count);
>
> if (conn->nat_action) {
> - ovs_assert(fwd_zone == conn->key_node[CT_DIR_REV].key.zone);
> + ovs_assert(conn->key_node[CT_DIR_FWD].key.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(&cz->conns,
> &conn->key_node[CT_DIR_REV].cm_node, hash);
> }
>
> - ovs_mutex_unlock(&cz->zone_lock);
> + ovsrcu_postpone(delete_conn, conn);
> + atomic_count_dec(&ct->n_conn);
> }
>
> /* Also removes the associated nat 'conn' from the lookup
> @@ -476,11 +476,27 @@ conn_clean(struct conntrack *ct, struct conn *conn)
> return;
> }
>
> - COVERAGE_INC(conntrack_remove);
> - conn_clean__(ct, conn);
> + struct conntrack_zone *cz = zone_lookup(ct,
> + conn->key_node[CT_DIR_FWD].key.zone);
>
> - ovsrcu_postpone(delete_conn, conn);
> - atomic_count_dec(&ct->n_conn);
> + ovs_mutex_lock(&cz->zone_lock);
> + conn_clean_protected__(ct, cz, conn);
> + ovs_mutex_unlock(&cz->zone_lock);
> +}
> +
> +/* Also removes the associated nat 'conn' from the lookup
> + datastructures. */
> +static void
> +conn_clean_protected(struct conntrack *ct, struct conntrack_zone *cz,
> + struct conn *conn)
> + OVS_EXCLUDED(conn->lock)
> + OVS_REQUIRES(cz->zone_lock)
> +{
> + if (atomic_flag_test_and_set(&conn->reclaimed)) {
> + return;
> + }
> +
> + conn_clean_protected__(ct, cz, conn);
> }
>
> static void
> @@ -1496,6 +1512,18 @@ conntrack_get_sweep_interval(struct conntrack *ct)
> return ms;
> }
>
> +static void
> +ct_sweep_buf(struct conntrack *ct, uint16_t zone, struct conn *conn_buf[100],
This 100 shouldn't be hardcoded. You have a define lower, but it should
be above this, I think.
> + unsigned int n_conn_buf)
> +{
> + struct conntrack_zone *cz = zone_lookup(ct, zone);
> + ovs_mutex_lock(&cz->zone_lock);
> + for (unsigned i = 0; i < n_conn_buf; i++) {
> + conn_clean_protected(ct, cz, conn_buf[i]);
> + }
> + ovs_mutex_unlock(&cz->zone_lock);
> +}
> +
> static size_t
> ct_sweep_zone(struct conntrack *ct, uint16_t zone, long long now,
> size_t *cleaned_count, size_t limit,
> @@ -1508,6 +1536,10 @@ ct_sweep_zone(struct conntrack *ct, uint16_t zone,
> long long now,
> struct conn *conn;
> long long expiration;
>
> +#define CONN_BUF_SIZE 100
> + struct conn *conn_buf[CONN_BUF_SIZE];
> + unsigned int n_conn_buf = 0;
> +
> cz = zone_lookup(ct, zone);
> if (atomic_count_get(&cz->count) == 0) {
> return conn_count;
> @@ -1533,12 +1565,18 @@ ct_sweep_zone(struct conntrack *ct, uint16_t zone,
> long long now,
> conn = CONTAINER_OF(keyn, struct conn, key_node[keyn->dir]);
> expiration = conn_expiration(conn);
There is an early return path handling that is missing here. If we are
< CONN_BUF_SIZE but for some reason hit 'limit', we will have a
conn_buf[..] array of connections, and will have incremented the
cleaned_count, but will bail out of the zone sweep early and leave them
behind.
> if (now >= expiration) {
> - conn_clean(ct, conn);
> + conn_buf[n_conn_buf++] = conn;
> (*cleaned_count)++;
> }
>
> + if (n_conn_buf == CONN_BUF_SIZE) {
> + ct_sweep_buf(ct, zone, conn_buf, n_conn_buf);
> + n_conn_buf = 0;
> + }
> +
> conn_count++;
> }
> + ct_sweep_buf(ct, zone, conn_buf, n_conn_buf);
>
> free(*current_position);
> *current_position = NULL;
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
