[PATCH] Fix a pthreadpool race

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

[PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
Hi!

This survives pthreadpooltest, it has not seen a full autobuild yet.
I'd just like to post it for thorough review. It has gone through
quite a few internal discussions back and forth bouncing ideas about
different solutions. If someone has a simpler solution that survives
the test in the second patch, I'm more than happy to review it.

Christof, looking at your patches now :-)

Review appreciated!

Thanks, Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

patch.txt (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
On Wed, Dec 06, 2017 at 05:53:57PM +0100, Volker Lendecke wrote:

> Hi!
>
> This survives pthreadpooltest, it has not seen a full autobuild yet.
> I'd just like to post it for thorough review. It has gone through
> quite a few internal discussions back and forth bouncing ideas about
> different solutions. If someone has a simpler solution that survives
> the test in the second patch, I'm more than happy to review it.
>
> Christof, looking at your patches now :-)
>
> Review appreciated!
Sorry, this missed a prerequisite patch. Attached.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

patch.txt (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
On Wed, Dec 06, 2017 at 06:56:56PM +0100, Volker Lendecke via samba-technical wrote:

> On Wed, Dec 06, 2017 at 05:53:57PM +0100, Volker Lendecke wrote:
> > Hi!
> >
> > This survives pthreadpooltest, it has not seen a full autobuild yet.
> > I'd just like to post it for thorough review. It has gone through
> > quite a few internal discussions back and forth bouncing ideas about
> > different solutions. If someone has a simpler solution that survives
> > the test in the second patch, I'm more than happy to review it.
> >
> > Christof, looking at your patches now :-)
> >
> > Review appreciated!
>
> Sorry, this missed a prerequisite patch. Attached.


OK - reviewing. First, understanding the problem the
test reproduces:

add a job (num_jobs = 1) -> creates thread to run it.
job finishes, thread sticks around (num_idle = 1).
num_jobs is now zero (initil job finished).

a) Idle thread is now waiting on pool->condvar inside
pthreadpool_server() in pthread_cond_timedwait().

Now, add another job ->

        pthreadpool_add_job()
                -> pthreadpool_put_job()
                        This adds the job to the queue.
                Oh, there is an idle thread so don't
                create one, do:

                pthread_cond_signal(&pool->condvar);

                and return.

Now call fork *before* idle thread in (a) wakes from
the signaling of pool->condvar.

In the parent (child is irrelevent):

Go into: pthreadpool_prepare() ->
                pthreadpool_prepare_pool()

                Set the variable to tell idle threads to exit:

                pool->prefork_cond = &prefork_cond;

                then wake them up with:

                pthread_cond_signal(&pool->condvar);

                This does nothing as the idle thread
                is already awoken.

b) Idle thread wakes up and does:

                Reduce idle thread count (num_idle = 0)

                pool->num_idle -= 1;

                Check if we're in the middle of a fork.

                if (pool->prefork_cond != NULL) {

                        Yes we are, tell pthreadpool_prepare()
                        we are exiting.

                        pthread_cond_signal(pool->prefork_cond);

                        And exit.

                        pthreadpool_server_exit(pool);
                        return NULL;
                }

So we come back from the fork in the parent with num_jobs = 1,
a job on the queue but no idle threads - and the code that
creates a new thread on job submission was skipped because
an idle thread existed at point (a).

Is that a correct explaination of the test ? If so I'll go
on to review the fix :-) :-).

Cheers,

        Jeremy.


> --
> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: +49-551-370000-0, fax: +49-551-370000-9
> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
> http://www.sernet.de, mailto:[hidden email]

