Nguyễn Thái Ngọc Duy wrote:
[snipped]
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> ---
> I changed Ramsay's mutex patch a little bit and incorporate it here.
> Ramsay, it'd be great if you could try it again on MinGW
Hmm, well do you want the good news, or the bad news ... :-D
First, I should say that I feel like I'm doing a very bad job of
communicating, so let me apologize for that and hope that this time
I make a better job of it!
This patch breaks the build on MinGW, since the emulation code has not
(thus far) included an implementation of pthread_key_delete(). I simply
commented out the call to that function, in cleanup_thread(), so that I
could test the remainder of the patch.
Although this patch is an improvement on previous patches, it still fails
in *exactly* the same way as earlier attempts.
I probably didn't make clear before that 'nr_threads' has been given too
many duties, which is the main reason for me introducing a new variable
'threads_active'. For example, ...
> builtin/index-pack.c | 198 ++++++++++++++++++++++++++++++++++----
> 3 files changed, 192 insertions(+), 18 deletions(-)
>
[snipped]
> +static inline void lock_mutex(pthread_mutex_t *mutex)
> +{
> + if (nr_threads > 1)
> + pthread_mutex_lock(mutex);
> +}
What is this condition testing (ie. what does it mean)? Does it mean:
1. there are some threads currently running ?
2. the mutex variables are in a usable state ?
Does this expression always express the same invariant?
The answer, of course, is *no*.
Let us consider the call to parse_pack_objects() at line 1367. Let us
suppose that we have been asked to use threads (from the config file,
the command-line, or simply !NO_PTHREADS), so that when we call the
parse_pack_objects() function nr_threads > 1.
Note that, at this point, no threads are active and the mutex variables
have not been initialised.
Now, at the beginning of parse_pack_objects(), we find some 'first pass'
processing [for (i = 0; i < nr_objects; i++) ... lines 839-851], which
includes a call to sha1_object() at line 848. sha1_object() in turn has
an invocation of the read_lock() macro (line 552), which in turn calls
lock_mutex() with a pointer to the read_mutex.
Note that, at this point, no threads are active and the mutex variables
have not been initialised.
Also note that "nr_threads > 1" is true. At this point, nr_threads is still
playing the "this is how many threads I have been requested to create" role.
But again, no threads have been created yet, the mutex variables haven't been
initialised, and ... well, *boom*.
So, in order to get it to work on MinGW (and this time I only tested on MinGW),
I had to apply the patch below (look familiar?).
[I ran the same four tests as before, five times in a row. On *one* occasion
t5300.22 (verify-pack catches a corrupted type/size of the 1st packed object data)
failed because the 'dd' command crashed! So, maybe there is a problem lurking.]
ATB,
Ramsay Jones
-- >8 --
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 7e3b287..6679734 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -69,6 +69,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;
@@ -105,13 +106,13 @@ static pthread_key_t key;
static inline void lock_mutex(pthread_mutex_t *mutex)
{
- if (nr_threads > 1)
+ if (threads_active)
pthread_mutex_lock(mutex);
}
static inline void unlock_mutex(pthread_mutex_t *mutex)
{
- if (nr_threads > 1)
+ if (threads_active)
pthread_mutex_unlock(mutex);
}
@@ -125,14 +126,16 @@ static void init_thread(void)
pthread_mutex_init(&work_mutex, NULL);
pthread_key_create(&key, NULL);
thread_data = xcalloc(nr_threads, sizeof(*thread_data));
+ 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);
- pthread_key_delete(key);
+ /*pthread_key_delete(key);*/
nr_threads = 1;
free(thread_data);
}
-- 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