From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 19 Mar 2013 15:53:30 GMT
Subject: Re: [PATCH v2] index-pack: guard nr_resolved_deltas reads by lock
Message-ID: <7vsj3rjshx.fsf@alter.siamese.dyndns.org>
In-Reply-To: <8ddf4db38f33034b5ebf504a18948bccf841ab72.1363702423.git.trast@student.ethz.ch>

Thomas Rast <trast@student.ethz.ch> writes:

> The threaded parts of index-pack increment the number of resolved
> deltas in nr_resolved_deltas guarded by counter_mutex.  However, the
> per-thread outer loop accessed nr_resolved_deltas without any locks.
>
> This is not wrong as such, since it doesn't matter all that much
> whether we get an outdated value.  However, unless someone proves that
> this one lock makes all the performance difference, it would be much
> cleaner to guard _all_ accesses to the variable with the lock.
>
> The only such use is display_progress() in the threaded section (all
> others are in the conclude_pack() callchain outside the threaded
> part).  To make it obvious that it cannot deadlock, move it out of
> work_mutex.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>
>> The only thing I don't
>> like here is the double locking (work_lock then counter_lock) is an
>> invitation for potential deadlocks (not now, but who now what can
>> change later). I think you could move work_lock(); down after
>> counter_unlock() so we hold one lock at a time.
>
> Good point.

Thanks guys for fixing my mess with these two patches.