Re: [PATCH 2/5] pack-objects: take lock before accessing `remaining`

[Johannes Sixt <j6t@kdbg.org>]

Am 15.08.2017 um 14:53 schrieb Martin Ågren:
> When checking the conditional of "while (me->remaining)", we did not
> hold the lock. Calling find_deltas would still be safe, since it checks
> "remaining" (after taking the lock) and is able to handle all values. In
> fact, this could (currently) not trigger any bug: a bug could happen if
> `remaining` transitioning from zero to non-zero races with the evaluation
> of the while-condition, but these are always separated by the
> data_ready-mechanism.
> 
> Make sure we have the lock when we read `remaining`. This does mean we
> release it just so that find_deltas can take it immediately again. We
> could tweak the contract so that the lock should be taken before calling
> find_deltas, but let's defer that until someone can actually show that
> "unlock+lock" has a measurable negative impact.
> 
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> I don't think this corrects any real error. The benefits of this patch
> would be "future-proofs things slightly" and "silences tsan, so that
> other errors don't drown in noise". Feel free to tell me those benefits
> are negligible and that this change actually hurts.
> 
>   builtin/pack-objects.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index c753e9237..bd391e97a 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -2170,7 +2170,10 @@ static void *threaded_find_deltas(void *arg)
>   {
>   	struct thread_params *me = arg;
>   
> +	progress_lock();
>   	while (me->remaining) {
> +		progress_unlock();
> +
>   		find_deltas(me->list, &me->remaining,
>   			    me->window, me->depth, me->processed);
>   
> @@ -2192,7 +2195,10 @@ static void *threaded_find_deltas(void *arg)
>   			pthread_cond_wait(&me->cond, &me->mutex);
>   		me->data_ready = 0;
>   		pthread_mutex_unlock(&me->mutex);
> +
> +		progress_lock();
>   	}
> +	progress_unlock();
>   	/* leave ->working 1 so that this doesn't get more work assigned */
>   	return NULL;
>   }
> 

It is correct that this access of me->remaining requires a lock. It 
could be solved in the way you did. I tried to do it differently, but 
all cleaner solutions that I can think of are overkill...

-- Hannes