[trafficserver] branch master updated: Change locking for spawning dedicated threads to avoid race conditions.

[am...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

amc pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git

The following commit(s) were added to refs/heads/master by this push:
       new  ed49508   Change locking for spawning dedicated threads to avoid race conditions.
ed49508 is described below

commit ed49508a0f1fd457a5f234073b3e51ad9f4b7097
Author: Alan M. Carroll <am...@apache.org>
AuthorDate: Tue May 30 11:31:54 2017 -0500

    Change locking for spawning dedicated threads to avoid race conditions.
---
 iocore/eventsystem/I_EventProcessor.h     |  1 +
 iocore/eventsystem/P_UnixEventProcessor.h |  1 +
 iocore/eventsystem/UnixEventProcessor.cc  | 32 +++++++++++++++++++++++--------
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/iocore/eventsystem/I_EventProcessor.h b/iocore/eventsystem/I_EventProcessor.h
index 5e1d112..f86d335 100644
--- a/iocore/eventsystem/I_EventProcessor.h
+++ b/iocore/eventsystem/I_EventProcessor.h
@@ -309,6 +309,7 @@ public:
   EThread *all_dthreads[MAX_EVENT_THREADS];
   volatile int n_dthreads; // No. of dedicated threads
   volatile int thread_data_used;
+  ink_mutex dedicated_spawn_thread_mutex;
 };
 
 extern inkcoreapi class EventProcessor eventProcessor;
diff --git a/iocore/eventsystem/P_UnixEventProcessor.h b/iocore/eventsystem/P_UnixEventProcessor.h
index 5ee4867..1f18ed7 100644
--- a/iocore/eventsystem/P_UnixEventProcessor.h
+++ b/iocore/eventsystem/P_UnixEventProcessor.h
@@ -36,6 +36,7 @@ EventProcessor::EventProcessor() : n_ethreads(0), n_thread_groups(0), n_dthreads
   memset(all_dthreads, 0, sizeof(all_dthreads));
   memset(n_threads_for_type, 0, sizeof(n_threads_for_type));
   memset(next_thread_for_type, 0, sizeof(next_thread_for_type));
+  ink_mutex_init(&dedicated_spawn_thread_mutex, "EventProcessorDedicatedThreadSpawn");
 }
 
 TS_INLINE off_t
diff --git a/iocore/eventsystem/UnixEventProcessor.cc b/iocore/eventsystem/UnixEventProcessor.cc
index 2b06c84..19f3850 100644
--- a/iocore/eventsystem/UnixEventProcessor.cc
+++ b/iocore/eventsystem/UnixEventProcessor.cc
@@ -265,17 +265,33 @@ EventProcessor::shutdown()
 Event *
 EventProcessor::spawn_thread(Continuation *cont, const char *thr_name, size_t stacksize)
 {
-  ink_release_assert(n_dthreads < MAX_EVENT_THREADS);
-  int n = ink_atomic_increment((int *)&n_dthreads, 1);
-  ink_release_assert(n < MAX_EVENT_THREADS);
-
+  /* Spawning threads in a live system - There are two potential race conditions in this logic. The
+     first is multiple calls to this method.  In that case @a all_dthreads can end up in a bad state
+     as the same entry is overwritten while another is left unitialized.
+     The other is read/write contention where another thread (e.g. the stats collection thread) is
+     iterating over the threads while the active count (@a n_dthreads) is being updated causing use
+     of not yet initialized array element.
+     This logic covers both situations. For write/write the actual array update is locked. The
+     potentially expensive set up is done outside the lock making the time spent locked small For
+     read/write it suffices to do the active count increment after initializing the array
+     element. It's not a problem if, for one cycle, a new thread is skipped.
+  */
+
+  // Do as much as possible outside the lock. Until the array element and count is changed
+  // this is thread safe.
   Event *e = eventAllocator.alloc();
 
   e->init(cont, 0, 0);
-  all_dthreads[n] = new EThread(DEDICATED, e);
-  e->ethread      = all_dthreads[n];
-  e->mutex        = all_dthreads[n]->mutex;
-  cont->mutex     = all_dthreads[n]->mutex;
+  e->ethread  = new EThread(DEDICATED, e);
+  e->mutex    = e->ethread->mutex;
+  cont->mutex = e->ethread->mutex;
+  {
+    ink_mutex_acquire(&dedicated_spawn_thread_mutex);
+    ink_release_assert(n_dthreads < MAX_EVENT_THREADS);
+    all_dthreads[n_dthreads] = e->ethread;
+    ++n_dthreads; // Be very sure this is after the array element update.
+    ink_mutex_release(&dedicated_spawn_thread_mutex);
+  }
 
   e->ethread->start(thr_name, stacksize, nullptr, nullptr, nullptr);
 

-- 
To stop receiving notification emails like this one, please contact
['"commits@trafficserver.apache.org" <co...@trafficserver.apache.org>'].