[PATCH] Fix a pthreadpool race

classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
On 15.12.2017 12:54, Volker Lendecke 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 see, this is a valid concern. Let me check if I can harden the code to
guarantee that pthreadpool_prepare_pool will not end in an endless loop.

--
Regards

    Ralph Wuerthner


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
On 12/15/2017 10:57 AM, Ralph Wuerthner via samba-technical wrote:

> On 15.12.2017 12:54, Volker Lendecke 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 see, this is a valid concern. Let me check if I can harden the code
> to guarantee that pthreadpool_prepare_pool will not end in an endless
> loop.

You could add a loop counter with a limit value test to generate an
error if exceeded.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list

On 15.12.2017 17:09, jim via samba-technical wrote:

> On 12/15/2017 10:57 AM, Ralph Wuerthner via samba-technical wrote:
>> On 15.12.2017 12:54, Volker Lendecke 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 see, this is a valid concern. Let me check if I can harden the code
>> to guarantee that pthreadpool_prepare_pool will not end in an endless
>> loop.
>
> You could add a loop counter with a limit value test to generate an
> error if exceeded.

I don't think that this is the right path. You will see this error, but
pthreadpool_prepare_pool() is still stuck and cannot continue because
there are idle threads sitting on pool->condvar. You could only escape
via a process exit, but this could lead to corrupted customer files.

I already started to work on this and on Monday I should have an
improved version available.

--
Regards

    Ralph Wuerthner


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
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

fix-job-starvation-after-fork-race-v2.patch (11K) Download Attachment
12