MEDIUM: stick-tables: relax stktable_trash_oldest() to only purge what is needed

stktable_trash_oldest() does insist a lot on purging what was requested,
only limited by STKTABLE_MAX_UPDATES_AT_ONCE. This is called in two
conditions, one to allocate a new stksess, and the other one to purge
entries of a stopping process. The cost of iterating over all shards
is huge, and a shard lock is taken each time before looking up entries.

Moreover, multiple threads can end up doing the same and looking hard for
many entries to purge when only one is needed. Furthermore, all threads
start from the same shard, hence synchronize their locks. All of this
costs a lot to other operations such as access from peers.

This commit simplifies the approach by ignoring the budget, starting
from a random shard number, and using a trylock so as to be able to
give up early in case of contention. The approach chosen here consists
in trying hard to flush at least one entry, but once at least one is
evicted or at least one trylock failed, then a failure on the trylock
will result in finishing.

The function now returns a success as long as one entry was freed.

With this, tests no longer show watchdog warnings during tests, though
a few still remain when stopping the tests (which are not related to
this function but to the contention from process_table_expire()).

With this change, under high contention some entries' purge might be
postponed and the table may occasionally contain slightly more entries
than their size (though this already happens since stksess_new() first
increments ->current before decrementing it).

Measures were made on a 64-core system with 8 peers
of 16 threads each, at CPU saturation (350k req/s each doing 10
track-sc) for 10M req, with 3 different approaches:

  - this one resulted in 1500 failures to find an entry (0.015%
    size overhead), with the lowest contention and the fairest
    peers distibution.

  - leaving only after a success resulted in 229 failures (0.0029%
    size overhead) but doubled the time spent in the function (on
    the write lock precisely).

  - leaving only when both a success and a failed lock were met
    resulted in 31 failures (0.00031% overhead) but the contention
    was high enough again so that peers were not all up to date.

Considering that a saturated machine might exceed its entries by
0.015% is pretty minimal, the mechanism is kept.

This should be backported to 3.2 after a bit more testing as it
resolves some watchdog warnings and panics. It requires precedent
commit "MINOR: stick-table: permit stksess_new() to temporarily
allocate more entries" to over-allocate instead of failing in case
of contention.

