From: Ramsay Jones <ramsay@ramsay1•demon.co.uk>
To: Nguyen Thai Ngoc Duy <pclouds@gmail•com>
Cc: Junio C Hamano <gitster@pobox•com>,
GIT Mailing-list <git@vger•kernel.org>
Subject: Re: [PATCH] builtin/index-pack.c: Fix some pthread_t misuse
Date: Sat, 10 Mar 2012 21:20:58 +0000 [thread overview]
Message-ID: <4F5BC5BA.9000404@ramsay1.demon.co.uk> (raw)
In-Reply-To: <20120308012953.GA8444@duynguyen-vnpc.dek-tpc.internal>
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< --
prev parent reply other threads:[~2012-03-11 18:33 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-03-07 19:00 [PATCH] builtin/index-pack.c: Fix some pthread_t misuse Ramsay Jones
2012-03-07 19:15 ` Junio C Hamano
2012-03-08 1:29 ` Nguyen Thai Ngoc Duy
2012-03-10 21:20 ` Ramsay Jones [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F5BC5BA.9000404@ramsay1.demon.co.uk \
--to=ramsay@ramsay1$(echo .)demon.co.uk \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=pclouds@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox