From e3fa8ebb0f45e31f77d7c5b4e34aceefe9a72a20 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Thu, 10 Feb 2022 12:10:56 -0500 Subject: [PATCH 1/6] Fix thread-safety violation in Allocations Profiler: Re-use the shared `ptls->bt_data` buffer from the thread-local storage for the buffer, to ensure that each thread has a separate buffer. This buffer is shared with the exception throwing mechanism, but is safe to share since julia exception throwing never interleaves with allocations profiling. --- src/gc-alloc-profiler.cpp | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/gc-alloc-profiler.cpp b/src/gc-alloc-profiler.cpp index a5af9031cd0e3..0ff387b870620 100644 --- a/src/gc-alloc-profiler.cpp +++ b/src/gc-alloc-profiler.cpp @@ -49,15 +49,24 @@ jl_combined_results g_combined_results; // Will live forever. // === stack stuff === jl_raw_backtrace_t get_raw_backtrace() JL_NOTSAFEPOINT { - // A single large buffer to record backtraces onto - static jl_bt_element_t static_bt_data[JL_MAX_BT_SIZE]; + // We first record the backtrace onto a MAX-sized buffer, so that we don't have to + // allocate the buffer until we know the size. To ensure thread-safety, we *re-use the + // per-thread backtrace buffer*, which is shared with Julia's exception throwing + // mechanism. This sharing is safe, because this function cannot be interleaved with + // exception throwing. + jl_ptls_t ptls = jl_current_task->ptls; + jl_bt_element_t *shared_bt_data_buffer = ptls->bt_data; - size_t bt_size = rec_backtrace(static_bt_data, JL_MAX_BT_SIZE, 2); + size_t bt_size = rec_backtrace(shared_bt_data_buffer, JL_MAX_BT_SIZE, 2); // Then we copy only the needed bytes out of the buffer into our profile. size_t bt_bytes = bt_size * sizeof(jl_bt_element_t); jl_bt_element_t *bt_data = (jl_bt_element_t*) malloc(bt_bytes); - memcpy(bt_data, static_bt_data, bt_bytes); + memcpy(bt_data, shared_bt_data_buffer, bt_bytes); + + // Now, "clear" the ptls buffer, so that this buffer isn't incorrectly rooting objects + // in that buffer. + ptls->bt_size = 0; return jl_raw_backtrace_t{ bt_data, From 581374d25ed68d073086370b60779ab36e0295f9 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Thu, 10 Feb 2022 15:14:47 -0500 Subject: [PATCH 2/6] Approach two: Create a separate per-thread allocations backtrace buffer. --- src/gc-alloc-profiler.cpp | 8 +++----- src/julia_threads.h | 2 ++ 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/gc-alloc-profiler.cpp b/src/gc-alloc-profiler.cpp index 0ff387b870620..12a88c9ec5b8f 100644 --- a/src/gc-alloc-profiler.cpp +++ b/src/gc-alloc-profiler.cpp @@ -50,12 +50,10 @@ jl_combined_results g_combined_results; // Will live forever. jl_raw_backtrace_t get_raw_backtrace() JL_NOTSAFEPOINT { // We first record the backtrace onto a MAX-sized buffer, so that we don't have to - // allocate the buffer until we know the size. To ensure thread-safety, we *re-use the - // per-thread backtrace buffer*, which is shared with Julia's exception throwing - // mechanism. This sharing is safe, because this function cannot be interleaved with - // exception throwing. + // allocate the buffer until we know the size. To ensure thread-safety, we use a + // per-thread backtrace buffer. jl_ptls_t ptls = jl_current_task->ptls; - jl_bt_element_t *shared_bt_data_buffer = ptls->bt_data; + jl_bt_element_t *shared_bt_data_buffer = ptls->profiling_bt_buffer; size_t bt_size = rec_backtrace(shared_bt_data_buffer, JL_MAX_BT_SIZE, 2); diff --git a/src/julia_threads.h b/src/julia_threads.h index 371eb51250115..22acf3aec8587 100644 --- a/src/julia_threads.h +++ b/src/julia_threads.h @@ -246,6 +246,8 @@ typedef struct _jl_tls_states_t { // Temporary backtrace buffer. Scanned for gc roots when bt_size > 0. struct _jl_bt_element_t *bt_data; // JL_MAX_BT_SIZE + 1 elements long size_t bt_size; // Size for backtrace in transit in bt_data + // Temporary backtrace buffer used only for allocations profiler. + struct _jl_bt_element_t *profiling_bt_buffer; // Atomically set by the sender, reset by the handler. volatile _Atomic(sig_atomic_t) signal_request; // TODO: no actual reason for this to be _Atomic // Allow the sigint to be raised asynchronously From e7fe3fb5868c85c2ef6be89bcf32579886fadcd1 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Wed, 16 Feb 2022 22:52:30 -0500 Subject: [PATCH 3/6] Update src/gc-alloc-profiler.cpp Co-authored-by: Jameson Nash --- src/gc-alloc-profiler.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/gc-alloc-profiler.cpp b/src/gc-alloc-profiler.cpp index 12a88c9ec5b8f..3ad55e80e9eac 100644 --- a/src/gc-alloc-profiler.cpp +++ b/src/gc-alloc-profiler.cpp @@ -54,6 +54,9 @@ jl_raw_backtrace_t get_raw_backtrace() JL_NOTSAFEPOINT { // per-thread backtrace buffer. jl_ptls_t ptls = jl_current_task->ptls; jl_bt_element_t *shared_bt_data_buffer = ptls->profiling_bt_buffer; + if (shared_bt_data_buffer == NULL) + ptls->profiling_bt_buffer = shared_bt_data_buffer = \ + malloc_s(sizeof(jl_bt_element_t) * (JL_MAX_BT_SIZE + 1)); size_t bt_size = rec_backtrace(shared_bt_data_buffer, JL_MAX_BT_SIZE, 2); From 77d401f9d87c572ee7022de4761db83892b0bf50 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Wed, 16 Feb 2022 22:54:16 -0500 Subject: [PATCH 4/6] Update src/gc-alloc-profiler.cpp Co-authored-by: Jameson Nash --- src/gc-alloc-profiler.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/gc-alloc-profiler.cpp b/src/gc-alloc-profiler.cpp index 3ad55e80e9eac..4846d7f7f7fb1 100644 --- a/src/gc-alloc-profiler.cpp +++ b/src/gc-alloc-profiler.cpp @@ -65,9 +65,6 @@ jl_raw_backtrace_t get_raw_backtrace() JL_NOTSAFEPOINT { jl_bt_element_t *bt_data = (jl_bt_element_t*) malloc(bt_bytes); memcpy(bt_data, shared_bt_data_buffer, bt_bytes); - // Now, "clear" the ptls buffer, so that this buffer isn't incorrectly rooting objects - // in that buffer. - ptls->bt_size = 0; return jl_raw_backtrace_t{ bt_data, From 123cf78d292a5d3f166bfe69c9217a3a729e112a Mon Sep 17 00:00:00 2001 From: Pete Vilter <7341+vilterp@users.noreply.github.com> Date: Fri, 18 Feb 2022 14:26:01 -0500 Subject: [PATCH 5/6] fix type error (#44235) --- src/gc-alloc-profiler.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/gc-alloc-profiler.cpp b/src/gc-alloc-profiler.cpp index 4846d7f7f7fb1..6d0af5cb9c1b6 100644 --- a/src/gc-alloc-profiler.cpp +++ b/src/gc-alloc-profiler.cpp @@ -54,9 +54,11 @@ jl_raw_backtrace_t get_raw_backtrace() JL_NOTSAFEPOINT { // per-thread backtrace buffer. jl_ptls_t ptls = jl_current_task->ptls; jl_bt_element_t *shared_bt_data_buffer = ptls->profiling_bt_buffer; - if (shared_bt_data_buffer == NULL) - ptls->profiling_bt_buffer = shared_bt_data_buffer = \ - malloc_s(sizeof(jl_bt_element_t) * (JL_MAX_BT_SIZE + 1)); + if (shared_bt_data_buffer == NULL) { + size_t size = sizeof(jl_bt_element_t) * (JL_MAX_BT_SIZE + 1); + shared_bt_data_buffer = (jl_bt_element_t*) malloc_s(size); + ptls->profiling_bt_buffer = shared_bt_data_buffer; + } size_t bt_size = rec_backtrace(shared_bt_data_buffer, JL_MAX_BT_SIZE, 2); From 42a9df232545d1fd5a8713350af8d8aa06aaaf18 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Tue, 22 Feb 2022 11:09:24 -0500 Subject: [PATCH 6/6] Update src/gc-alloc-profiler.cpp --- src/gc-alloc-profiler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gc-alloc-profiler.cpp b/src/gc-alloc-profiler.cpp index 6d0af5cb9c1b6..603eea1b0ea82 100644 --- a/src/gc-alloc-profiler.cpp +++ b/src/gc-alloc-profiler.cpp @@ -64,7 +64,7 @@ jl_raw_backtrace_t get_raw_backtrace() JL_NOTSAFEPOINT { // Then we copy only the needed bytes out of the buffer into our profile. size_t bt_bytes = bt_size * sizeof(jl_bt_element_t); - jl_bt_element_t *bt_data = (jl_bt_element_t*) malloc(bt_bytes); + jl_bt_element_t *bt_data = (jl_bt_element_t*) malloc_s(bt_bytes); memcpy(bt_data, shared_bt_data_buffer, bt_bytes);