From 1e928c074b68fd2b065f83896ddadcafc0a8cbed Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Tue, 28 May 2019 18:57:25 +0200 Subject: [PATCH] MEDIUM: task: don't grab the WR lock just to check the WQ When profiling locks, it appears that the WQ's lock has become the most contended one, despite the WQ being split by thread. The reason is that each thread takes the WQ lock before checking if it it does have something to do. In practice the WQ almost only contains health checks and rare tasks that can be scheduled anywhere, so this is a real waste of resources. This patch proceeds differently. Now that the WQ's lock was turned to RW lock, we proceed in 3 phases : 1) locklessly check for the queue's emptiness 2) take an R lock to retrieve the first element and check if it is expired. This way most visits are performed with an R lock to find and return the next expiration date. 3) if one expiration is found, we perform the WR-locked lookup as usual. As a result, on a one-minute test involving 8 threads and 64 streams at 1.3 million ctxsw/s, before this patch the lock profiler reported this : Stats about Lock TASK_WQ: # write lock : 1125496 # write unlock: 1125496 (0) # wait time for write : 263.143 msec # wait time for write/lock: 233.802 nsec # read lock : 0 # read unlock : 0 (0) # wait time for read : 0.000 msec # wait time for read/lock : 0.000 nsec And after : Stats about Lock TASK_WQ: # write lock : 173 # write unlock: 173 (0) # wait time for write : 0.018 msec # wait time for write/lock: 103.988 nsec # read lock : 1072706 # read unlock : 1072706 (0) # wait time for read : 60.702 msec # wait time for read/lock : 56.588 nsec Thus the contention was divided by 4.3. --- src/task.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/task.c b/src/task.c index 5845ffe04..0799d0089 100644 --- a/src/task.c +++ b/src/task.c @@ -163,6 +163,7 @@ int wake_expired_tasks() { struct task *task; struct eb32_node *eb; + __decl_hathreads(int key); int ret = TICK_ETERNITY; while (1) { @@ -210,6 +211,29 @@ int wake_expired_tasks() } #ifdef USE_THREAD + if (eb_is_empty(&timers)) + goto leave; + + HA_RWLOCK_RDLOCK(TASK_WQ_LOCK, &wq_lock); + eb = eb32_lookup_ge(&timers, now_ms - TIMER_LOOK_BACK); + if (!eb) { + eb = eb32_first(&timers); + if (likely(!eb)) { + HA_RWLOCK_RDUNLOCK(TASK_WQ_LOCK, &wq_lock); + goto leave; + } + } + key = eb->key; + HA_RWLOCK_RDUNLOCK(TASK_WQ_LOCK, &wq_lock); + + if (tick_is_lt(now_ms, key)) { + /* timer not expired yet, revisit it later */ + ret = tick_first(ret, key); + goto leave; + } + + /* There's really something of interest here, let's visit the queue */ + while (1) { HA_RWLOCK_WRLOCK(TASK_WQ_LOCK, &wq_lock); lookup_next: @@ -258,6 +282,7 @@ int wake_expired_tasks() HA_RWLOCK_WRUNLOCK(TASK_WQ_LOCK, &wq_lock); #endif +leave: return ret; }