On Sat, Dec 28, 2024 at 02:05:41PM -0500, Jeff King wrote:
> So I suspect the race is actually trickier, and that the "weird state"
> is not something that happens just while pthread_create() is being
> called, but is actually running _in the thread itself_. So even though
> pthread_create() has returned for each thread, they are still setting
> themselves up before running.
> [...]
> So a full fix would actually require synchronization where we spawn each
> thread, then wait for all of them to hit the barrier to declare
> themselves ready, and then let them all start running. There is a
> pthread_barrier type that would help with this, but we've never used it
> before (so we'd probably need to at least provide a Windows compat
> layer).
So here's a fix that seems to work, but doesn't address the portability
issues. One tweak is that the exit() call is actually not in a
sub-thread, but in the main thread itself. I don't think that really
changes the analysis too much, but it does mean we need to include the
main thread in the barrier wait.
diff --git a/builtin/grep.c b/builtin/grep.c
index d00ee76f24..933b4503b8 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -101,6 +101,9 @@ static pthread_cond_t cond_write;
/* Signalled when we are finished with everything. */
static pthread_cond_t cond_result;
+/* Synchronize the start of all threads */
+static pthread_barrier_t start_barrier;
+
static int skip_first_line;
static void add_work(struct grep_opt *opt, struct grep_source *gs)
@@ -198,6 +201,8 @@ static void *run(void *arg)
int hit = 0;
struct grep_opt *opt = arg;
+ pthread_barrier_wait(&start_barrier);
+
while (1) {
struct work_item *w = get_work();
if (!w)
@@ -229,6 +234,7 @@ static void start_threads(struct grep_opt *opt)
pthread_cond_init(&cond_add, NULL);
pthread_cond_init(&cond_write, NULL);
pthread_cond_init(&cond_result, NULL);
+ pthread_barrier_init(&start_barrier, NULL, num_threads + 1);
grep_use_locks = 1;
enable_obj_read_lock();
@@ -248,6 +254,7 @@ static void start_threads(struct grep_opt *opt)
die(_("grep: failed to create thread: %s"),
strerror(err));
}
+ pthread_barrier_wait(&start_barrier);
}
static int wait_all(void)
@@ -284,6 +291,7 @@ static int wait_all(void)
pthread_cond_destroy(&cond_add);
pthread_cond_destroy(&cond_write);
pthread_cond_destroy(&cond_result);
+ pthread_barrier_destroy(&start_barrier);
grep_use_locks = 0;
disable_obj_read_lock();
It would probably be pretty easy to do something similar for index-pack,
though really this is something I guess we'd need to do manually
anywhere we spawn worker threads.
-Peff
