Skip to content

Fix bthread_join memory visibility with paired release/acquire on version_butex - #3538

Merged
wwbmmm merged 1 commit into
apache:masterfrom
chenBright:fix_bthread_join_fence
Sep 13, 2026
Merged

Fix bthread_join memory visibility with paired release/acquire on version_butex#3538
wwbmmm merged 1 commit into
apache:masterfrom
chenBright:fix_bthread_join_fence

Conversation

@chenBright

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: resolve #3274

Problem Summary:

The join/end handshake on version_butex was not correctly synchronized.

  • Producer (task_runner, at bthread end): bumped the version with a plain write
    ++*m->version_butex. The surrounding version_lock provides release semantics
    only to threads that also take that lock, but join() does not, so there is no release on
    version_butex for the join path.
  • Consumer (TaskGroup::join): exited its wait loop via a plain read *m->version_butex,
    with no acquire ordering.

The result is a data race with no happens-before edge, so writes the joined bthread made
before ending were not guaranteed visible after join() returned. On x86 (TSO) the hardware
masked this, but on ARM it surfaced.

PR #3276 attempted a fix by adding a lone atomic_thread_fence(acquire) after the loop. But
per the C++ memory model an acquire fence only establishes synchronization when a preceding
atomic load reads a value from a matching release operation. Here the load was a plain read and
the producer had no release store, so the fence pairs with nothing. It happens to work on ARM
only because the compiler emits a real dmb ishld, not because the model guarantees it.

What is changed and the side effects?

Changed:

This PR replaces that band-aid with a properly paired release/acquire on version_butex.

Side effects:

  • Performance effects:

  • Breaking backward compatibility:


Check List:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Resolve the remaining version_butex data race and add the requested regression test.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes bthread_join memory visibility by pairing release publication with acquire observation on version_butex.

Changes:

  • Atomically publishes version updates at task completion.
  • Uses acquire loads during joining.
  • Documents synchronization semantics.
File summaries
File Summary
src/bthread/task_meta.h Updates version synchronization documentation.
src/bthread/task_group.cpp Implements release/acquire handling; TaskGroup::exists() still has an unresolved plain-read race, and a visibility regression test is requested.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/bthread/task_group.cpp
Comment thread src/bthread/task_group.cpp

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Low-level concurrency changes warrant final human review.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@wwbmmm
wwbmmm merged commit 1b6d435 into apache:master Sep 13, 2026
25 checks passed
@chenBright
chenBright deleted the fix_bthread_join_fence branch September 13, 2026 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bthread_join lacks acquire fence on ARM, causing stale reads of joined bthread's memory writes

3 participants