Skip to content

Commit 413a8d2

Browse files
authored
fix(avm): avoid data race on shared interaction selector writes (port #24456 to v5-next) (#24457)
Ports #24456 to `v5-next`. ## What was wrong The interactions tracegen phase runs every lookup/permutation job concurrently: `AvmTraceGenHelper::fill_trace_interactions` concatenates all builders' jobs and dispatches them with `parallel_for`. Lookups whose fine-grained destination selector is a *shared* column (`DST_SELECTOR != outer_dst_selector`) can resolve to the same `dst_row` from different jobs, so multiple threads write the same `(selector, row)` cell at once. The previous code did a guarded read-modify-write on that shared cell: ```cpp if (DST_SELECTOR != outer_dst_selector && trace.get(DST_SELECTOR, dst_row) != 1) { trace.set(DST_SELECTOR, dst_row, 1); } ``` Both the `get` and the non-atomic 32-byte `set` race against the other threads' writes to the same cell — a data race, i.e. undefined behavior. In practice it produced the right value (every writer stores `1`), but it is still UB. ## The fix Add an opt-in `use_atomic_limbs` flag to `TraceContainer::set`. When set, the field's four 64-bit limbs are written with **relaxed atomic stores** — four plain `movq` on x86-64, no lock and no libatomic call. (A whole-field `std::atomic_ref<FF>` is *not* an option on the hot path: at 32 bytes it exceeds the hardware lock-free width and falls back to a locked libatomic call.) The shared selector write now uses it and drops the guard read: ```cpp if (DST_SELECTOR != outer_dst_selector) { trace.set(DST_SELECTOR, dst_row, 1, /*use_atomic_limbs=*/true); } ``` The write is an unconditional, idempotent atomic store of `1`. Because every concurrent writer stores the *same* value, per-limb atomicity is sufficient — no torn value is possible — so this is data-race-free without the cost of whole-field atomicity. The default (non-atomic) `set` hot path is untouched and keeps its sparse-column "zero = absent" fast path. ## Performance (this PR vs baseline) Mega bulk AVM tx, full proving, `HARDWARE_CONCURRENCY=16`. Tracegen stage timings, median of runs (baseline n=3, this PR n=6): | Stage | baseline | this PR | |---|---|---| | tracegen traces | 1,536 ms | 1,537 ms | | tracegen interactions | 350 ms | 329 ms | | tracegen all | 1,917 ms | 1,936 ms | No measurable cost — the safety fix is free. The per-limb atomic only fires on the shared selector writes in the interactions stage; the traces stage is byte-for-byte the same hot path as baseline.
2 parents c54416a + 44ad67c commit 413a8d2

3 files changed

Lines changed: 41 additions & 14 deletions

File tree

barretenberg/cpp/src/barretenberg/vm2/tracegen/lib/lookup_builder.hpp

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,10 @@ template <typename LookupSettings_> class IndexedLookupTraceBuilder : public Int
5151
}
5252

