Re: [PATCH] builtin/index-pack.c: Fix some pthread_t misuse

[Nguyen Thai Ngoc Duy <pclouds@gmail.com>]

On Wed, Mar 07, 2012 at 11:15:00AM -0800, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
> > However, pthread_t is intended to be an opaque (implementation defined)
> > type. For example, an implementation may choose to use a structure to
> > implement the type.  Therefore, assigning zero (or any other constant)
> > to a pthread_t is not supported in general.
> > ...
> > Note that, for the same reason given above, you can not, in general,
> > directly compare pthread_t handles with the built-in equality operator.
> > In order to compare pthread_t's for equality, the POSIX standard requires
> > the use of pthread_equal().
> 
> Thanks, the above analysis all sound sensible.
> 
> I do not think it matters in *this* case, but if a loop iterates
> over an array of things with a field of type pthread_t in it, whose
> element may or may not be valid, and wants to mark the validity of
> an element with the value of its pthread_t field, what is the proper
> way to do so?  I.e.
> 
> 	for (i = 0; i < ARRAY_SIZE(thread_data); i++) {
> 		if (pthread_invalid(thread_data[i].thread)
> 			continue; /* not used */
>         	if (!pthread_equal(self, thread_data[i].thread))
>                 	continue; /* not me */
> 		/* ah, this is mine! */
>                 ...
> 	}
> 
> Perhaps the answer is "Don't do it" and that is perfectly fine, but
> does Nguyen's code rely on the final clean-up (assignment with 0 you
> are removing with this patch) to mark that these elements are no
> longer relevant?

The 0 assignment is not strictly needed. Shortly after that,
nr_threads is set back to 1, which makes thread_data[1..]
inaccessible, and thread_data[0] is dedicated for the main thread
already.

We can get rid of pthread_t comparison by using TLS (I avoided this
because this would be the first time pthread_getspecific is used in
git).

Does this help your crash Ramsay? It also moves down cleanup_thread()
so counter_mutex is always valid when it's used. Seems to work with
linux-2.6.git.

-- 8< --
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index edd7cbd..098f350 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -104,6 +104,8 @@ static pthread_mutex_t work_mutex;
 #define work_lock()		pthread_mutex_lock(&work_mutex)
 #define work_unlock()		pthread_mutex_unlock(&work_mutex)
 
+static pthread_key_t key;
+
 /*
  * Mutex and conditional variable can't be statically-initialized on Windows.
  */
@@ -111,6 +113,7 @@ static void init_thread(void)
 {
 	init_recursive_mutex(&read_mutex);
 	pthread_mutex_init(&work_mutex, NULL);
+	pthread_key_create(&key, NULL);
 }
 
 static void cleanup_thread(void)
@@ -284,31 +287,19 @@ static NORETURN void bad_object(unsigned long offset, const char *format, ...)
 static struct thread_local *get_thread_data(void)
 {
 #ifndef NO_PTHREADS
-	int i;
-	pthread_t self = pthread_self();
-	for (i = 1; i < nr_threads; i++)
-		if (self == thread_data[i].thread)
-			return &thread_data[i];
+	if (nr_threads > 1)
+		return pthread_getspecific(key);
 #endif
+	assert(nr_threads == 1 &&
+	       "This should only be reached when all threads are gone");
 	return &thread_data[0];
 }
 
 static void resolve_one_delta(void)
 {
-#ifndef NO_PTHREADS
-	int i;
-	pthread_t self = pthread_self();
-	for (i = 1; i < nr_threads; i++)
-		if (self == thread_data[i].thread) {
-			counter_lock();
-			nr_resolved_deltas++;
-			counter_unlock();
-			return;
-		}
-#endif
-	assert(nr_threads == 1 &&
-	       "This should only be reached when all threads are gone");
+	counter_lock();
 	nr_resolved_deltas++;
+	counter_unlock();
 }
 
 static struct base_data *alloc_base_data(void)
@@ -796,6 +787,7 @@ static void second_pass(struct object_entry *obj)
 
 static void *threaded_second_pass(void *arg)
 {
+	pthread_setspecific(key, arg);
 	for (;;) {
 		int i, nr = 16;
 		work_lock();
@@ -883,15 +875,12 @@ static void parse_pack_objects(unsigned char *sha1)
 		init_thread();
 		for (i = 1; i < nr_threads; i++) {
 			int ret = pthread_create(&thread_data[i].thread, NULL,
-						 threaded_second_pass, NULL);
+						 threaded_second_pass, thread_data + i);
 			if (ret)
 				die("unable to create thread: %s", strerror(ret));
 		}
-		for (i = 1; i < nr_threads; i++) {
+		for (i = 1; i < nr_threads; i++)
 			pthread_join(thread_data[i].thread, NULL);
-			thread_data[i].thread = 0;
-		}
-		cleanup_thread();
 
 		/* stop get_thread_data() from looking up beyond the
 		   first item, when fix_unresolved_deltas() runs */
@@ -1411,6 +1400,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 			die("pack has %d unresolved deltas",
 			    nr_deltas - nr_resolved_deltas);
 	}
+#ifndef NO_PTHREADS
+	cleanup_thread();
+#endif
 	free(deltas);
 	if (strict)
 		check_objects();
-- 8< --