Felix Huettner via dev <[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]>
> ---
>
> Notes:
> v4->v5: fix conntrack_clean jumping to next zone if the current one was
> only partially cleaned
>
> lib/conntrack-private.h | 11 +++++--
> lib/conntrack.c | 64 +++++++++++++++++++++++++++++------------
> 2 files changed, 53 insertions(+), 22 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 213b23fce..1a28ea680 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -211,9 +211,13 @@ struct conntrack {
> atomic_int64_t default_zone_limit;
>
> uint32_t hash_basis; /* Salt for hashing a connection key. */
> +
> + /* Background cleanup thread. */
> pthread_t clean_thread; /* Periodically cleans up connection tracker. */
> struct latch clean_thread_exit; /* To destroy the 'clean_thread'. */
> - unsigned int next_clean_zone; /* Next zone where the clean should run. */
> + uint16_t current_clean_zone; /* Current zone where the clean should run.
> */
> + /* The position in the cmap of the current zone. */
> + struct cmap_position *current_clean_position;
>
> /* Counting connections. */
> atomic_count n_conn; /* Number of connections currently tracked. */
> @@ -235,8 +239,9 @@ struct conntrack {
>
> /* Lock acquisition order:
> * 1. 'conn->lock'
> - * 2. 'ct_lock'
> - * 3. 'resources_lock'
> + * 2. 'zone_lock'
> + * 3. 'ct_lock'
> + * 4. 'resources_lock'
> */
>
> extern struct ct_l4_proto ct_proto_tcp;
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 3e38e46ec..f2adb82a3 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -529,6 +529,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,20 +1497,36 @@ conntrack_get_sweep_interval(struct conntrack *ct)
> return ms;
> }
>
> -static size_t
> +static bool
> ct_sweep_zone(struct conntrack *ct, uint16_t zone, long long now,
> - size_t *cleaned_count)
> + size_t *cleaned_count, size_t *conn_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;
> + unsigned int conn_handled = 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) {
> + *conn_count = 0;
I think we should free and NULL the current position here. Otherwise,
when the early bail happens it might be stale.
> + return true;
> + }
> +
> + 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_handled > limit) {
> + *conn_count = conn_handled;
> + return false;
> + }
> +
> if (keyn->dir != CT_DIR_FWD) {
> continue;
> }
> @@ -1515,13 +1535,16 @@ 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++;
> + conn_handled++;
> }
> - *cleaned_count = cleaned;
> - return conn_count;
> +
> + free(*current_position);
> + *current_position = NULL;
> + *conn_count = conn_handled;
> + return true;
> }
>
> /* Cleans up old connection entries from 'ct'. Returns the time
> @@ -1534,29 +1557,32 @@ conntrack_clean(struct conntrack *ct, long long now)
> unsigned int n_conn_limit, i;
> size_t clean_end, count = 0;
* count at this point is '0'
> size_t total_cleaned = 0;
> - uint16_t current_zone = ct->next_clean_zone;
> + bool zone_finished;
>
> atomic_read_relaxed(&ct->n_conn_limit, &n_conn_limit);
> clean_end = n_conn_limit / 64;
>
> for (i = 0; i < ARRAY_SIZE(ct->zones); i++) {
> + size_t zone_count = 0;
> size_t cleaned = 0;
>
> + zone_finished = ct_sweep_zone(ct, ct->current_clean_zone, now,
> &cleaned,
> + &zone_count, clean_end - count,
Given you moved the point where we accumulate vs. the point where we
check, I have a spidey sense that there *could* be an underflow here
someday. Might be better to either try pinning:
size_t clean_budget =
clean_end - count < clean_end ? clean_end - count : 0
or keep the (count > clean_end) check happening before the sweep.
> + &ct->current_clean_position);
> + total_cleaned += cleaned;
> +
> + if (zone_finished) {
> + /* This will overflow and thereby allow us to iterate through all
> + * zones. */
> + ct->current_clean_zone++;
> + }
> +
> if (count > clean_end) {
Count check happens. Without an increment anywhere I can find.
> next_wakeup = 0;
> break;
> }
> -
> - count += ct_sweep_zone(ct, current_zone, now, &cleaned);
> - total_cleaned += cleaned;
> -
> - /* This will overflow and thereby allow us to iterate through all
> - * zones. */
> - current_zone++;
With no more increment, I guess optimizer throws away the check
anyway. :)
> }
>
> - 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