5353
trace.set(LookupSettings::COUNTS, dst_row, trace.get(LookupSettings::COUNTS, dst_row) + 1);
54-
// Set the fine grained inner selector if it's not already one.
55-
if (LookupSettings::DST_SELECTOR != this->outer_dst_selector &&
56-
trace.get(LookupSettings::DST_SELECTOR, dst_row) != 1) {
57-
trace.set(LookupSettings::DST_SELECTOR, dst_row, 1);
54+
if (LookupSettings::DST_SELECTOR != this->outer_dst_selector) {
55+
// This step might write to the same cell from multiple threads, so we use atomic limbs to avoid UB.
56+
// Since we are always writing a 1, the end result will be 1 even under concurrency.
57+
trace.set(LookupSettings::DST_SELECTOR, dst_row, 1, /*use_atomic_limbs=*/true);
5858
}
5959
});
6060
}
@@ -198,10 +198,10 @@ template <typename LookupSettings> class LookupIntoDynamicTableSequential : publ
198198
if (dst_selector == 1 && src_values == trace.get_multiple(LookupSettings::DST_COLUMNS, dst_row)) {
199199
trace.set(LookupSettings::COUNTS, dst_row, trace.get(LookupSettings::COUNTS, dst_row) + 1);
200200

201-
// Set the fine grained inner selector if it's not already one.
202-
if (LookupSettings::DST_SELECTOR != this->outer_dst_selector &&
203-
trace.get(LookupSettings::DST_SELECTOR, dst_row) != 1) {
204-
trace.set(LookupSettings::DST_SELECTOR, dst_row, 1);
201+
if (LookupSettings::DST_SELECTOR != this->outer_dst_selector) {
202+
// This step might write to the same cell from multiple threads, so we use atomic limbs to avoid
203+
// UB. Since we are always writing a 1, the end result will be 1 even under concurrency.
204+
trace.set(LookupSettings::DST_SELECTOR, dst_row, 1, /*use_atomic_limbs=*/true);
205205
}
206206

207207
found = true;

barretenberg/cpp/src/barretenberg/vm2/tracegen/trace_container.cpp

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
#include <ranges>
55

66
#include "barretenberg/common/assert.hpp"
7+
#include "barretenberg/common/compiler_hints.hpp"
78
#include "barretenberg/common/log.hpp"
89
#include "barretenberg/common/ref_vector.hpp"
910
#include "barretenberg/vm2/common/field.hpp"
@@ -13,7 +14,19 @@ namespace bb::avm2::tracegen {
1314
namespace {
1415

1516
// We need a zero value to return (a reference to) when a value is not found.
16-
static const FF zero = FF::zero();
17+
const FF zero = FF::zero();
18+
19+
// Writes each 64-bit limb of the field with a relaxed atomic store. Each limb is naturally aligned (FF is
20+
// alignas(32), 4x uint64_t), so this lowers to 4 plain `movq` stores on x86-64 — no lock, no libatomic call
21+
// (unlike a whole-field std::atomic_ref<FF>, whose 32 bytes exceed the lock-free width). Lets set() make a
22+
// same-cell concurrent write data-race-free when every writer stores the same value.
23+
inline void store_per_limb(FF& cell, const FF& value)
24+
{
25+
static_assert(sizeof(FF) == 4 * sizeof(uint64_t));
26+
for (size_t i = 0; i < 4; ++i) {
27+
std::atomic_ref<uint64_t>(cell.data[i]).store(value.data[i], std::memory_order_relaxed);
28+
}
29+
}
1730

1831
} // namespace
1932

@@ -62,7 +75,7 @@ TraceContainer::ColumnInterval& TraceContainer::get_or_create_shard(SparseColumn
6275
return *expected; // CAS failure loaded the winning pointer into `expected` (acquire).
6376
}
6477

65-
void TraceContainer::set(Column col, uint32_t row, const FF& value)
78+
void TraceContainer::set(Column col, uint32_t row, const FF& value, bool use_atomic_limbs)
6679
{
6780
auto& column_data = (*trace)[static_cast<size_t>(col)];
6881
const size_t shard_idx = row / INTERVAL_SIZE;
@@ -73,12 +86,22 @@ void TraceContainer::set(Column col, uint32_t row, const FF& value)
7386
// Lock-free: a single atomic load finds the shard (created on first write), then we write our
7487
// own dense cell directly. Different rows are distinct array elements, so concurrent writers of
7588
// this column (or even of the same shard, at a chunk boundary) never race and never serialize.
76-
get_or_create_shard(column_data, shard_idx).rows[offset] = value;
89+
auto& cell = get_or_create_shard(column_data, shard_idx).rows[offset];
90+
if (BB_UNLIKELY(use_atomic_limbs)) {
91+
store_per_limb(cell, value);
92+
} else {
93+
cell = value;
94+
}
7795
} else {
78-
// Zero value: clear if present. We never create a shard (clearing an absent row is a no-op).
96+
// Zero value: clear if present. We never create a shard, so sparse (mostly-zero) columns are not
97+
// materialized (an unset cell already reads as zero).
7998
ColumnInterval* shard = column_data.slots[shard_idx].load(std::memory_order_acquire);
8099
if (shard != nullptr) {
81-
shard->rows[offset] = FF::zero();
100+
if (BB_UNLIKELY(use_atomic_limbs)) {
101+
store_per_limb(shard->rows[offset], zero);
102+
} else {
103+
shard->rows[offset] = FF::zero();
104+
}
82105
}
83106
}
84107
}

barretenberg/cpp/src/barretenberg/vm2/tracegen/trace_container.hpp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,11 @@ class TraceContainer {
7676
// Extended version of get that works with shifted columns. More expensive.
7777
const FF& get_column_or_shift(ColumnAndShifts col, uint32_t row) const;
7878

79-
void set(Column col, uint32_t row, const FF& value);
79+
// Sets the value of a cell. Thread-safe if the same cell is not written to from multiple threads.
80+
// If writing to the same cell from multiple threads, use_atomic_limbs=true to use atomic limbs.
81+
// This makes the write slower, but it will not be UB. However, it is also not thread-safe.
82+
// Use only if you know what you are doing.
83+
void set(Column col, uint32_t row, const FF& value, bool use_atomic_limbs = false);
8084
// Bulk setting for a given row.
8185
void set(uint32_t row, std::span<const std::pair<Column, FF>> values);
8286
// Reserve column size. Useful for precomputed columns.

0 commit comments

Comments
 (0)