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 <[email protected]> # 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/ _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