> From 20b2b0a926a2119c77cc25127d9f6c6c683d4b13 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <[hidden email]>
> Date: Tue, 5 Dec 2017 15:34:09 +0100
> Subject: [PATCH 1/3] lib: Don't use PTHREAD_COND_INITIALIZER for a stack
>  variable
>
> susv4 speaks about statically allocated variables. Stack vars are not.
>
> Signed-off-by: Volker Lendecke <[hidden email]>
> ---
>  lib/pthreadpool/pthreadpool.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lib/pthreadpool/pthreadpool.c b/lib/pthreadpool/pthreadpool.c
> index 23885aa6d11..646708adb62 100644
> --- a/lib/pthreadpool/pthreadpool.c
> +++ b/lib/pthreadpool/pthreadpool.c
> @@ -179,9 +179,12 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult,
>  
>  static void pthreadpool_prepare_pool(struct pthreadpool *pool)
>  {
> - pthread_cond_t prefork_cond = PTHREAD_COND_INITIALIZER;
> + pthread_cond_t prefork_cond;
>   int ret;
>  
> + ret = pthread_cond_init(&prefork_cond, NULL);
> + assert(ret == 0);
> +
>   ret = pthread_mutex_lock(&pool->mutex);
>   assert(ret == 0);
>  
> --
> 2.11.0
>
>
> From 03934e534d190542e91184ab47a591f68772c6ea Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <[hidden email]>
> Date: Wed, 29 Nov 2017 16:45:40 +0100
> Subject: [PATCH 2/3] pthreadpool: Fix starvation after fork
>
> After the race is before the race:
>
> 1) Create an idle thread
> 2) Add a job: This won't create a thread anymore
> 3) Immediately fork
>
> The idle thread will be woken twice before it's actually woken up: Both
> pthreadpool_add_job and pthreadpool_prepare_pool call cond_signal, for
> different reasons. We must look at pool->prefork_cond first because otherwise
> we will end up in a blocking job deep within a fork call, the helper thread
> must take its fingers off the condvar as quickly as possible.  This means that
> after the fork there's no idle thread around anymore that would pick up the job
> submitted in 2). So we must keep the idle threads around across the fork.
>
> The quick solution to re-create one helper thread in pthreadpool_parent has a
> fatal flaw: What do we do if that pthread_create call fails? We're deep in an
> application calling fork(), and doing fancy signalling from there is really
> something we must avoid.
>
> This has one potential performance issue: If we have hundreds of idle threads
> (do we ever have that) during the fork, the call to pthread_mutex_lock on the
> fork_mutex from pthreadpool_server (the helper thread) will probably cause a
> thundering herd when the _parent call unlocks the fork_mutex. The solution for
> this to just keep one idle thread around. But this adds code that is not
> strictly required functionally for now.
>
> Signed-off-by: Volker Lendecke <[hidden email]>
> ---
>  lib/pthreadpool/pthreadpool.c | 92 ++++++++++++++++++++++++++++++++++---------
>  1 file changed, 73 insertions(+), 19 deletions(-)
>
> diff --git a/lib/pthreadpool/pthreadpool.c b/lib/pthreadpool/pthreadpool.c
> index 646708adb62..0bf7109a8f8 100644
> --- a/lib/pthreadpool/pthreadpool.c
> +++ b/lib/pthreadpool/pthreadpool.c
> @@ -91,11 +91,19 @@ struct pthreadpool {
>   int num_idle;
>  
>   /*
> - * Condition variable indicating that we should quickly go
> - * away making way for fork() without anybody waiting on
> - * pool->condvar.
> + * Condition variable indicating that helper threads should
> + * quickly go away making way for fork() without anybody
> + * waiting on pool->condvar.
>   */
>   pthread_cond_t *prefork_cond;
> +
> + /*
> + * Waiting position for helper threads while fork is
> + * running. The forking thread will have locked it, and all
> + * idle helper threads will sit here until after the fork,
> + * where the forking thread will unlock it again.
> + */
> + pthread_mutex_t fork_mutex;
>  };
>  
>  static pthread_mutex_t pthreadpools_mutex = PTHREAD_MUTEX_INITIALIZER;
> @@ -151,6 +159,15 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult,
>   return ret;
>   }
>  
> + ret = pthread_mutex_init(&pool->fork_mutex, NULL);
> + if (ret != 0) {
> + pthread_cond_destroy(&pool->condvar);
> + pthread_mutex_destroy(&pool->mutex);
> + free(pool->jobs);
> + free(pool);
> + return ret;
> + }
> +
>   pool->shutdown = false;
>   pool->num_threads = 0;
>   pool->max_threads = max_threads;
> @@ -159,6 +176,7 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult,
>  
>   ret = pthread_mutex_lock(&pthreadpools_mutex);
>   if (ret != 0) {
> + pthread_mutex_destroy(&pool->fork_mutex);
>   pthread_cond_destroy(&pool->condvar);
>   pthread_mutex_destroy(&pool->mutex);
>   free(pool->jobs);
> @@ -179,21 +197,26 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult,
>  
>  static void pthreadpool_prepare_pool(struct pthreadpool *pool)
>  {
> - pthread_cond_t prefork_cond;
>   int ret;
>  
> - ret = pthread_cond_init(&prefork_cond, NULL);
> + ret = pthread_mutex_lock(&pool->fork_mutex);
>   assert(ret == 0);
>  
>   ret = pthread_mutex_lock(&pool->mutex);
>   assert(ret == 0);
>  
>   while (pool->num_idle != 0) {
> + int num_idle = pool->num_idle;
> + pthread_cond_t prefork_cond;
> +
> + ret = pthread_cond_init(&prefork_cond, NULL);
> + assert(ret == 0);
> +
>   /*
> - * Exit all idle threads, which are all blocked in
> - * pool->condvar. In the child we can destroy the
> - * pool, which would result in undefined behaviour in
> - * the pthread_cond_destroy(pool->condvar). glibc just
> + * Push all idle threads off pool->condvar. In the
> + * child we can destroy the pool, which would result
> + * in undefined behaviour in the
> + * pthread_cond_destroy(pool->condvar). glibc just
>   * blocks here.
>   */
>   pool->prefork_cond = &prefork_cond;
> @@ -201,14 +224,16 @@ static void pthreadpool_prepare_pool(struct pthreadpool *pool)
>   ret = pthread_cond_signal(&pool->condvar);
>   assert(ret == 0);
>  
> - ret = pthread_cond_wait(&prefork_cond, &pool->mutex);
> - assert(ret == 0);
> + while (pool->num_idle == num_idle) {
> + ret = pthread_cond_wait(&prefork_cond, &pool->mutex);
> + assert(ret == 0);
> + }
>  
>   pool->prefork_cond = NULL;
> - }
>  
> - ret = pthread_cond_destroy(&prefork_cond);
> - assert(ret == 0);
> + ret = pthread_cond_destroy(&prefork_cond);
> + assert(ret == 0);
> + }
>  
>   /*
>   * Probably it's well-defined somewhere: What happens to
> @@ -249,6 +274,8 @@ static void pthreadpool_parent(void)
>   assert(ret == 0);
>   ret = pthread_mutex_unlock(&pool->mutex);
>   assert(ret == 0);
> + ret = pthread_mutex_unlock(&pool->fork_mutex);
> + assert(ret == 0);
>   }
>  
>   ret = pthread_mutex_unlock(&pthreadpools_mutex);
> @@ -271,8 +298,12 @@ static void pthreadpool_child(void)
>  
>   ret = pthread_cond_init(&pool->condvar, NULL);
>   assert(ret == 0);
> +
>   ret = pthread_mutex_unlock(&pool->mutex);
>   assert(ret == 0);
> +
> + ret = pthread_mutex_unlock(&pool->fork_mutex);
> + assert(ret == 0);
>   }
>  
>   ret = pthread_mutex_unlock(&pthreadpools_mutex);
> @@ -287,7 +318,7 @@ static void pthreadpool_prep_atfork(void)
>  
>  static int pthreadpool_free(struct pthreadpool *pool)
>  {
> - int ret, ret1;
> + int ret, ret1, ret2;
>  
>   ret = pthread_mutex_lock(&pthreadpools_mutex);
>   if (ret != 0) {
> @@ -299,6 +330,7 @@ static int pthreadpool_free(struct pthreadpool *pool)
>  
>   ret = pthread_mutex_destroy(&pool->mutex);
>   ret1 = pthread_cond_destroy(&pool->condvar);
> + ret2 = pthread_mutex_destroy(&pool->fork_mutex);
>  
>   if (ret != 0) {
>   return ret;
> @@ -306,6 +338,9 @@ static int pthreadpool_free(struct pthreadpool *pool)
>   if (ret1 != 0) {
>   return ret1;
>   }
> + if (ret2 != 0) {
> + return ret2;
> + }
>  
>   free(pool->jobs);
>   free(pool);
> @@ -465,11 +500,30 @@ static void *pthreadpool_server(void *arg)
>   /*
>   * Me must allow fork() to continue
>   * without anybody waiting on
> - * &pool->condvar.
> + * &pool->condvar. Tell
> + * pthreadpool_prepare_pool that we
> + * got that message.
>   */
> - pthread_cond_signal(pool->prefork_cond);
> - pthreadpool_server_exit(pool);
> - return NULL;
> +
> + res = pthread_cond_signal(pool->prefork_cond);
> + assert(res == 0);
> +
> + res = pthread_mutex_unlock(&pool->mutex);
> + assert(res == 0);
> +
> + /*
> + * pthreadpool_prepare_pool has
> + * already locked this mutex across
> + * the fork. This makes us wait
> + * without sitting in a condvar.
> + */
> + res = pthread_mutex_lock(&pool->fork_mutex);
> + assert(res == 0);
> + res = pthread_mutex_unlock(&pool->fork_mutex);
> + assert(res == 0);
> +
> + res = pthread_mutex_lock(&pool->mutex);
> + assert(res == 0);
>   }
>  
>   if (res == ETIMEDOUT) {
> --
> 2.11.0
>
>
> From b4d0c5ae20bce5849470d1acf2e567e6824685c9 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <[hidden email]>
> Date: Wed, 29 Nov 2017 18:55:21 +0100
> Subject: [PATCH 3/3] pthreadpool: Add a test for the race condition fixed in
>  the last commit
>
> Signed-off-by: Volker Lendecke <[hidden email]>
> ---
>  lib/pthreadpool/tests.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
>
> diff --git a/lib/pthreadpool/tests.c b/lib/pthreadpool/tests.c
> index 1aab80c2bb4..f0ae0aa4a93 100644
> --- a/lib/pthreadpool/tests.c
> +++ b/lib/pthreadpool/tests.c
> @@ -308,6 +308,82 @@ static int test_busyfork(void)
>   return 0;
>  }
>  
> +static int test_busyfork2(void)
> +{
> + struct pthreadpool_pipe *p;
> + pid_t child;
> + int ret, jobnum;
> + struct pollfd pfd;
> +
> + ret = pthreadpool_pipe_init(1, &p);
> + if (ret != 0) {
> + fprintf(stderr, "pthreadpool_pipe_init failed: %s\n",
> + strerror(ret));
> + return -1;
> + }
> +
> + ret = pthreadpool_pipe_add_job(p, 1, busyfork_job, NULL);
> + if (ret != 0) {
> + fprintf(stderr, "pthreadpool_add_job failed: %s\n",
> + strerror(ret));
> + return -1;
> + }
> +
> + ret = pthreadpool_pipe_finished_jobs(p, &jobnum, 1);
> + if (ret != 1) {
> + fprintf(stderr, "pthreadpool_pipe_finished_jobs failed\n");
> + return -1;
> + }
> +
> + ret = poll(NULL, 0, 10);
> + if (ret == -1) {
> + perror("poll failed");
> + return -1;
> + }
> +
> + ret = pthreadpool_pipe_add_job(p, 1, busyfork_job, NULL);
> + if (ret != 0) {
> + fprintf(stderr, "pthreadpool_add_job failed: %s\n",
> + strerror(ret));
> + return -1;
> + }
> +
> + /*
> + * Do the fork right after the add_job. This tests a race
> + * where the atfork prepare handler gets all idle threads off
> + * the condvar. If we are faster doing the fork than the
> + * existing idle thread could get out of idle and take the
> + * job, after the fork we end up with no threads to take care
> + * of the job.
> + */
> +
> + child = fork();
> + if (child < 0) {
> + perror("fork failed");
> + return -1;
> + }
> +
> + if (child == 0) {
> + exit(0);
> + }
> +
> + pfd = (struct pollfd) {
> + .fd = pthreadpool_pipe_signal_fd(p),
> + .events = POLLIN|POLLERR
> + };
> +
> + do {
> + ret = poll(&pfd, 1, 5000);
> + } while ((ret == -1) && (errno == EINTR));
> +
> + if (ret == 0) {
> + fprintf(stderr, "job unfinished after 5 seconds\n");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
>  static void test_tevent_wait(void *private_data)
>  {
>   int *timeout = private_data;
> @@ -423,6 +499,12 @@ int main(void)
>   return 1;
>   }
>  
> + ret = test_busyfork2();
> + if (ret != 0) {
> + fprintf(stderr, "test_busyfork2 failed\n");
> + return 1;
> + }
> +
>   printf("success\n");
>   return 0;
>  }
> --
> 2.11.0
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
On Wed, Dec 06, 2017 at 02:28:06PM -0800, Jeremy Allison wrote:

> On Wed, Dec 06, 2017 at 06:56:56PM +0100, Volker Lendecke via samba-technical wrote:
> > On Wed, Dec 06, 2017 at 05:53:57PM +0100, Volker Lendecke wrote:
> > > Hi!
> > >
> > > This survives pthreadpooltest, it has not seen a full autobuild yet.
> > > I'd just like to post it for thorough review. It has gone through
> > > quite a few internal discussions back and forth bouncing ideas about
> > > different solutions. If someone has a simpler solution that survives
> > > the test in the second patch, I'm more than happy to review it.
> > >
> > > Christof, looking at your patches now :-)
> > >
> > > Review appreciated!
> >
> > Sorry, this missed a prerequisite patch. Attached.
>
>
> OK - reviewing. First, understanding the problem the
> test reproduces:
>
> add a job (num_jobs = 1) -> creates thread to run it.
> job finishes, thread sticks around (num_idle = 1).
> num_jobs is now zero (initil job finished).
>
> a) Idle thread is now waiting on pool->condvar inside
> pthreadpool_server() in pthread_cond_timedwait().
>
> Now, add another job ->
>
> pthreadpool_add_job()
> -> pthreadpool_put_job()
> This adds the job to the queue.
> Oh, there is an idle thread so don't
> create one, do:
>
> pthread_cond_signal(&pool->condvar);
>
> and return.
>
> Now call fork *before* idle thread in (a) wakes from
> the signaling of pool->condvar.
>
> In the parent (child is irrelevent):
>
> Go into: pthreadpool_prepare() ->
> pthreadpool_prepare_pool()
>
> Set the variable to tell idle threads to exit:
>
> pool->prefork_cond = &prefork_cond;
>
> then wake them up with:
>
> pthread_cond_signal(&pool->condvar);
>
> This does nothing as the idle thread
> is already awoken.
>
> b) Idle thread wakes up and does:
>
> Reduce idle thread count (num_idle = 0)
>
> pool->num_idle -= 1;
>
> Check if we're in the middle of a fork.
>
> if (pool->prefork_cond != NULL) {
>
> Yes we are, tell pthreadpool_prepare()
> we are exiting.
>
> pthread_cond_signal(pool->prefork_cond);
>
> And exit.
>
> pthreadpool_server_exit(pool);
> return NULL;
> }
>
> So we come back from the fork in the parent with num_jobs = 1,
> a job on the queue but no idle threads - and the code that
> creates a new thread on job submission was skipped because
> an idle thread existed at point (a).
>
> Is that a correct explaination of the test ? If so I'll go
> on to review the fix :-) :-).

