[PATCH 2/5] pack-objects: take lock before accessing `remaining`
From: Martin Ågren <hidden>
Date: 2017-08-15 12:53:39
Subsystem:
the rest · Maintainer:
Linus Torvalds
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 <redacted> --- 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; }
--
2.14.1.151.gdfeca7a7e