[PATCH] Fix a pthreadpool race

[Ralph Wuerthner]

On 15.12.2017 12:54, Volker Lendecke wrote:
> On Fri, Dec 15, 2017 at 12:17:07PM +0100, Ralph Wuerthner wrote:
> 
> That is of course a lot more elegant. Looking through the code I have
> one concern: The threads are kept busy throughout the fork, which is
> good. I would like to see a good argument why the while(num_idle!=0)
> loop in pthreadpool_prepare_pool deterministically terminates. It's
> probably not really supported right now, but what if a helper thread
> creates new jobs? This might keep the helper threads busy, and they
> might re-enter idle state over and over again when prepare_pool checks
> for idle threads. More parallelism is good, your patch is of course
> simpler and thus preferrable, but I would feel better if we had a
> "proof" that prepare_pool never runs into an endless loop. Keep in
> mind that every single small race we open will eventually be hit at a
> customer installation :-)

I looked at my patch again and identified a deadlock when 
pthread_cond_timedwait() in pthreadpool_server() exits with ETIMEDOUT, 
no jobs are pending and a fork is in progress. With my original patch 
the thread would exit without signaling pool->prefork_cond which would 
result in a deadlock. Please see my updated patch with a fix for this.

Other than that I checked the code and from my perspective all idle 
threads deterministically terminate:

Assume the following start condition: pthreadpool_prepare_pool() has 
been entered and we are in the while (pool->num_idle != 0) loop 
resulting in pool->prefork_cond is not NULL. I can see the following 
states for worker threads:

1) a new worker thread has been created
The new thread will enter the while(1) loop in pthreadpool_server(). If 
no new jobs are outstanding the thread will terminate immediately 
because pool->prefork_cond != NULL without calling 
pthread_cond_timedwait() and without becoming an idle thread.
If jobs are outstanding a job will be pulled from the job list and the 
job function will be called and we continue as described in state 2). 
The thread will not become an idle thread.

2) a worker threads just completed the job function
If no more jobs are outstanding and the shutdown flag is set the thread 
will terminate. If not the thread will jump back to the beginning of the 
while(1) loop. If no jobs are outstanding the thread will terminate 
immediately because pool->prefork_cond != NULL without becoming an idle 
thread. Otherwise the thread will pull a job from the job list, the job 
function will be called and we continue as described in state 2).

3) an idle thread is waiting on the conditional variable pool->condvar
The thread will wakeup. We signal pthreadpool_prepare_pool() that the 
number of idle threads has been decreased. If no more jobs are 
outstanding the thread terminates immediately. If a timeout occurred and 
no more jobs are outstanding the thread terminates immediately too.
 From now on the thread is only running if jobs are outstanding: the job 
function will be called and the thread will continue as described in 
state 2).

Overall: threads can become idle only when no fork is in progress. 
Already idle threads will wakeup and terminate if no jobs are present or 
process the next jobs.

-- 
Regards

    Ralph Wuerthner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-job-starvation-after-fork-race-v2.patch
Type: text/x-patch
Size: 11832 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20171218/46dce6a5/fix-job-starvation-after-fork-race-v2.bin>