OK, assuming that the previous explaination is correct, the
fix is to create a new pthreadpool context mutex:

pool->fork_mutex

and in pthreadpool_server(), when an idle thread wakes up and
notices we're in the prepare fork state, it puts itself to
sleep by waiting on the new pool->fork_mutex.

And in pthreadpool_prepare_pool(), instead of waiting for
the idle threads to exit, hold the pool->fork_mutex and
signal each idle thread in turn, and wait for the pool->num_idle
to go to zero - which means they're all blocked waiting on
pool->fork_mutex.

When the parent continues, pthreadpool_parent()
unlocks the pool->fork_mutex and all the previously
'idle' threads wake up (and you mention the thundering
herd problem, which is as you say vanishingly small :-)
and pick up any remaining job.

Nice work. Looks elegant to me and I can't see any
other way of fixing it rather than the create pthread
in pthreadpool_parent() call, which you ruled out
for the lack of ability to return any error.

Cool ! Reviewed-by: Jeremy Allison <[hidden email]>

Feel free to push with any of the
text of this or the previous message in the
commit message if you think it will help others :-).

Cheers,

        Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
On Wed, Dec 06, 2017 at 04:53:15PM -0800, Jeremy Allison wrote:
> Feel free to push with any of the
> text of this or the previous message in the
> commit message if you think it will help others :-).

Attached patch has the very nice explanation in the commit message. It
also removes the necessity for the preliminary patch. Runs in a
private autobuild now. I'll push once this survived and nobody objects
in the meantime.

Thanks!

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

patch.txt (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
On Thu, Dec 07, 2017 at 07:59:35AM +0100, Volker Lendecke wrote:

> On Wed, Dec 06, 2017 at 04:53:15PM -0800, Jeremy Allison wrote:
> > Feel free to push with any of the
> > text of this or the previous message in the
> > commit message if you think it will help others :-).
>
> Attached patch has the very nice explanation in the commit message. It
> also removes the necessity for the preliminary patch. Runs in a
> private autobuild now. I'll push once this survived and nobody objects
> in the meantime.
>
> Thanks!

Thanks for adding the extra text Volker, it makes it much
easier for people who don't understand MT-code (looking at
myself in the mirror here :-) to understand the commit
changes. Please push !

Jeremy.

> --
> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: +49-551-370000-0, fax: +49-551-370000-9
> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
> http://www.sernet.de, mailto:[hidden email]

