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

Ramsay Jones [off-list ref] 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?

On Wed, Mar 07, 2012 at 11:15:00AM -0800, Junio C Hamano wrote:

Ramsay Jones [off-list ref] writes:

quoted

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

Nguyen Thai Ngoc Duy wrote:

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.

[ patch snipped ]

No this does not help, because it doesn't address the fundamental problem.

The problem is simply that mutex variables are being used when they are
not in a usable state; either they are not initialised at all (counter_mutex),
or they are being used *before* they are initialised (in init_thread()) or
*after* they are destroyed (in cleanup_thread()).

If I apply the patch below on top of your patch (it should look familiar!),
then everything works fine. (This time I only tested on MinGW and cygwin).

Also, I would move the call to cleanup_thread() back up to it's original
location, since the move down serves no useful purpose.

I don't know that using TLS here is an improvement or not (it's probably a
wash). ;-)

[BTW, using an uninitialised mutex on cygwin and Linux *seems* to work without
problem, but I can't be sure that's true (I haven't looked at the code). :-P ]

ATB,
Ramsay Jones

-- 8< --

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 098f350..31f923c 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c

@@ -74,6 +74,7 @@ static int nr_processed;
 static int nr_deltas;
 static int nr_resolved_deltas;
 static int nr_threads;
+static int threads_active;
 
 static int from_stdin;
 static int strict;

@@ -93,16 +94,16 @@ static int input_fd, output_fd, pack_fd;
 #ifndef NO_PTHREADS
 
 static pthread_mutex_t read_mutex;
-#define read_lock()		pthread_mutex_lock(&read_mutex)
-#define read_unlock()		pthread_mutex_unlock(&read_mutex)
+#define read_lock()		if (threads_active) pthread_mutex_lock(&read_mutex)
+#define read_unlock()		if (threads_active) pthread_mutex_unlock(&read_mutex)
 
 static pthread_mutex_t counter_mutex;
-#define counter_lock()		pthread_mutex_lock(&counter_mutex)
-#define counter_unlock()		pthread_mutex_unlock(&counter_mutex)
+#define counter_lock()		if (threads_active) pthread_mutex_lock(&counter_mutex)
+#define counter_unlock()	if (threads_active) pthread_mutex_unlock(&counter_mutex)
 
 static pthread_mutex_t work_mutex;
-#define work_lock()		pthread_mutex_lock(&work_mutex)
-#define work_unlock()		pthread_mutex_unlock(&work_mutex)
+#define work_lock()		if (threads_active) pthread_mutex_lock(&work_mutex)
+#define work_unlock()		if (threads_active) pthread_mutex_unlock(&work_mutex)
 
 static pthread_key_t key;
 

@@ -112,13 +113,17 @@ static pthread_key_t key;
 static void init_thread(void)
 {
 	init_recursive_mutex(&read_mutex);
+	pthread_mutex_init(&counter_mutex, NULL);
 	pthread_mutex_init(&work_mutex, NULL);
 	pthread_key_create(&key, NULL);
+	threads_active = 1;
 }
 
 static void cleanup_thread(void)
 {
+	threads_active = 0;
 	pthread_mutex_destroy(&read_mutex);
+	pthread_mutex_destroy(&counter_mutex);
 	pthread_mutex_destroy(&work_mutex);
 }
 
-- 8< --