Re: [PATCH] builtin/index-pack.c: Fix some pthread_t misuse
From: Nguyen Thai Ngoc Duy <hidden>
Date: 2016-06-15 22:53:14
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH] builtin/index-pack.c: Fix some pthread_t misuse · Ramsay Jones <hidden>
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< --