[PATCH] Cleanups for pthreadpool.c

[Jeremy Allison]

On Tue, Dec 12, 2017 at 03:53:08PM +0100, Volker Lendecke via samba-technical wrote:
> Hi!
> 
> While hunting the deadlock I did the attached small patches.
> 
> Review appreciated!

LGTM. Thanks for the logic cleanup, makes it clearer.

Reviewed-by: Jeremy Allison <jra at samba.org>

Pushed !

> -- 
> 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:kontakt at sernet.de

> From fcd201bcfe99562bfecfe51a6058d24008d4352b Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Tue, 12 Dec 2017 13:52:56 +0100
> Subject: [PATCH 1/2] pthreadpool: Simplify the logic in add_job a bit
> 
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
>  lib/pthreadpool/pthreadpool.c | 36 +++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/pthreadpool/pthreadpool.c b/lib/pthreadpool/pthreadpool.c
> index b70694a6a1b..2dfda38f144 100644
> --- a/lib/pthreadpool/pthreadpool.c
> +++ b/lib/pthreadpool/pthreadpool.c
> @@ -678,27 +678,33 @@ int pthreadpool_add_job(struct pthreadpool *pool, int job_id,
>  	}
>  
>  	res = pthreadpool_create_thread(pool);
> -	if (res != 0) {
> -		if (pool->num_threads == 0) {
> -			/*
> -			 * No thread could be created to run job,
> -			 * fallback to sync call.
> -			 */
> -			pthreadpool_undo_put_job(pool);
> -			pthread_mutex_unlock(&pool->mutex);
> -
> -			fn(private_data);
> -			return pool->signal_fn(job_id, fn, private_data,
> -					       pool->signal_fn_private_data);
> -		}
> +	if (res == 0) {
> +		res = pthread_mutex_unlock(&pool->mutex);
> +		assert(res == 0);
> +		return 0;
> +	}
>  
> +	if (pool->num_threads != 0) {
>  		/*
>  		 * At least one thread is still available, let
>  		 * that one run the queued job.
>  		 */
> -		res = 0;
> +		res = pthread_mutex_unlock(&pool->mutex);
> +		assert(res == 0);
> +		return 0;
>  	}
>  
> -	pthread_mutex_unlock(&pool->mutex);
> +	/*
> +	 * No thread could be created to run job, fallback to sync
> +	 * call.
> +	 */
> +	pthreadpool_undo_put_job(pool);
> +
> +	res = pthread_mutex_unlock(&pool->mutex);
> +	assert(res == 0);
> +
> +	fn(private_data);
> +	res = pool->signal_fn(job_id, fn, private_data,
> +			      pool->signal_fn_private_data);
>  	return res;
>  }
> -- 
> 2.11.0
> 
> 
> From cad890cad3c639fa4b9ce3ed72b33726700f93bf Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Tue, 12 Dec 2017 13:58:48 +0100
> Subject: [PATCH 2/2] pthreadpool: Add some asserts
> 
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
>  lib/pthreadpool/pthreadpool.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/pthreadpool/pthreadpool.c b/lib/pthreadpool/pthreadpool.c
> index 2dfda38f144..92a88c9ca84 100644
> --- a/lib/pthreadpool/pthreadpool.c
> +++ b/lib/pthreadpool/pthreadpool.c
> @@ -652,11 +652,13 @@ int pthreadpool_add_job(struct pthreadpool *pool, int job_id,
>  	 * Add job to the end of the queue
>  	 */
>  	if (!pthreadpool_put_job(pool, job_id, fn, private_data)) {
> -		pthread_mutex_unlock(&pool->mutex);
> +		res = pthread_mutex_unlock(&pool->mutex);
> +		assert(res == 0);
>  		return ENOMEM;
>  	}
>  
>  	if (pool->num_idle > 0) {
> +		int unlock_res;
>  		/*
>  		 * We have idle threads, wake one.
>  		 */
> @@ -664,7 +666,8 @@ int pthreadpool_add_job(struct pthreadpool *pool, int job_id,
>  		if (res != 0) {
>  			pthreadpool_undo_put_job(pool);
>  		}
> -		pthread_mutex_unlock(&pool->mutex);
> +		unlock_res = pthread_mutex_unlock(&pool->mutex);
> +		assert(unlock_res == 0);
>  		return res;
>  	}
>  
> @@ -673,7 +676,8 @@ int pthreadpool_add_job(struct pthreadpool *pool, int job_id,
>  		/*
>  		 * No more new threads, we just queue the request
>  		 */
> -		pthread_mutex_unlock(&pool->mutex);
> +		res = pthread_mutex_unlock(&pool->mutex);
> +		assert(res == 0);
>  		return 0;
>  	}
>  
> -- 
> 2.11.0
>