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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
---
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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux