Fix optimistic locking under weak memory ordering - #2581
Conversation
- `BTree.h:1246` dereferences `next` before the parent lease is validated. - During concurrent insert/split, the reader can compute `idx` from one snapshot and read `children[idx]` from a partially visible later snapshot. - On aarch64, weaker store visibility makes this much more plausible: `numElements` can become observable before the matching child slot is safely observable to a stale optimistic reader. - `OptimisticReadWriteLock` is also too weak for this seqlock-style usage, especially around validation and writer publication.
- takes a root lease before reading root - takes a node lease before reading keys, numElements, and children - validates the node lease before returning found/not-found - validates the parent after taking the child lease - restarts if a child pointer is null from an invalidated snapshot
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2581 +/- ##
==========================================
- Coverage 81.25% 81.23% -0.02%
==========================================
Files 492 492
Lines 31009 31047 +38
==========================================
+ Hits 25195 25222 +27
- Misses 5814 5825 +11
🚀 New features to boost your workflow:
|
85e5fd0 to
ecb775c
Compare
| * referencing its position. If not found, an end-iterator will be returned. | ||
| */ | ||
| iterator find(const Key& k, operation_hints& hints) const { | ||
| #ifdef IS_PARALLEL |
There was a problem hiding this comment.
I believe this method is not intended to be thread-safe. It is used when no write operations are in progress, or at least was intended to be. EqRel uses BTree out of the expected environment, and is not protecting it from modification.
I think it would be better to modify EqRel instead, and leave Btree.find() as is - not thread-safe, but fast. I explored this with mmcgr@47e8558
which seems to work.
| if (cur->lock.validate(cur_lease)) { | ||
| assert(false && "B-tree inner node has null child"); | ||
| } |
There was a problem hiding this comment.
Maybe simplify these to
| if (cur->lock.validate(cur_lease)) { | |
| assert(false && "B-tree inner node has null child"); | |
| } | |
| assert(!cur->lock.validate(cur_lease) && "B-tree inner node has null child"); |
|
|
||
| // set last bit => make it odd | ||
| auto v = version.fetch_or(0x1, std::memory_order_acquire); | ||
| auto v = version.fetch_or(0x1, std::memory_order_acq_rel); |
There was a problem hiding this comment.
Are the memory order changes necessary? They would have protected against the missing nullptr check in BTree, but that wasn't a problem with the lock itself. Adding the nullptr check is enough to avoid the crashes.
I'm not entirely confident that I've understood the necessary ordering when we're doing optimistic writing, so I'm also fine with the change as it makes it easier to be clear on the order of the operations.
2 participants
Reproduced the crash described in #2476 on Apple Mac mini M2 on macOS Sequoia 15.6.
Test
cprog4only use b-tree, no symbol table nor record table, so the issue is definitely not inConcurrentFlyweightas proposed in the issue description.I built
souffleandcprog4inRelWithDebugInfomode. Then runcprog4in a loop until it crashed and created a core dump:With
lldbpointing to the crashing location inBTree.hI used gpt-5.5 to investigate possible concurrency issues in Souffle's b-tree and optimistic RW lock implementations, under the assumption of aarch64 weak memory consistency. It created a first fix.Then I found a similar sporadic crash in
test_binary_relation, againlldbhelped me pinpointing the exact location of the crash in thebtree::findfunction and gpt-5.5 created a second fix.Both fixes were validated on the same Apple Mac mini M2.
Fix #2476