Fix for ngx_unlock() and thus race conditions in AIO mechanism

[Maxim Dounin]

Hello!

On Mon, Apr 18, 2016 at 10:40:59PM +0100, Mindaugas Rasiukevicius wrote:

> Some background: enabling AIO threads can result in "stuck" connections,
> permanently waiting for handler callback (in the system these connections
> appear in a CLOSE_WAIT state).  This happens due to race condition(s) in
> the AIO thread pool code.  Since the window is tiny, it may take hundreds
> of thousands of requests on a many-core machine to trigger a single race,
> but it is present in the real world: on some systems we accumulate over
> a hundred of them a day.
> 
> Fix: add a compiler barrier to ngx_unlock(); since it is a macro, it is
> subject to the compiler reordering optimisations.  Consider the following
> fragment of the ngx_thread_pool_cycle() function:
> 
>         ngx_spinlock(&ngx_thread_pool_done_lock, 1, 2048);
>         *ngx_thread_pool_done.last = task;
>         ngx_thread_pool_done.last = &task->next;
>         ngx_unlock(&ngx_thread_pool_done_lock);
>         (void) ngx_notify(ngx_thread_pool_handler);

Thanks for catching.

Patch that follows the same logic as used in ngx_update_time(), that is, with
an explicit ngx_memory_barrier() call before ngx_unlock(), and no barrier
semantics in ngx_unlock() itself:

# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1461074302 -10800
#      Tue Apr 19 16:58:22 2016 +0300
# Node ID 2bd8671356581d351a07c871bf57676b23605109
# Parent  2fe71825c99a3e7442c2f96899379675148a83f7
Thread pools: memory barriers in thread task notifications.

The ngx_thread_pool_done object isn't volatile, and at least some
compilers assume that it is permitted to reorder modifications of
volatile and non-volatile objects.  Added appropriate ngx_memory_barrier()
calls to make sure all modifications will happen before the lock is released.

Reported by Mindaugas Rasiukevicius,
http://mailman.nginx.org/pipermail/nginx-devel/2016-April/008160.html.

diff --git a/src/core/ngx_thread_pool.c b/src/core/ngx_thread_pool.c
--- a/src/core/ngx_thread_pool.c
+++ b/src/core/ngx_thread_pool.c
@@ -345,6 +345,8 @@ ngx_thread_pool_cycle(void *data)
         *ngx_thread_pool_done.last = task;
         ngx_thread_pool_done.last = &task->next;
 
+        ngx_memory_barrier();
+
         ngx_unlock(&ngx_thread_pool_done_lock);
 
         (void) ngx_notify(ngx_thread_pool_handler);
@@ -366,6 +368,8 @@ ngx_thread_pool_handler(ngx_event_t *ev)
     ngx_thread_pool_done.first = NULL;
     ngx_thread_pool_done.last = &ngx_thread_pool_done.first;
 
+    ngx_memory_barrier();
+
     ngx_unlock(&ngx_thread_pool_done_lock);
 
     while (task) {

-- 
Maxim Dounin
http://nginx.org/