mirror of
http://git.haproxy.org/git/haproxy.git/
synced 2025-02-19 12:16:59 +00:00
BUG/MEDIUM: init/threads: prevent initialized threads from starting before others
Since commit 6ec902a ("MINOR: threads: serialize threads initialization") we now serialize threads initialization. But doing so has emphasized another race which is that some threads may actually start the loop before others are done initializing. As soon as all threads enter the first thread_release() call, their rdv bit is cleared and they're all waiting for all others' rdv to be cleared as well, with their harmless bit set. The first one to notice the cleared mask will progress through thread_isolate(), take rdv again preventing most others from noticing its short pass to zero, and this first one will be able to run all the way through the initialization till the last call to thread_release() which it happily crosses, being the only one with the rdv bit, leaving the room for one or a few others to do the same. This results in some threads entering the loop before others are done with their initialization, which is particularly bad. PiBa-NL reported that some regtests fail for him due to this (which was impossible to reproduce here, but races are racy by definition). However placing some printf() in the initialization code definitely shows this unsychronized startup. This patch takes a different approach in three steps : - first, we don't start with thread_release() anymore and we don't set the rdv mask anymore in the main call. This was initially done to let all threads start toghether, which we don't want. Instead we just start with thread_isolate(). Since all threads are harmful by default, they all wait for each other's readiness before starting. - second, we don't release with thread_release() but with thread_sync_release(), meaning that we don't leave the function until other ones have reached the point in the function where they decide to leave it as well. - third, it makes sure we don't start the listeners using protocol_enable_all() before all threads have allocated their local FD tables or have initialized their pollers, otherwise startup could be racy as well. It's worth noting that it is even possible to limit this call to thread #0 as it only needs to be performed once. This now guarantees that all thread init calls start only after all threads are ready, and that no thread enters the polling loop before all others have completed their initialization. Please check GH issues #111 and #117 for more context. No backport is needed, though if some new init races are reported in 1.9 (or even 1.8) which do not affect 2.0, then it may make sense to carefully backport this small series.
This commit is contained in:
parent
9a1f57351d
commit
7109282577
@ -2572,9 +2572,6 @@ static void *run_thread_poll_loop(void *data)
|
||||
ti->clock_id = CLOCK_THREAD_CPUTIME_ID;
|
||||
#endif
|
||||
#endif
|
||||
/* broadcast that we are ready and wait for other threads to start */
|
||||
thread_release();
|
||||
|
||||
/* Now, initialize one thread init at a time. This is better since
|
||||
* some init code is a bit tricky and may release global resources
|
||||
* after reallocating them locally. This will also ensure there is
|
||||
@ -2607,10 +2604,17 @@ static void *run_thread_poll_loop(void *data)
|
||||
}
|
||||
}
|
||||
|
||||
/* enabling protocols will result in fd_insert() calls to be performed,
|
||||
* we want all threads to have already allocated their local fd tables
|
||||
* before doing so.
|
||||
*/
|
||||
thread_sync_release();
|
||||
thread_isolate();
|
||||
|
||||
protocol_enable_all();
|
||||
|
||||
/* done initializing this thread, wait for others */
|
||||
thread_release();
|
||||
/* done initializing this thread, don't start before others are done */
|
||||
thread_sync_release();
|
||||
|
||||
run_poll_loop();
|
||||
|
||||
@ -3248,12 +3252,6 @@ int main(int argc, char **argv)
|
||||
sigdelset(&blocked_sig, SIGSEGV);
|
||||
pthread_sigmask(SIG_SETMASK, &blocked_sig, &old_sig);
|
||||
|
||||
/* mark the fact that threads must wait for each other
|
||||
* during startup. Once initialized, they just have to
|
||||
* call thread_release().
|
||||
*/
|
||||
threads_want_rdv_mask = all_threads_mask;
|
||||
|
||||
/* Create nbthread-1 thread. The first thread is the current process */
|
||||
thread_info[0].pthread = pthread_self();
|
||||
for (i = 1; i < global.nbthread; i++)
|
||||
|
Loading…
Reference in New Issue
Block a user