From 1b32790324ebf4b83fafe2d01ca440c1a93198ba Mon Sep 17 00:00:00 2001 From: Olivier Houchard Date: Fri, 15 Mar 2019 00:23:10 +0100 Subject: [PATCH] BUG/MEDIUM: tasks: Make sure we wake sleeping threads if needed. When waking a task on a remote thread, we currently check 1) if this thread was sleeping, and 2) if it was already marked as active before writing to its pipe. Unfortunately this doesn't always work as desired because only one thread from the mask is woken up, while the active_tasks_mask indicates all eligible threads for this task. As a result, if one multi-thread task (e.g. a health check) wakes up to run on any thread, then an accept() dispatches an incoming connection on thread 2, this thread will already have its bit set in active_tasks_mask because of the previous wakeup and will not be woken up. This is easily noticeable on 2.0-dev by injecting on a multi-threaded listener with a single connection at a time while health checks are running quickly in the background : the injection runs slowly with random response times (the poll timeouts). In 1.9 it affects the dequeing of server connections, which occasionally experience pauses if multiple threads share the same queue. The correct solution consists in adjusting the sleeping_thread_mask when waking another thread up. This mask reflects threads that are sleeping, hence that need to be signaled to wake up. Threads with a bit in active_tasks_mask already don't have their sleeping_thread_mask bit set before polling so the principle remains consistent. And by doing so we can remove the old_active_mask field. This should be backported to 1.9. --- src/task.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/task.c b/src/task.c index a3765139d..637e23551 100644 --- a/src/task.c +++ b/src/task.c @@ -69,7 +69,6 @@ void __task_wakeup(struct task *t, struct eb_root *root) { void *expected = NULL; int *rq_size; - unsigned long __maybe_unused old_active_mask; #ifdef USE_THREAD if (root == &rqueue) { @@ -125,7 +124,6 @@ redo: __ha_barrier_atomic_store(); } #endif - old_active_mask = active_tasks_mask; _HA_ATOMIC_OR(&active_tasks_mask, t->thread_mask); t->rq.key = _HA_ATOMIC_ADD(&rqueue_ticks, 1); @@ -160,9 +158,13 @@ redo: * wake one. */ if ((((t->thread_mask & all_threads_mask) & sleeping_thread_mask) == - (t->thread_mask & all_threads_mask)) && - !(t->thread_mask & old_active_mask)) - wake_thread(my_ffsl((t->thread_mask & all_threads_mask) &~ tid_bit) - 1); + (t->thread_mask & all_threads_mask))) { + unsigned long m = (t->thread_mask & all_threads_mask) &~ tid_bit; + + m = (m & (m - 1)) ^ m; // keep lowest bit set + _HA_ATOMIC_AND(&sleeping_thread_mask, ~m); + wake_thread(my_ffsl(m) - 1); + } #endif return; }