> From 287c0232d2e7ef4d7c6ce278e31606f2b5a470e4 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <[hidden email]>
> Date: Wed, 29 Nov 2017 16:45:40 +0100
> Subject: [PATCH 1/2] pthreadpool: Fix starvation after fork
>
> After the race is before the race:
>
> 1) Create an idle thread
> 2) Add a job: This won't create a thread anymore
> 3) Immediately fork
>
> The idle thread will be woken twice before it's actually woken up: Both
> pthreadpool_add_job and pthreadpool_prepare_pool call cond_signal, for
> different reasons. We must look at pool->prefork_cond first because otherwise
> we will end up in a blocking job deep within a fork call, the helper thread
> must take its fingers off the condvar as quickly as possible.  This means that
> after the fork there's no idle thread around anymore that would pick up the job
> submitted in 2). So we must keep the idle threads around across the fork.
>
> The quick solution to re-create one helper thread in pthreadpool_parent has a
> fatal flaw: What do we do if that pthread_create call fails? We're deep in an
> application calling fork(), and doing fancy signalling from there is really
> something we must avoid.
>
> This has one potential performance issue: If we have hundreds of idle threads
> (do we ever have that) during the fork, the call to pthread_mutex_lock on the
> fork_mutex from pthreadpool_server (the helper thread) will probably cause a
> thundering herd when the _parent call unlocks the fork_mutex. The solution for
> this to just keep one idle thread around. But this adds code that is not
> strictly required functionally for now.
>
> More detailed explanation from Jeremy:
>
> First, understanding the problem the test reproduces:
>
> add a job (num_jobs = 1) -> creates thread to run it.
> job finishes, thread sticks around (num_idle = 1).
> num_jobs is now zero (initial job finished).
>
> a) Idle thread is now waiting on pool->condvar inside
> pthreadpool_server() in pthread_cond_timedwait().
>
> Now, add another job ->
>
> pthreadpool_add_job()
> -> pthreadpool_put_job()
> This adds the job to the queue.
> Oh, there is an idle thread so don't
> create one, do:
>
> pthread_cond_signal(&pool->condvar);
>
> and return.
>
> Now call fork *before* idle thread in (a) wakes from
> the signaling of pool->condvar.
>
> In the parent (child is irrelevent):
>
> Go into: pthreadpool_prepare() ->
> pthreadpool_prepare_pool()
>
> Set the variable to tell idle threads to exit:
>
> pool->prefork_cond = &prefork_cond;
>
> then wake them up with:
>
> pthread_cond_signal(&pool->condvar);
>
> This does nothing as the idle thread
> is already awoken.
>
> b) Idle thread wakes up and does:
>
> Reduce idle thread count (num_idle = 0)
>
> pool->num_idle -= 1;
>
> Check if we're in the middle of a fork.
>
> if (pool->prefork_cond != NULL) {
>
> Yes we are, tell pthreadpool_prepare()
> we are exiting.
>
> pthread_cond_signal(pool->prefork_cond);
>
> And exit.
>
> pthreadpool_server_exit(pool);
> return NULL;
> }
>
> So we come back from the fork in the parent with num_jobs = 1,
> a job on the queue but no idle threads - and the code that
> creates a new thread on job submission was skipped because
> an idle thread existed at point (a).
>
> OK, assuming that the previous explaination is correct, the
> fix is to create a new pthreadpool context mutex:
>
> pool->fork_mutex
>
> and in pthreadpool_server(), when an idle thread wakes up and
> notices we're in the prepare fork state, it puts itself to
> sleep by waiting on the new pool->fork_mutex.
>
> And in pthreadpool_prepare_pool(), instead of waiting for
> the idle threads to exit, hold the pool->fork_mutex and
> signal each idle thread in turn, and wait for the pool->num_idle
> to go to zero - which means they're all blocked waiting on
> pool->fork_mutex.
>
> When the parent continues, pthreadpool_parent()
> unlocks the pool->fork_mutex and all the previously
> 'idle' threads wake up (and you mention the thundering
> herd problem, which is as you say vanishingly small :-)
> and pick up any remaining job.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13179
> Signed-off-by: Volker Lendecke <[hidden email]>
> Reviewed-by: Jeremy Allison <[hidden email]>
> ---
>  lib/pthreadpool/pthreadpool.c | 93 ++++++++++++++++++++++++++++++++++---------
>  1 file changed, 75 insertions(+), 18 deletions(-)
>
> diff --git a/lib/pthreadpool/pthreadpool.c b/lib/pthreadpool/pthreadpool.c
> index 23885aa6d11..0bf7109a8f8 100644
> --- a/lib/pthreadpool/pthreadpool.c
> +++ b/lib/pthreadpool/pthreadpool.c
> @@ -91,11 +91,19 @@ struct pthreadpool {
>   int num_idle;
>  
>   /*
> - * Condition variable indicating that we should quickly go
> - * away making way for fork() without anybody waiting on
> - * pool->condvar.
> + * Condition variable indicating that helper threads should
> + * quickly go away making way for fork() without anybody
> + * waiting on pool->condvar.
>   */
>   pthread_cond_t *prefork_cond;
> +
> + /*
> + * Waiting position for helper threads while fork is
> + * running. The forking thread will have locked it, and all
> + * idle helper threads will sit here until after the fork,
> + * where the forking thread will unlock it again.
> + */
> + pthread_mutex_t fork_mutex;
>  };
>  
>  static pthread_mutex_t pthreadpools_mutex = PTHREAD_MUTEX_INITIALIZER;
> @@ -151,6 +159,15 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult,
>   return ret;
>   }
>  
> + ret = pthread_mutex_init(&pool->fork_mutex, NULL);
> + if (ret != 0) {
> + pthread_cond_destroy(&pool->condvar);
> + pthread_mutex_destroy(&pool->mutex);
> + free(pool->jobs);
> + free(pool);
> + return ret;
> + }
> +
>   pool->shutdown = false;
>   pool->num_threads = 0;
>   pool->max_threads = max_threads;
> @@ -159,6 +176,7 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult,
>  
>   ret = pthread_mutex_lock(&pthreadpools_mutex);
>   if (ret != 0) {
> + pthread_mutex_destroy(&pool->fork_mutex);
>   pthread_cond_destroy(&pool->condvar);
>   pthread_mutex_destroy(&pool->mutex);
>   free(pool->jobs);
> @@ -179,18 +197,26 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult,
>  
>  static void pthreadpool_prepare_pool(struct pthreadpool *pool)
>  {
> - pthread_cond_t prefork_cond = PTHREAD_COND_INITIALIZER;
>   int ret;
>  
> + ret = pthread_mutex_lock(&pool->fork_mutex);
> + assert(ret == 0);
> +
>   ret = pthread_mutex_lock(&pool->mutex);
>   assert(ret == 0);
>  
>   while (pool->num_idle != 0) {
> + int num_idle = pool->num_idle;
> + pthread_cond_t prefork_cond;
> +
> + ret = pthread_cond_init(&prefork_cond, NULL);
> + assert(ret == 0);
> +
>   /*
> - * Exit all idle threads, which are all blocked in
> - * pool->condvar. In the child we can destroy the
> - * pool, which would result in undefined behaviour in
> - * the pthread_cond_destroy(pool->condvar). glibc just
> + * Push all idle threads off pool->condvar. In the
> + * child we can destroy the pool, which would result
> + * in undefined behaviour in the
> + * pthread_cond_destroy(pool->condvar). glibc just
>   * blocks here.
>   */
>   pool->prefork_cond = &prefork_cond;
> @@ -198,14 +224,16 @@ static void pthreadpool_prepare_pool(struct pthreadpool *pool)
>   ret = pthread_cond_signal(&pool->condvar);
>   assert(ret == 0);
>  
> - ret = pthread_cond_wait(&prefork_cond, &pool->mutex);
> - assert(ret == 0);
> + while (pool->num_idle == num_idle) {
> + ret = pthread_cond_wait(&prefork_cond, &pool->mutex);
> + assert(ret == 0);
> + }
>  
>   pool->prefork_cond = NULL;
> - }
>  
> - ret = pthread_cond_destroy(&prefork_cond);
> - assert(ret == 0);
> + ret = pthread_cond_destroy(&prefork_cond);
> + assert(ret == 0);
> + }
>  
>   /*
>   * Probably it's well-defined somewhere: What happens to
> @@ -246,6 +274,8 @@ static void pthreadpool_parent(void)
>   assert(ret == 0);
>   ret = pthread_mutex_unlock(&pool->mutex);
>   assert(ret == 0);
> + ret = pthread_mutex_unlock(&pool->fork_mutex);
> + assert(ret == 0);
>   }
>  
>   ret = pthread_mutex_unlock(&pthreadpools_mutex);
> @@ -268,8 +298,12 @@ static void pthreadpool_child(void)
>  
>   ret = pthread_cond_init(&pool->condvar, NULL);
>   assert(ret == 0);
> +
>   ret = pthread_mutex_unlock(&pool->mutex);
>   assert(ret == 0);
> +
> + ret = pthread_mutex_unlock(&pool->fork_mutex);
> + assert(ret == 0);
>   }
>  
>   ret = pthread_mutex_unlock(&pthreadpools_mutex);
> @@ -284,7 +318,7 @@ static void pthreadpool_prep_atfork(void)
>  
>  static int pthreadpool_free(struct pthreadpool *pool)
>  {
> - int ret, ret1;
> + int ret, ret1, ret2;
>  
>   ret = pthread_mutex_lock(&pthreadpools_mutex);
>   if (ret != 0) {
> @@ -296,6 +330,7 @@ static int pthreadpool_free(struct pthreadpool *pool)
>  
>   ret = pthread_mutex_destroy(&pool->mutex);
>   ret1 = pthread_cond_destroy(&pool->condvar);
> + ret2 = pthread_mutex_destroy(&pool->fork_mutex);
>  
>   if (ret != 0) {
>   return ret;
> @@ -303,6 +338,9 @@ static int pthreadpool_free(struct pthreadpool *pool)
>   if (ret1 != 0) {
>   return ret1;
>   }
> + if (ret2 != 0) {
> + return ret2;
> + }
>  
>   free(pool->jobs);
>   free(pool);
> @@ -462,11 +500,30 @@ static void *pthreadpool_server(void *arg)
>   /*
>   * Me must allow fork() to continue
>   * without anybody waiting on
> - * &pool->condvar.
> + * &pool->condvar. Tell
> + * pthreadpool_prepare_pool that we
> + * got that message.
>   */
> - pthread_cond_signal(pool->prefork_cond);
> - pthreadpool_server_exit(pool);
> - return NULL;
> +
> + res = pthread_cond_signal(pool->prefork_cond);
> + assert(res == 0);
> +
> + res = pthread_mutex_unlock(&pool->mutex);
> + assert(res == 0);
> +
> + /*
> + * pthreadpool_prepare_pool has
> + * already locked this mutex across
> + * the fork. This makes us wait
> + * without sitting in a condvar.
> + */
> + res = pthread_mutex_lock(&pool->fork_mutex);
> + assert(res == 0);
> + res = pthread_mutex_unlock(&pool->fork_mutex);
> + assert(res == 0);
> +
> + res = pthread_mutex_lock(&pool->mutex);
> + assert(res == 0);
>   }
>  
>   if (res == ETIMEDOUT) {
> --
> 2.11.0
>
>
> From c9486a43c83083e47893979a3d450e4d4b464673 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <[hidden email]>
> Date: Wed, 29 Nov 2017 18:55:21 +0100
> Subject: [PATCH 2/2] pthreadpool: Add a test for the race condition fixed in
>  the last commit
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13179
> Signed-off-by: Volker Lendecke <[hidden email]>
> Reviewed-by: Jeremy Allison <[hidden email]>
> ---
>  lib/pthreadpool/tests.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
>
> diff --git a/lib/pthreadpool/tests.c b/lib/pthreadpool/tests.c
> index 1aab80c2bb4..f0ae0aa4a93 100644
> --- a/lib/pthreadpool/tests.c
> +++ b/lib/pthreadpool/tests.c
> @@ -308,6 +308,82 @@ static int test_busyfork(void)
>   return 0;
>  }
>  
> +static int test_busyfork2(void)
> +{
> + struct pthreadpool_pipe *p;
> + pid_t child;
> + int ret, jobnum;
> + struct pollfd pfd;
> +
> + ret = pthreadpool_pipe_init(1, &p);
> + if (ret != 0) {
> + fprintf(stderr, "pthreadpool_pipe_init failed: %s\n",
> + strerror(ret));
> + return -1;
> + }
> +
> + ret = pthreadpool_pipe_add_job(p, 1, busyfork_job, NULL);
> + if (ret != 0) {
> + fprintf(stderr, "pthreadpool_add_job failed: %s\n",
> + strerror(ret));
> + return -1;
> + }
> +
> + ret = pthreadpool_pipe_finished_jobs(p, &jobnum, 1);
> + if (ret != 1) {
> + fprintf(stderr, "pthreadpool_pipe_finished_jobs failed\n");
> + return -1;
> + }
> +
> + ret = poll(NULL, 0, 10);
> + if (ret == -1) {
> + perror("poll failed");
> + return -1;
> + }
> +
> + ret = pthreadpool_pipe_add_job(p, 1, busyfork_job, NULL);
> + if (ret != 0) {
> + fprintf(stderr, "pthreadpool_add_job failed: %s\n",
> + strerror(ret));
> + return -1;
> + }
> +
> + /*
> + * Do the fork right after the add_job. This tests a race
> + * where the atfork prepare handler gets all idle threads off
> + * the condvar. If we are faster doing the fork than the
> + * existing idle thread could get out of idle and take the
> + * job, after the fork we end up with no threads to take care
> + * of the job.
> + */
> +
> + child = fork();
> + if (child < 0) {
> + perror("fork failed");
> + return -1;
> + }
> +
> + if (child == 0) {
> + exit(0);
> + }
> +
> + pfd = (struct pollfd) {
> + .fd = pthreadpool_pipe_signal_fd(p),
> + .events = POLLIN|POLLERR
> + };
> +
> + do {
> + ret = poll(&pfd, 1, 5000);
> + } while ((ret == -1) && (errno == EINTR));
> +
> + if (ret == 0) {
> + fprintf(stderr, "job unfinished after 5 seconds\n");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
>  static void test_tevent_wait(void *private_data)
>  {
>   int *timeout = private_data;
> @@ -423,6 +499,12 @@ int main(void)
>   return 1;
>   }
>  
> + ret = test_busyfork2();
> + if (ret != 0) {
> + fprintf(stderr, "test_busyfork2 failed\n");
> + return 1;
> + }
> +
>   printf("success\n");
>   return 0;
>  }
> --
> 2.11.0
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
Ok I've managed to get to this to dead lock on our cloud. The build I've
been testing sets socket close exec. But I don't see how this would
impact the pthread cmocka tests.

stack trace attached.

On 08/12/17 05:56, Jeremy Allison via samba-technical wrote:

> On Thu, Dec 07, 2017 at 07:59:35AM +0100, Volker Lendecke wrote:
>> On Wed, Dec 06, 2017 at 04:53:15PM -0800, Jeremy Allison wrote:
>>> Feel free to push with any of the
>>> text of this or the previous message in the
>>> commit message if you think it will help others :-).
>>
>> Attached patch has the very nice explanation in the commit message. It
>> also removes the necessity for the preliminary patch. Runs in a
>> private autobuild now. I'll push once this survived and nobody objects
>> in the meantime.
>>
>> Thanks!
>
> Thanks for adding the extra text Volker, it makes it much
> easier for people who don't understand MT-code (looking at
> myself in the mirror here :-) to understand the commit
> changes. Please push !
>
> Jeremy.
>
>> --
>> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
>> phone: +49-551-370000-0, fax: +49-551-370000-9
>> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
>> http://www.sernet.de, mailto:[hidden email]
>
>> From 287c0232d2e7ef4d7c6ce278e31606f2b5a470e4 Mon Sep 17 00:00:00 2001
>> From: Volker Lendecke <[hidden email]>
>> Date: Wed, 29 Nov 2017 16:45:40 +0100
>> Subject: [PATCH 1/2] pthreadpool: Fix starvation after fork
>>
>> After the race is before the race:
>>
>> 1) Create an idle thread
>> 2) Add a job: This won't create a thread anymore
>> 3) Immediately fork
>>
>> The idle thread will be woken twice before it's actually woken up: Both
>> pthreadpool_add_job and pthreadpool_prepare_pool call cond_signal, for
>> different reasons. We must look at pool->prefork_cond first because otherwise
>> we will end up in a blocking job deep within a fork call, the helper thread
>> must take its fingers off the condvar as quickly as possible.  This means that
>> after the fork there's no idle thread around anymore that would pick up the job
>> submitted in 2). So we must keep the idle threads around across the fork.
>>
>> The quick solution to re-create one helper thread in pthreadpool_parent has a
>> fatal flaw: What do we do if that pthread_create call fails? We're deep in an
>> application calling fork(), and doing fancy signalling from there is really
>> something we must avoid.
>>
>> This has one potential performance issue: If we have hundreds of idle threads
>> (do we ever have that) during the fork, the call to pthread_mutex_lock on the
>> fork_mutex from pthreadpool_server (the helper thread) will probably cause a
>> thundering herd when the _parent call unlocks the fork_mutex. The solution for
>> this to just keep one idle thread around. But this adds code that is not
>> strictly required functionally for now.
>>
>> More detailed explanation from Jeremy:
>>
>> First, understanding the problem the test reproduces:
>>
>> add a job (num_jobs = 1) -> creates thread to run it.
>> job finishes, thread sticks around (num_idle = 1).
>> num_jobs is now zero (initial job finished).
>>
>> a) Idle thread is now waiting on pool->condvar inside
>> pthreadpool_server() in pthread_cond_timedwait().
>>
>> Now, add another job ->
>>
>> pthreadpool_add_job()
>> -> pthreadpool_put_job()
>> This adds the job to the queue.
>> Oh, there is an idle thread so don't
>> create one, do:
>>
>> pthread_cond_signal(&pool->condvar);
>>
>> and return.
>>
>> Now call fork *before* idle thread in (a) wakes from
>> the signaling of pool->condvar.
>>
>> In the parent (child is irrelevent):
>>
>> Go into: pthreadpool_prepare() ->
>> pthreadpool_prepare_pool()
>>
>> Set the variable to tell idle threads to exit:
>>
>> pool->prefork_cond = &prefork_cond;
>>
>> then wake them up with:
>>
>> pthread_cond_signal(&pool->condvar);
>>
>> This does nothing as the idle thread
>> is already awoken.
>>
>> b) Idle thread wakes up and does:
>>
>> Reduce idle thread count (num_idle = 0)
>>
>> pool->num_idle -= 1;
>>
>> Check if we're in the middle of a fork.
>>
>> if (pool->prefork_cond != NULL) {
>>
>> Yes we are, tell pthreadpool_prepare()
>> we are exiting.
>>
>> pthread_cond_signal(pool->prefork_cond);
>>
>> And exit.
>>
>> pthreadpool_server_exit(pool);
>> return NULL;
>> }
>>
>> So we come back from the fork in the parent with num_jobs = 1,
>> a job on the queue but no idle threads - and the code that
>> creates a new thread on job submission was skipped because
>> an idle thread existed at point (a).
>>
>> OK, assuming that the previous explaination is correct, the
>> fix is to create a new pthreadpool context mutex:
>>
>> pool->fork_mutex
>>
>> and in pthreadpool_server(), when an idle thread wakes up and
>> notices we're in the prepare fork state, it puts itself to
>> sleep by waiting on the new pool->fork_mutex.
>>
>> And in pthreadpool_prepare_pool(), instead of waiting for
>> the idle threads to exit, hold the pool->fork_mutex and
>> signal each idle thread in turn, and wait for the pool->num_idle
>> to go to zero - which means they're all blocked waiting on
>> pool->fork_mutex.
>>
>> When the parent continues, pthreadpool_parent()
>> unlocks the pool->fork_mutex and all the previously
>> 'idle' threads wake up (and you mention the thundering
>> herd problem, which is as you say vanishingly small :-)
>> and pick up any remaining job.
>>
>> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13179
>> Signed-off-by: Volker Lendecke <[hidden email]>
>> Reviewed-by: Jeremy Allison <[hidden email]>
>> ---
>>  lib/pthreadpool/pthreadpool.c | 93 ++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 75 insertions(+), 18 deletions(-)
>>
>> diff --git a/lib/pthreadpool/pthreadpool.c b/lib/pthreadpool/pthreadpool.c
>> index 23885aa6d11..0bf7109a8f8 100644
>> --- a/lib/pthreadpool/pthreadpool.c
>> +++ b/lib/pthreadpool/pthreadpool.c
>> @@ -91,11 +91,19 @@ struct pthreadpool {
>>   int num_idle;
>>  
>>   /*
>> - * Condition variable indicating that we should quickly go
>> - * away making way for fork() without anybody waiting on
>> - * pool->condvar.
>> + * Condition variable indicating that helper threads should
>> + * quickly go away making way for fork() without anybody
>> + * waiting on pool->condvar.
>>   */
>>   pthread_cond_t *prefork_cond;
>> +
>> + /*
>> + * Waiting position for helper threads while fork is
>> + * running. The forking thread will have locked it, and all
>> + * idle helper threads will sit here until after the fork,
>> + * where the forking thread will unlock it again.
>> + */
>> + pthread_mutex_t fork_mutex;
>>  };
>>  
>>  static pthread_mutex_t pthreadpools_mutex = PTHREAD_MUTEX_INITIALIZER;
>> @@ -151,6 +159,15 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult,
>>   return ret;
>>   }
>>  
>> + ret = pthread_mutex_init(&pool->fork_mutex, NULL);
>> + if (ret != 0) {
>> + pthread_cond_destroy(&pool->condvar);
>> + pthread_mutex_destroy(&pool->mutex);
>> + free(pool->jobs);
>> + free(pool);
>> + return ret;
>> + }
>> +
>>   pool->shutdown = false;
>>   pool->num_threads = 0;
>>   pool->max_threads = max_threads;
>> @@ -159,6 +176,7 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult,
>>  
>>   ret = pthread_mutex_lock(&pthreadpools_mutex);
>>   if (ret != 0) {
>> + pthread_mutex_destroy(&pool->fork_mutex);
>>   pthread_cond_destroy(&pool->condvar);
>>   pthread_mutex_destroy(&pool->mutex);
>>   free(pool->jobs);
>> @@ -179,18 +197,26 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult,
>>  
>>  static void pthreadpool_prepare_pool(struct pthreadpool *pool)
>>  {
>> - pthread_cond_t prefork_cond = PTHREAD_COND_INITIALIZER;
>>   int ret;
>>  
>> + ret = pthread_mutex_lock(&pool->fork_mutex);
>> + assert(ret == 0);
>> +
>>   ret = pthread_mutex_lock(&pool->mutex);
>>   assert(ret == 0);
>>  
>>   while (pool->num_idle != 0) {
>> + int num_idle = pool->num_idle;
>> + pthread_cond_t prefork_cond;
>> +
>> + ret = pthread_cond_init(&prefork_cond, NULL);
>> + assert(ret == 0);
>> +
>>   /*
>> - * Exit all idle threads, which are all blocked in
>> - * pool->condvar. In the child we can destroy the
>> - * pool, which would result in undefined behaviour in
>> - * the pthread_cond_destroy(pool->condvar). glibc just
>> + * Push all idle threads off pool->condvar. In the
>> + * child we can destroy the pool, which would result
>> + * in undefined behaviour in the
>> + * pthread_cond_destroy(pool->condvar). glibc just
>>   * blocks here.
>>   */
>>   pool->prefork_cond = &prefork_cond;
>> @@ -198,14 +224,16 @@ static void pthreadpool_prepare_pool(struct pthreadpool *pool)
>>   ret = pthread_cond_signal(&pool->condvar);
>>   assert(ret == 0);
>>  
>> - ret = pthread_cond_wait(&prefork_cond, &pool->mutex);
>> - assert(ret == 0);
>> + while (pool->num_idle == num_idle) {
>> + ret = pthread_cond_wait(&prefork_cond, &pool->mutex);
>> + assert(ret == 0);
>> + }
>>  
>>   pool->prefork_cond = NULL;
>> - }
>>  
>> - ret = pthread_cond_destroy(&prefork_cond);
>> - assert(ret == 0);
>> + ret = pthread_cond_destroy(&prefork_cond);
>> + assert(ret == 0);
>> + }
>>  
>>   /*
>>   * Probably it's well-defined somewhere: What happens to
>> @@ -246,6 +274,8 @@ static void pthreadpool_parent(void)
>>   assert(ret == 0);
>>   ret = pthread_mutex_unlock(&pool->mutex);
>>   assert(ret == 0);
>> + ret = pthread_mutex_unlock(&pool->fork_mutex);
>> + assert(ret == 0);
>>   }
>>  
>>   ret = pthread_mutex_unlock(&pthreadpools_mutex);
>> @@ -268,8 +298,12 @@ static void pthreadpool_child(void)
>>  
>>   ret = pthread_cond_init(&pool->condvar, NULL);
>>   assert(ret == 0);
>> +
>>   ret = pthread_mutex_unlock(&pool->mutex);
>>   assert(ret == 0);
>> +
>> + ret = pthread_mutex_unlock(&pool->fork_mutex);
>> + assert(ret == 0);
>>   }
>>  
>>   ret = pthread_mutex_unlock(&pthreadpools_mutex);
>> @@ -284,7 +318,7 @@ static void pthreadpool_prep_atfork(void)
>>  
>>  static int pthreadpool_free(struct pthreadpool *pool)
>>  {
>> - int ret, ret1;
>> + int ret, ret1, ret2;
>>  
>>   ret = pthread_mutex_lock(&pthreadpools_mutex);
>>   if (ret != 0) {
>> @@ -296,6 +330,7 @@ static int pthreadpool_free(struct pthreadpool *pool)
>>  
>>   ret = pthread_mutex_destroy(&pool->mutex);
>>   ret1 = pthread_cond_destroy(&pool->condvar);
>> + ret2 = pthread_mutex_destroy(&pool->fork_mutex);
>>  
>>   if (ret != 0) {
>>   return ret;
>> @@ -303,6 +338,9 @@ static int pthreadpool_free(struct pthreadpool *pool)
>>   if (ret1 != 0) {
>>   return ret1;
>>   }
>> + if (ret2 != 0) {
>> + return ret2;
>> + }
>>  
>>   free(pool->jobs);
>>   free(pool);
>> @@ -462,11 +500,30 @@ static void *pthreadpool_server(void *arg)
>>   /*
>>   * Me must allow fork() to continue
>>   * without anybody waiting on
>> - * &pool->condvar.
>> + * &pool->condvar. Tell
>> + * pthreadpool_prepare_pool that we
>> + * got that message.
>>   */
>> - pthread_cond_signal(pool->prefork_cond);
>> - pthreadpool_server_exit(pool);
>> - return NULL;
>> +
>> + res = pthread_cond_signal(pool->prefork_cond);
>> + assert(res == 0);
>> +
>> + res = pthread_mutex_unlock(&pool->mutex);
>> + assert(res == 0);
>> +
>> + /*
>> + * pthreadpool_prepare_pool has
>> + * already locked this mutex across
>> + * the fork. This makes us wait
>> + * without sitting in a condvar.
>> + */
>> + res = pthread_mutex_lock(&pool->fork_mutex);
>> + assert(res == 0);
>> + res = pthread_mutex_unlock(&pool->fork_mutex);
>> + assert(res == 0);
>> +
>> + res = pthread_mutex_lock(&pool->mutex);
>> + assert(res == 0);
>>   }
>>  
>>   if (res == ETIMEDOUT) {
>> --
>> 2.11.0
>>
>>
>> From c9486a43c83083e47893979a3d450e4d4b464673 Mon Sep 17 00:00:00 2001
>> From: Volker Lendecke <[hidden email]>
>> Date: Wed, 29 Nov 2017 18:55:21 +0100
>> Subject: [PATCH 2/2] pthreadpool: Add a test for the race condition fixed in
>>  the last commit
>>
>> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13179
>> Signed-off-by: Volker Lendecke <[hidden email]>
>> Reviewed-by: Jeremy Allison <[hidden email]>
>> ---
>>  lib/pthreadpool/tests.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 82 insertions(+)
>>
>> diff --git a/lib/pthreadpool/tests.c b/lib/pthreadpool/tests.c
>> index 1aab80c2bb4..f0ae0aa4a93 100644
>> --- a/lib/pthreadpool/tests.c
>> +++ b/lib/pthreadpool/tests.c
>> @@ -308,6 +308,82 @@ static int test_busyfork(void)
>>   return 0;
>>  }
>>  
>> +static int test_busyfork2(void)
>> +{
>> + struct pthreadpool_pipe *p;
>> + pid_t child;
>> + int ret, jobnum;
>> + struct pollfd pfd;
>> +
>> + ret = pthreadpool_pipe_init(1, &p);
>> + if (ret != 0) {
>> + fprintf(stderr, "pthreadpool_pipe_init failed: %s\n",
>> + strerror(ret));
>> + return -1;
>> + }
>> +
>> + ret = pthreadpool_pipe_add_job(p, 1, busyfork_job, NULL);
>> + if (ret != 0) {
>> + fprintf(stderr, "pthreadpool_add_job failed: %s\n",
>> + strerror(ret));
>> + return -1;
>> + }
>> +
>> + ret = pthreadpool_pipe_finished_jobs(p, &jobnum, 1);
>> + if (ret != 1) {
>> + fprintf(stderr, "pthreadpool_pipe_finished_jobs failed\n");
>> + return -1;
>> + }
>> +
>> + ret = poll(NULL, 0, 10);
>> + if (ret == -1) {
>> + perror("poll failed");
>> + return -1;
>> + }
>> +
>> + ret = pthreadpool_pipe_add_job(p, 1, busyfork_job, NULL);
>> + if (ret != 0) {
>> + fprintf(stderr, "pthreadpool_add_job failed: %s\n",
>> + strerror(ret));
>> + return -1;
>> + }
>> +
>> + /*
>> + * Do the fork right after the add_job. This tests a race
>> + * where the atfork prepare handler gets all idle threads off
>> + * the condvar. If we are faster doing the fork than the
>> + * existing idle thread could get out of idle and take the
>> + * job, after the fork we end up with no threads to take care
>> + * of the job.
>> + */
>> +
>> + child = fork();
>> + if (child < 0) {
>> + perror("fork failed");
>> + return -1;
>> + }
>> +
>> + if (child == 0) {
>> + exit(0);
>> + }
>> +
>> + pfd = (struct pollfd) {
>> + .fd = pthreadpool_pipe_signal_fd(p),
>> + .events = POLLIN|POLLERR
>> + };
>> +
>> + do {
>> + ret = poll(&pfd, 1, 5000);
>> + } while ((ret == -1) && (errno == EINTR));
>> +
>> + if (ret == 0) {
>> + fprintf(stderr, "job unfinished after 5 seconds\n");
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>>  static void test_tevent_wait(void *private_data)
>>  {
>>   int *timeout = private_data;
>> @@ -423,6 +499,12 @@ int main(void)
>>   return 1;
>>   }
>>  
>> + ret = test_busyfork2();
>> + if (ret != 0) {
>> + fprintf(stderr, "test_busyfork2 failed\n");
>> + return 1;
>> + }
>> +
>>   printf("success\n");
>>   return 0;
>>  }
>> --
>> 2.11.0
>>
>
>

stack.trace (7K) Download Attachment
signature.asc (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
On Mon, Dec 11, 2017 at 04:25:14PM +1300, Gary Lockyer via samba-technical wrote:
> Ok I've managed to get to this to dead lock on our cloud. The build I've
> been testing sets socket close exec. But I don't see how this would
> impact the pthread cmocka tests.
>
> stack trace attached.

Do you have full debug symbols available, so that we can see the full
stacktrace including source code files and line numbers?

Thanks, Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
Stack trace with symbols attached.

Cheers
Gary

On 11/12/17 21:53, Volker Lendecke via samba-technical wrote:

> On Mon, Dec 11, 2017 at 04:25:14PM +1300, Gary Lockyer via samba-technical wrote:
>> Ok I've managed to get to this to dead lock on our cloud. The build I've
>> been testing sets socket close exec. But I don't see how this would
>> impact the pthread cmocka tests.
>>
>> stack trace attached.
>
> Do you have full debug symbols available, so that we can see the full
> stacktrace including source code files and line numbers?
>
> Thanks, Volker
>

stack.trace (14K) Download Attachment
signature.asc (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
On Tue, Dec 12, 2017 at 06:57:22AM +1300, Gary Lockyer wrote:
> Stack trace with symbols attached.

Thanks. This is pretty easy to reproduce in the new cmocka test by
Christof Schmitt. Just run

while : ; do valgrind --tool=helgrind --num-callers=50 bin/pthreadpooltest_cmocka; done

and wait a few seconds. I'm getting

==5733== ---Thread-Announcement------------------------------------------
==5733==
==5733== Thread #1 is the program's root thread
==5733==
==5733== ----------------------------------------------------------------
==5733==
==5733== Thread #1: Attempt to re-lock a non-recursive lock I already hold
==5733==    at 0x4C30044: ??? (in /usr/lib/valgrind/vgpreload_helgrind-amd64-linux.so)
==5733==    by 0x10AC08: pthreadpool_destroy (pthreadpool.c:360)
==5733==    by 0x10C77F: pthreadpool_tevent_destructor (pthreadpool_tevent.c:111)
==5733==    by 0x56887BD: _tc_free_internal (talloc.c:1078)
==5733==    by 0x568491B: _tc_free_children_internal (talloc.c:1593)
==5733==    by 0x568899C: _tc_free_internal (talloc.c:1104)
==5733==    by 0x568491B: _tc_free_children_internal (talloc.c:1593)
==5733==    by 0x568899C: _tc_free_internal (talloc.c:1104)
==5733==    by 0x5683C61: _talloc_free_internal (talloc.c:1174)
==5733==    by 0x5684B53: _talloc_free (talloc.c:1716)
==5733==    by 0x10A5D4: teardown_pthreadpool_tevent (tests_cmocka.c:60)
==5733==    by 0x50622AC: cmocka_run_one_test_or_fixture (cmocka.c:2628)
==5733==    by 0x5060C1D: cmocka_run_one_tests (cmocka.c:2752)
==5733==    by 0x5060414: _cmocka_run_group_tests (cmocka.c:2839)
==5733==    by 0x10A307: main (tests_cmocka.c:175)
==5733==  Lock was previously acquired
==5733==    at 0x4C3010C: ??? (in /usr/lib/valgrind/vgpreload_helgrind-amd64-linux.so)
==5733==    by 0x10AFA6: pthreadpool_add_job (pthreadpool.c:636)
==5733==    by 0x10CADB: pthreadpool_tevent_job_send (pthreadpool_tevent.c:297)
==5733==    by 0x10A641: test_create_do (tests_cmocka.c:101)
==5733==    by 0x10A465: test_create (tests_cmocka.c:160)
==5733==    by 0x50621EC: cmocka_run_one_test_or_fixture (cmocka.c:2616)
==5733==    by 0x5060B0B: cmocka_run_one_tests (cmocka.c:2724)
==5733==    by 0x5060414: _cmocka_run_group_tests (cmocka.c:2839)
==5733==    by 0x10A307: main (tests_cmocka.c:175)

and I am completely confused. How can we leave the mutex locked when
leaving pthreadpool_add_job... Looking further...

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
On Tue, Dec 12, 2017 at 01:40:29PM +0100, Volker Lendecke wrote:
> On Tue, Dec 12, 2017 at 06:57:22AM +1300, Gary Lockyer wrote:
> > Stack trace with symbols attached.
>
> Thanks. This is pretty easy to reproduce in the new cmocka test by
> Christof Schmitt. Just run
>
> while : ; do valgrind --tool=helgrind --num-callers=50 bin/pthreadpooltest_cmocka; done
>
> and wait a few seconds. I'm getting

Just got

failure: test_create [
No entries for symbol __wrap_pthread_create.
../lib/pthreadpool/tests_cmocka.c:75: error: Could not get value to mock function __wrap_pthread_create
../lib/pthreadpool/tests_cmocka.c:150: note: Previously returned mock value was declared here
]

in one of a few hundred runs. Andreas, can you imagine what that might
be?

Thanks,

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
On Tue, Dec 12, 2017 at 02:37:02PM +0100, Volker Lendecke wrote:

> On Tue, Dec 12, 2017 at 01:40:29PM +0100, Volker Lendecke wrote:
> > On Tue, Dec 12, 2017 at 06:57:22AM +1300, Gary Lockyer wrote:
> > > Stack trace with symbols attached.
> >
> > Thanks. This is pretty easy to reproduce in the new cmocka test by
> > Christof Schmitt. Just run
> >
> > while : ; do valgrind --tool=helgrind --num-callers=50 bin/pthreadpooltest_cmocka; done
> >
> > and wait a few seconds. I'm getting
>
> Just got
>
> failure: test_create [
> No entries for symbol __wrap_pthread_create.
> ../lib/pthreadpool/tests_cmocka.c:75: error: Could not get value to mock function __wrap_pthread_create
> ../lib/pthreadpool/tests_cmocka.c:150: note: Previously returned mock value was declared here
> ]
>
> in one of a few hundred runs. Andreas, can you imagine what that might
> be?

Hmmmm. cmocka.c does setjmp/longjmp. This *might* be an explanation
how we leave pthreadpool_add_job with a mutex locked. I think I need
to fully understand cmocka.c now, now that the Samba Team has decided
that we are fully responsible for that as well :-(

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

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 Tue, Dec 12, 2017 at 01:40:29PM +0100, Volker Lendecke via samba-technical wrote:

> On Tue, Dec 12, 2017 at 06:57:22AM +1300, Gary Lockyer wrote:
> > Stack trace with symbols attached.
>
> Thanks. This is pretty easy to reproduce in the new cmocka test by
> Christof Schmitt. Just run
>
> while : ; do valgrind --tool=helgrind --num-callers=50 bin/pthreadpooltest_cmocka; done
>
> and wait a few seconds. I'm getting
>
> ==5733== ---Thread-Announcement------------------------------------------
> ==5733==
> ==5733== Thread #1 is the program's root thread
> ==5733==
> ==5733== ----------------------------------------------------------------
> ==5733==
> ==5733== Thread #1: Attempt to re-lock a non-recursive lock I already hold
> ==5733==    at 0x4C30044: ??? (in /usr/lib/valgrind/vgpreload_helgrind-amd64-linux.so)
> ==5733==    by 0x10AC08: pthreadpool_destroy (pthreadpool.c:360)
> ==5733==    by 0x10C77F: pthreadpool_tevent_destructor (pthreadpool_tevent.c:111)
> ==5733==    by 0x56887BD: _tc_free_internal (talloc.c:1078)
> ==5733==    by 0x568491B: _tc_free_children_internal (talloc.c:1593)
> ==5733==    by 0x568899C: _tc_free_internal (talloc.c:1104)
> ==5733==    by 0x568491B: _tc_free_children_internal (talloc.c:1593)
> ==5733==    by 0x568899C: _tc_free_internal (talloc.c:1104)
> ==5733==    by 0x5683C61: _talloc_free_internal (talloc.c:1174)
> ==5733==    by 0x5684B53: _talloc_free (talloc.c:1716)
> ==5733==    by 0x10A5D4: teardown_pthreadpool_tevent (tests_cmocka.c:60)
> ==5733==    by 0x50622AC: cmocka_run_one_test_or_fixture (cmocka.c:2628)
> ==5733==    by 0x5060C1D: cmocka_run_one_tests (cmocka.c:2752)
> ==5733==    by 0x5060414: _cmocka_run_group_tests (cmocka.c:2839)
> ==5733==    by 0x10A307: main (tests_cmocka.c:175)
> ==5733==  Lock was previously acquired
> ==5733==    at 0x4C3010C: ??? (in /usr/lib/valgrind/vgpreload_helgrind-amd64-linux.so)
> ==5733==    by 0x10AFA6: pthreadpool_add_job (pthreadpool.c:636)
> ==5733==    by 0x10CADB: pthreadpool_tevent_job_send (pthreadpool_tevent.c:297)
> ==5733==    by 0x10A641: test_create_do (tests_cmocka.c:101)
> ==5733==    by 0x10A465: test_create (tests_cmocka.c:160)
> ==5733==    by 0x50621EC: cmocka_run_one_test_or_fixture (cmocka.c:2616)
> ==5733==    by 0x5060B0B: cmocka_run_one_tests (cmocka.c:2724)
> ==5733==    by 0x5060414: _cmocka_run_group_tests (cmocka.c:2839)
> ==5733==    by 0x10A307: main (tests_cmocka.c:175)
>
> and I am completely confused. How can we leave the mutex locked when
> leaving pthreadpool_add_job... Looking further...

cmocka keeps the global list to pass input to the mock function. Data is
appeneded through will_return and then the mock function calls mock_type
to dequeue an entry and act on it.

cmocka verifies that for each test, the number of will_return calls
matches the number of calls to mock_type. That could be a problem in
this test case:

        /*
         * When a thread can be created, the job will run in the worker thread.
         */
        will_return(__wrap_pthread_create, 0);
        ret = test_create_do(t->ev, t->pool, &in_main_thread);
        assert_return_code(ret, 0);
        assert_false(in_main_thread);

In the above, pthread_create will be called once, so having one entry passed
through will_return is always correct.

        /*
         * Workerthread will still be active for a second; immediately
         * running another job will also use the worker thread, even
         * if a new thread cannot be created.
         */
        ret = test_create_do(t->ev, t->pool, &in_main_thread);
        assert_return_code(ret, 0);
        assert_false(in_main_thread);

The assumption here is that no call to pthread_create is done and hence no call
to will_return is required. What likely happened is that the worker thread
exited, then test_create_do triggered the creation of a new pthread, which in
turn caused a call to mock_type. As there is no entry in the global list,
cmocka tried to abort through a jump while still holding the mutex.

Since this now appears to be a problem in some test environments, i would
suggest to simply remove the last part of the test (the call to test_create_do
without a previous will_return).

Christof

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
On Tue, Dec 12, 2017 at 01:02:46PM -0700, Christof Schmitt wrote:
> The assumption here is that no call to pthread_create is done and hence no call
> to will_return is required. What likely happened is that the worker thread
> exited, then test_create_do triggered the creation of a new pthread, which in
> turn caused a call to mock_type. As there is no entry in the global list,
> cmocka tried to abort through a jump while still holding the mutex.

Worker threads only exit after one second of idle time. I can
reproduce the issue within a minute if running the cmocka test in a
tight loop, and eventually it will block. But my system is completely
idle otherwise, so it's extremely unlikely that one second passed
between two test steps.

> Since this now appears to be a problem in some test environments, i would
> suggest to simply remove the last part of the test (the call to test_create_do
> without a previous will_return).

I have it on a stock debian stretch, 64-bit. Admittedly in a virtual
environment, but a very fast one.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

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 Tue, Dec 12, 2017 at 01:02:46PM -0700, Christof Schmitt wrote:
> The assumption here is that no call to pthread_create is done and hence no call
> to will_return is required. What likely happened is that the worker thread
> exited, then test_create_do triggered the creation of a new pthread, which in
> turn caused a call to mock_type. As there is no entry in the global list,
> cmocka tried to abort through a jump while still holding the mutex.

The helper thread did not exit, it was *not yet* idle.

The attached patch fixes the problem for me.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

patch.txt (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
On Tue, Dec 12, 2017 at 11:10:55PM +0100, Volker Lendecke wrote:

> On Tue, Dec 12, 2017 at 01:02:46PM -0700, Christof Schmitt wrote:
> > The assumption here is that no call to pthread_create is done and hence no call
> > to will_return is required. What likely happened is that the worker thread
> > exited, then test_create_do triggered the creation of a new pthread, which in
> > turn caused a call to mock_type. As there is no entry in the global list,
> > cmocka tried to abort through a jump while still holding the mutex.
>
> The helper thread did not exit, it was *not yet* idle.
>
> The attached patch fixes the problem for me.

Makese sense.

Reviewed-by: Christof Schmitt <[hidden email]>

Pushed to autobuild.

Christof

>
> Volker
>
> --
> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: +49-551-370000-0, fax: +49-551-370000-9
> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
> http://www.sernet.de, mailto:[hidden email]

> From ae056538b473d94e720af93092dec13f5386c38f Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <[hidden email]>
> Date: Tue, 12 Dec 2017 23:07:39 +0100
> Subject: [PATCH] pthreadpool: Fix deadlock
>
> Christof's idea from was that the thread already exited. It could also be that
> the thread is not yet idle when the new pthreadpool_add_jobs comes around the
> corner.
>
> Signed-off-by: Volker Lendecke <[hidden email]>
> ---
>  lib/pthreadpool/tests_cmocka.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/lib/pthreadpool/tests_cmocka.c b/lib/pthreadpool/tests_cmocka.c
> index 75a935fa42c..9753d212e1c 100644
> --- a/lib/pthreadpool/tests_cmocka.c
> +++ b/lib/pthreadpool/tests_cmocka.c
> @@ -28,6 +28,7 @@
>  #include <pthreadpool_tevent.h>
>  
>  #include <cmocka.h>
> +#include <poll.h>
>  
>  struct pthreadpool_tevent_test {
>   struct tevent_context *ev;
> @@ -152,6 +153,8 @@ static void test_create(void **state)
>   assert_return_code(ret, 0);
>   assert_false(in_main_thread);
>  
> + poll(NULL, 0, 10);
> +
>   /*
>   * Workerthread will still be active for a second; immediately
>   * running another job will also use the worker thread, even
> --
> 2.11.0
>


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 Tue, Dec 12, 2017 at 11:10:55PM +0100, Volker Lendecke wrote:

> On Tue, Dec 12, 2017 at 01:02:46PM -0700, Christof Schmitt wrote:
> > The assumption here is that no call to pthread_create is done and hence no call
> > to will_return is required. What likely happened is that the worker thread
> > exited, then test_create_do triggered the creation of a new pthread, which in
> > turn caused a call to mock_type. As there is no entry in the global list,
> > cmocka tried to abort through a jump while still holding the mutex.
>
> The helper thread did not exit, it was *not yet* idle.
>
> The attached patch fixes the problem for me.
Gna. Forgot to add the mailing list reference. Same patch, new commit
msg.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

patch.txt (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
On Tue, Dec 12, 2017 at 11:17:26PM +0100, Volker Lendecke wrote:

> On Tue, Dec 12, 2017 at 11:10:55PM +0100, Volker Lendecke wrote:
> > On Tue, Dec 12, 2017 at 01:02:46PM -0700, Christof Schmitt wrote:
> > > The assumption here is that no call to pthread_create is done and hence no call
> > > to will_return is required. What likely happened is that the worker thread
> > > exited, then test_create_do triggered the creation of a new pthread, which in
> > > turn caused a call to mock_type. As there is no entry in the global list,
> > > cmocka tried to abort through a jump while still holding the mutex.
> >
> > The helper thread did not exit, it was *not yet* idle.
> >
> > The attached patch fixes the problem for me.
>
> Gna. Forgot to add the mailing list reference. Same patch, new commit
> msg.

Reviewed and pushed updated patch.

Christof

>
> Volker
>
> --
> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: +49-551-370000-0, fax: +49-551-370000-9
> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
> http://www.sernet.de, mailto:[hidden email]

> From 83f01a22522f0efd7e6138577b8c427e2223b02f Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <[hidden email]>
> Date: Tue, 12 Dec 2017 23:07:39 +0100
> Subject: [PATCH] pthreadpool: Fix deadlock
>
> Christof's idea from
>
> https://lists.samba.org/archive/samba-technical/2017-December/124384.html
>
> was that the thread already exited. It could also be that the thread is
> not yet idle when the new pthreadpool_add_jobs comes around the corner.
>
> Signed-off-by: Volker Lendecke <[hidden email]>
> ---
>  lib/pthreadpool/tests_cmocka.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/lib/pthreadpool/tests_cmocka.c b/lib/pthreadpool/tests_cmocka.c
> index 75a935fa42c..9753d212e1c 100644
> --- a/lib/pthreadpool/tests_cmocka.c
> +++ b/lib/pthreadpool/tests_cmocka.c
> @@ -28,6 +28,7 @@
>  #include <pthreadpool_tevent.h>
>  
>  #include <cmocka.h>
> +#include <poll.h>
>  
>  struct pthreadpool_tevent_test {
>   struct tevent_context *ev;
> @@ -152,6 +153,8 @@ static void test_create(void **state)
>   assert_return_code(ret, 0);
>   assert_false(in_main_thread);
>  
> + poll(NULL, 0, 10);
> +
>   /*
>   * Workerthread will still be active for a second; immediately
>   * running another job will also use the worker thread, even
> --
> 2.11.0
>


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
Hi Volker!

On 07.12.2017 07:59, Volker Lendecke via samba-technical wrote:
> Attached patch has the very nice explanation in the commit message. It
> also removes the necessity for the preliminary patch. Runs in a
> private autobuild now. I'll push once this survived and nobody objects
> in the meantime.

Maybe I'm a little bit late, but please take a look at my attached patch
set. It fixes the job-starvation-after-fork race without introducing a
new mutex. By moving the check on pool->prefork_cond in front of the
pthread_cond_timedwait() call on pool->condvar the thread will continue
with processing jobs. Threads will only be killed if the job queue is empty.

I addition I wrote a new test case to test this scenario with multiple
threads running in parallel and doing a fork with jobs sitting on the
job queue and doing a 2nd fork when only idle threads are around. One
todo for this test case would be to add a check that all idle threads
are gone after the 2nd fork. Unfortunately I didn't found a portable
solution without checking /proc/<pid>/task or running a ps command.
Maybe somebody else on this list has a clever idea.

The patch set applies on master.

--
Regards

    Ralph Wuerthner

fix-job-starvation-after-fork-race.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix a pthreadpool race

Samba - samba-technical mailing list
On Fri, Dec 15, 2017 at 12:17:07PM +0100, Ralph Wuerthner wrote:

> Hi Volker!
>
> On 07.12.2017 07:59, Volker Lendecke via samba-technical wrote:
> > Attached patch has the very nice explanation in the commit message. It
> > also removes the necessity for the preliminary patch. Runs in a
> > private autobuild now. I'll push once this survived and nobody objects
> > in the meantime.
>
> Maybe I'm a little bit late, but please take a look at my attached patch
> set. It fixes the job-starvation-after-fork race without introducing a new
> mutex. By moving the check on pool->prefork_cond in front of the
> pthread_cond_timedwait() call on pool->condvar the thread will continue with
> processing jobs. Threads will only be killed if the job queue is empty.
>
> I addition I wrote a new test case to test this scenario with multiple
> threads running in parallel and doing a fork with jobs sitting on the job
> queue and doing a 2nd fork when only idle threads are around. One todo for
> this test case would be to add a check that all idle threads are gone after
> the 2nd fork. Unfortunately I didn't found a portable solution without
> checking /proc/<pid>/task or running a ps command. Maybe somebody else on
> this list has a clever idea.
>
> The patch set applies on master.

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 :-)

Thanks,

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

12