On Wed, Mar 07, 2012 at 11:15:00AM -0800, Junio C Hamano wrote:
> Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxx> 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< --
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html