diff --git a/include/haproxy/stick_table.h b/include/haproxy/stick_table.h
index e770f50889fdd60cf3414277781063a735c5cee2..c72b4a0d8736dc31ce2768aeb6e2ab5f73caa917 100644 (file)
--- a/include/haproxy/stick_table.h
+++ b/include/haproxy/stick_table.h
@@ -71,7 +71,7 @@ struct stkctr *smp_create_src_stkctr(struct session *sess, struct stream *strm,
 int stktable_compatible_sample(struct sample_expr *expr, unsigned long table_type);
 int stktable_register_data_store(int idx, const char *name, int std_type, int arg_type);
 int stktable_get_data_type(char *name);
-int stktable_trash_oldest(struct stktable *t, int to_batch);
+int stktable_trash_oldest(struct stktable *t);
 int __stksess_kill(struct stktable *t, struct stksess *ts);
 
 /************************* Composite address manipulation *********************

diff --git a/src/proxy.c b/src/proxy.c
index 3dda6265bf68187b125e49ac0854c6ee4cdd24dc..8c3bbaac2b5365af2a5c573fe9c645e52e521d16 100644 (file)
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -2190,7 +2190,6 @@ struct task *manage_proxy(struct task *t, void *context, unsigned int state)
                         * to push to a new process and
                         * we are free to flush the table.
                         */
-                       int budget;
                        int cleaned_up;
 
                        /* We purposely enforce a budget limitation since we don't want
@@ -2203,14 +2202,12 @@ struct task *manage_proxy(struct task *t, void *context, unsigned int state)
                         * Moreover, we must also anticipate the pool_gc() call which
                         * will also be much slower if there is too much work at once
                         */
-                       budget = MIN(p->table->current, (1 << 15)); /* max: 32K */
-                       cleaned_up = stktable_trash_oldest(p->table, budget);
+                       cleaned_up = stktable_trash_oldest(p->table);
                        if (cleaned_up) {
                                /* immediately release freed memory since we are stopping */
                                pool_gc(NULL);
-                               if (cleaned_up > (budget / 2)) {
-                                       /* most of the budget was used to purge entries,
-                                        * it is very likely that there are still trashable
+                               if (cleaned_up) {
+                                       /* it is very likely that there are still trashable
                                         * entries in the table, reschedule a new cleanup
                                         * attempt ASAP
                                         */

diff --git a/src/stick_table.c b/src/stick_table.c
index def13d14d79b0fce4fd90e27881d98fbd33ef657..a7129b2fd2118dae996486c6152da3c5f529afa0 100644 (file)
--- a/src/stick_table.c
+++ b/src/stick_table.c
@@ -286,12 +286,12 @@ static struct stksess *__stksess_init(struct stktable *t, struct stksess * ts)
 }
 
 /*
- * Trash oldest <to_batch> sticky sessions from table <t>
- * Returns number of trashed sticky sessions. It may actually trash less
- * than expected if finding these requires too long a search time (e.g.
- * most of them have ts->ref_cnt>0). This function locks the table.
+ * Trash up to STKTABLE_MAX_UPDATES_AT_ONCE oldest sticky sessions from table
+ * <t>. Returns non-null if it managed to release at least one entry. It will
+ * avoid waiting on a lock if it managed to release at least one object. It
+ * tries hard to limit the time spent evicting objects.
  */
-int stktable_trash_oldest(struct stktable *t, int to_batch)
+int stktable_trash_oldest(struct stktable *t)
 {
        struct stksess *ts;
        struct eb32_node *eb;
@@ -299,24 +299,34 @@ int stktable_trash_oldest(struct stktable *t, int to_batch)
        int max_per_shard;
        int done_per_shard;
        int batched = 0;
+       int to_batch;
        int updt_locked;
+       int failed_once = 0;
        int looped;
        int shard;
+       int init_shard;
 
-       shard = 0;
+       /* start from a random shard number to avoid starvation in the last ones */
+       shard = init_shard = statistical_prng_range(CONFIG_HAP_TBL_BUCKETS - 1);
 
-       if (to_batch > STKTABLE_MAX_UPDATES_AT_ONCE)
-               to_batch = STKTABLE_MAX_UPDATES_AT_ONCE;
+       to_batch = STKTABLE_MAX_UPDATES_AT_ONCE;
 
        max_search = to_batch * 2; // no more than 50% misses
        max_per_shard = (to_batch + CONFIG_HAP_TBL_BUCKETS - 1) / CONFIG_HAP_TBL_BUCKETS;
 
-       while (batched < to_batch) {
+       do {
                done_per_shard = 0;
                looped = 0;
                updt_locked = 0;
 
-               HA_RWLOCK_WRLOCK(STK_TABLE_LOCK, &t->shards[shard].sh_lock);
+               if (HA_RWLOCK_TRYWRLOCK(STK_TABLE_LOCK, &t->shards[shard].sh_lock) != 0) {
+                       if (batched)
+                               break; // no point insisting, we have or made some room
+                       if (failed_once)
+                               break; // already waited once, that's enough
+                       failed_once = 1;
+                       HA_RWLOCK_WRLOCK(STK_TABLE_LOCK, &t->shards[shard].sh_lock);
+               }
 
                eb = eb32_lookup_ge(&t->shards[shard].exps, now_ms - TIMER_LOOK_BACK);
                while (batched < to_batch && done_per_shard < max_per_shard) {
@@ -349,8 +359,12 @@ int stktable_trash_oldest(struct stktable *t, int to_batch)
 
                        if (ts->expire != ts->exp.key || HA_ATOMIC_LOAD(&ts->ref_cnt) != 0) {
                        requeue:
-                               if (!tick_isset(ts->expire))
+                               if (!tick_isset(ts->expire)) {
+                                       /* don't waste more time here it we're not alone */
+                                       if (failed_once)
+                                               break;
                                        continue;
+                               }
 
                                ts->exp.key = ts->expire;
                                eb32_insert(&t->shards[shard].exps, &ts->exp);
@@ -369,6 +383,9 @@ int stktable_trash_oldest(struct stktable *t, int to_batch)
                                if (!eb || tick_is_lt(ts->exp.key, eb->key))
                                        eb = &ts->exp;
 
+                               /* don't waste more time here it we're not alone */
+                               if (failed_once)
+                                       break;
                                continue;
                        }
 
@@ -395,6 +412,10 @@ int stktable_trash_oldest(struct stktable *t, int to_batch)
                        __stksess_free(t, ts);
                        batched++;
                        done_per_shard++;
+
+                       /* don't waste more time here it we're not alone */
+                       if (failed_once)
+                               break;
                }
 
                if (updt_locked)
@@ -402,13 +423,10 @@ int stktable_trash_oldest(struct stktable *t, int to_batch)
 
                HA_RWLOCK_WRUNLOCK(STK_TABLE_LOCK, &t->shards[shard].sh_lock);
 
-               if (max_search <= 0)
-                       break;
-
-               shard = (shard + 1) % CONFIG_HAP_TBL_BUCKETS;
-               if (!shard)
-                       break;
-       }
+               shard++;
+               if (shard >= CONFIG_HAP_TBL_BUCKETS)
+                       shard = 0;
+       } while (max_search > 0 && shard != init_shard);
 
        return batched;
 }
@@ -438,7 +456,7 @@ struct stksess *stksess_new(struct stktable *t, struct stktable_key *key)
                 * locking contention but it's not a problem in practice,
                 * these will be recovered later.
                 */
-               stktable_trash_oldest(t, (t->size >> 8) + 1);
+               stktable_trash_oldest(t);
        }
 
        ts = pool_alloc(t->pool);