git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 3/5] thread-utils: introduce optional barrier type

From
Jeff King <peff@peff.net>
Date
Dec 30, 2024, 04:28 UTC
Message-ID
<20241230042830.GC113400@coredump.intra.peff.net>
In-Reply-To
<20241230042325.GA112439@coredump.intra.peff.net>

One thread primitive we don't yet support is a barrier: it waits for all threads to reach a synchronization point before letting any of them continue. This would be useful for avoiding the LSan race we see in index-pack (and other places) by having all threads complete their initialization before any of them start to do real work.

POSIX introduced a pthread_barrier_t in 2004, which does what we want. But if we want to rely on it:

  1. Our Windows pthread emulation would need a new set of wrapper
     functions. There's a Synchronization Barrier primitive there, which
     was introduced in Windows 8 (which is old enough for us to depend
     on).
  2. macOS (and possibly other systems) has pthreads but not
     pthread_barrier_t. So there we'd have to implement our own barrier
     based on the mutex and cond primitives.

Those are do-able, but since we only care about avoiding races in our LSan builds, there's an easier way: make it a noop on systems without a native pthread barrier.

This patch introduces a "maybe_thread_barrier" API. The clunky name (rather than just using pthread_barrier directly) should hopefully clue people in that on some systems it will do nothing. It's wired to a Makefile knob which has to be triggered manually, and we enable it for the linux-leaks CI jobs (since we know we'll have it there).

There are some other possible options:
  - we could turn it on all the time for Linux systems based on uname.
    But we really only care about it for LSan builds, and there is no
    need to add extra code to regular builds.
  - we could turn it on only for LSan builds. But that would break
    builds on non-Linux platforms (like macOS) that otherwise should
    support sanitizers.
  - we could trigger only on the combination of Linux and LSan together.
    This isn't too hard to do, but the uname check isn't completely
    accurate. It is really about what your libc supports, and non-glibc
    systems might not have it (though at least musl seems to).
    So we'd risk breaking builds on those systems, which would need to
    add a new knob. Though the upside would be that running local "make
    SANITIZE=leak test" would be protected automatically.

And of course none of this protects LSan runs from races on systems without pthread barriers. It's probably OK in practice to protect only our CI jobs, though. The race is rare-ish and most leak-checking happens through CI.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile       |  7 +++++++
 ci/lib.sh      |  1 +
 thread-utils.h | 17 +++++++++++++++++
 3 files changed, 25 insertions(+)
diff --git a/Makefile b/Makefile
index 97e8385b66..2c6dad8a75 100644
--- a/Makefile
+++ b/Makefile
@@ -141,6 +141,10 @@ include shared.mak
 #
 # Define NO_PTHREADS if you do not have or do not want to use Pthreads.
 #
+# Define THREAD_BARRIER_PTHREAD if your system has pthread_barrier_t. Barrier
+# support is optional and is only helpful when building with SANITIZE=leak, as
+# it is used to eliminate some races in the leak-checker.
+#
 # Define NO_PREAD if you have a problem with pread() system call (e.g.
 # cygwin1.dll before v1.5.22).
 #
@@ -2079,6 +2083,9 @@ ifdef NO_PTHREADS
 else
 	BASIC_CFLAGS += $(PTHREAD_CFLAGS)
 	EXTLIBS += $(PTHREAD_LIBS)
+	ifdef THREAD_BARRIER_PTHREAD
+		BASIC_CFLAGS += -DTHREAD_BARRIER_PTHREAD
+	endif
 endif
 
 ifdef HAVE_PATHS_H
diff --git a/ci/lib.sh b/ci/lib.sh
index 8885ee3c3f..6a1267fbcb 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -385,6 +385,7 @@ linux-musl)
 	;;
 linux-leaks|linux-reftable-leaks)
 	export SANITIZE=leak
+	export THREAD_BARRIER_PTHREAD=1
 	;;
 linux-asan-ubsan)
 	export SANITIZE=address,undefined
diff --git a/thread-utils.h b/thread-utils.h
index 4961487ed9..3df5be9916 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -53,5 +53,22 @@ int dummy_pthread_init(void *);
 int online_cpus(void);
 int init_recursive_mutex(pthread_mutex_t*);
 
+#ifdef THREAD_BARRIER_PTHREAD
+#define maybe_thread_barrier_t pthread_barrier_t
+#define maybe_thread_barrier_init pthread_barrier_init
+#define maybe_thread_barrier_wait pthread_barrier_wait
+#define maybe_thread_barrier_destroy pthread_barrier_destroy
+#else
+#define maybe_thread_barrier_t int
+static inline int maybe_thread_barrier_init(maybe_thread_barrier_t *b UNUSED,
+					    void *attr UNUSED,
+					    unsigned nr UNUSED)
+{
+	errno = ENOSYS;
+	return -1;
+}
+#define maybe_thread_barrier_wait(barrier)
+#define maybe_thread_barrier_destroy(barrier)
+#endif
 
 #endif /* THREAD_COMPAT_H */
-- 
bar
Previous: Jeff KingNext: Patrick Steinhardt
Message 6 of 11 in “fixing thread races in linux-leaks CI”
  1. 0/5 fixing thread races in linux-leaks CIJeff King, Dec 30, 2024
  2. 1/5 test-lib: use individual lsan dir for --stress runsJeff King, Dec 30, 2024
  3. Jeff KingDec 30, 2024
  4. Junio C HamanoDec 30, 2024
  5. 2/5 Revert "index-pack: spawn threads atomically"Jeff King, Dec 30, 2024
  6. 3/5 thread-utils: introduce optional barrier typeJeff King, Dec 30, 2024
  7. Patrick SteinhardtDec 30, 2024
  8. Jeff KingJan 1, 2025
  9. Patrick SteinhardtJan 3, 2025
  10. 4/5 index-pack: work around LSan threading race with barrierJeff King, Dec 30, 2024
  11. 5/5 grep: work around LSan threading race with barrierJeff King, Dec 30, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.