Felix Huettner <[email protected]> writes:
> If a single zone creates a lot of churn we would have otherwise blocked
> the clean thread for quite a while.
>
> The approach with cmap_position might have the drawback that we
> potentially skip some connections that have been inserted in the
> meantime, but cmap_cursor can not be used as we have a rcu quiesce
> period in between iterations.
>
> Signed-off-by: Felix Huettner <[email protected]>
> ---
> lib/conntrack-private.h | 11 ++++++++---
> lib/conntrack.c | 38 ++++++++++++++++++++++++++++----------
> 2 files changed, 36 insertions(+), 13 deletions(-)
[...]
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index b1ac0d9ca..eb8248ed9 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -527,6 +527,10 @@ conntrack_destroy(struct conntrack *ct)
> ovs_mutex_unlock(&ct->resources_lock);
> ovs_mutex_destroy(&ct->resources_lock);
>
> + if (ct->current_clean_position) {
> + free(ct->current_clean_position);
> + }
> +
> ipf_destroy(ct->ipf);
> free(ct);
> }
> @@ -1493,18 +1497,32 @@ conntrack_get_sweep_interval(struct conntrack *ct)
>
> static size_t
> ct_sweep_zone(struct conntrack *ct, uint16_t zone, long long now,
> - size_t *cleaned_count)
> + size_t *cleaned_count, size_t limit,
> + struct cmap_position **current_position)
> OVS_NO_THREAD_SAFETY_ANALYSIS
> {
> struct conn_key_node *keyn;
> struct conntrack_zone *cz;
> unsigned int conn_count = 0;
> - unsigned int cleaned = 0;
> struct conn *conn;
> + struct cmap_node *node;
> long long expiration;
>
> cz = zone_lookup(ct, zone);
> - CMAP_FOR_EACH (keyn, cm_node, &cz->conns) {
> + if (atomic_count_get(&cz->count) == 0) {
> + return conn_count;
> + }
> +
> + if (!*current_position) {
> + *current_position = xzalloc(sizeof(**current_position));
> + }
> +
> + while ((node = cmap_next_position(&cz->conns, *current_position))) {
> + keyn = OBJECT_CONTAINING(node, keyn, cm_node);
> + if (conn_count > limit) {
I think I see an issue that the cursor position isn't being updated
properly when we do the early bail, so we'll always go to the next zone
(ct->current_clean_zone++ line) but the current_clean_position indexes
are all invalid (because they will be referencing a different zone's
cmap).
Just resetting the indexes whenever we advance the clean-zone counter is
probably good enough. It may be worth considering whether each zone
should have a cmap_position from which to continue, but it will have a
large memory footprint for a specific traffic flow case, so I'm not 100%
sure it's really worth it. In the case that we reset, though, we may
have connections that *never* expire, so it probably isn't perfect that
way either. Keeping the position details can help with that though.
Not really sure what the best approach is without a redesign of the
entire cleanup mechanism to something different.
Actually, the ordered expiration list *does* address this a bit, because
expiration is kept oldest-first, but the issue there is the cost of the
ordered list management. Maybe it wouldn't be as bad with per-zone
ordered list rather than the single global exp list.
Maybe an alternative for the cmap walking approach without exp lists
could be to have the per-zone cleanup position object, but only allocate
it when we actually have connections. That could help with some of the
memory pressure, and prevent us having to do an exp_list also.
Paolo, what are your thoughts?
> + return conn_count;
> + }
> +
> if (keyn->dir != CT_DIR_FWD) {
> continue;
> }
> @@ -1513,12 +1531,14 @@ ct_sweep_zone(struct conntrack *ct, uint16_t zone,
> long long now,
> expiration = conn_expiration(conn);
> if (now >= expiration) {
> conn_clean(ct, conn);
> - cleaned++;
> + (*cleaned_count)++;
> }
>
> conn_count++;
> }
> - *cleaned_count = cleaned;
> +
> + free(*current_position);
> + *current_position = NULL;
> return conn_count;
> }
>
> @@ -1532,7 +1552,6 @@ conntrack_clean(struct conntrack *ct, long long now)
> unsigned int n_conn_limit, i;
> size_t clean_end, count = 0;
> size_t total_cleaned = 0;
> - uint16_t current_zone = ct->next_clean_zone;
>
> atomic_read_relaxed(&ct->n_conn_limit, &n_conn_limit);
> clean_end = n_conn_limit / 64;
> @@ -1545,16 +1564,15 @@ conntrack_clean(struct conntrack *ct, long long now)
> break;
> }
>
> - count += ct_sweep_zone(ct, current_zone, now, &cleaned);
> + count += ct_sweep_zone(ct, ct->current_clean_zone, now, &cleaned,
> + clean_end - count,
> &ct->current_clean_position);
> total_cleaned += cleaned;
>
> /* This will overflow and thereby allow us to iterate through all
> * zones. */
> - current_zone++;
> + ct->current_clean_zone++;
> }
>
> - ct->next_clean_zone = current_zone + 1;
> -
> VLOG_DBG("conntrack cleaned %"PRIuSIZE" entries out of %"PRIuSIZE
> " entries in %lld msec", total_cleaned, count,
> time_msec() - now);
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
