Skip to content

bthread: use macros for safer thread-local access on ARM#3380

Open
altman08 wants to merge 1 commit into
apache:masterfrom
altman08:master
Open

bthread: use macros for safer thread-local access on ARM#3380
altman08 wants to merge 1 commit into
apache:masterfrom
altman08:master

Conversation

@altman08

Copy link
Copy Markdown
Contributor

Route accesses to tls_task_group, tls_task_group_nosignal, tls_bls and several mutex-profiling thread-locals (tls_inside_lock, tls_warn_up, tls_pthread_lock_count, tls_csites, tls_ever_created_keytable) through the BAIDU_(GET|SET|GET_PTR)_VOLATILE_THREAD_LOCAL macros instead of touching the raw __thread variables directly.

On aarch64/clang the compiler can incorrectly cache the address of a thread_local variable across a suspend point (e.g. when a bthread is rescheduled onto another worker), so raw accesses can silently read a stale TaskGroup*/LocalStorage. Going through the accessor macros everywhere forces a fresh, non-cached load/store on those platforms while staying a plain variable access elsewhere.

Add bthread::tls_bls_ptr() as the single helper for obtaining the current LocalStorage*, and update all call sites (bthread.cpp, butex.cpp, fd.cpp, key.cpp, mutex.cpp, task_control.cpp, brpc/controller.cpp, test/bthread_unittest.cpp) to use it instead of declaring their own stale/mistyped extern references to tls_task_group or tls_bls.

Also hardens borrow_keytable()/return_keytable() in key.cpp: the KeyTableList pointer obtained under the read lock is no longer reused after re-acquiring the write lock, since pool->list/pool->destroyed may change concurrently via bthread_keytable_pool_destroy(); the thread-local list is drained first before falling back to the pool's global free list.

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

What is changed and the side effects?

Changed:

Side effects:

  • Performance effects:

  • Breaking backward compatibility:


Check List:

Comment thread src/bthread/bthread.cpp Outdated
TaskGroup* g = tls_task_group_nosignal;
if (NULL == g) {
auto g = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_task_group_nosignal);
if (NULL == g || g->tag() != tag) {

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.

Use CHECK_EQ(g->tag(), tag) is more appropriate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Use CHECK_EQ(g->tag(), tag) is more appropriate.

fixed

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.

Pull request overview

This PR strengthens bthread thread-local (TLS) correctness on aarch64/clang by routing accesses to key TLS variables through the BAIDU_(GET|SET|GET_PTR)_VOLATILE_THREAD_LOCAL accessors, and by consolidating access to LocalStorage via a single helper.

Changes:

  • Add bthread::tls_bls_ptr() and migrate call sites away from direct __thread/raw TLS references.
  • Convert multiple TLS variables (task group pointers, mutex profiling TLS flags/counters, etc.) to use volatile TLS macros and accessor calls.
  • Harden keytable pool borrow/return logic to avoid reusing potentially stale pointers across lock upgrades.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/bthread_unittest.cpp Updates span-related TLS access to use tls_bls_ptr()
src/bthread/task_meta.h Introduces tls_bls_ptr() helper for LocalStorage TLS
src/bthread/task_group.cpp Refactors LocalStorage cleanup to use tls_bls_ptr() helper
src/bthread/task_control.cpp Routes tls_task_group initialization/reads through volatile TLS accessors
src/bthread/mutex.cpp Converts contention-profiler TLS vars to volatile TLS macros/accessors
src/bthread/key.cpp Removes raw TLS externs; uses tls_bls_ptr() and hardens keytable pool operations
src/bthread/fd.cpp Uses volatile TLS accessors for tls_task_group
src/bthread/butex.cpp Uses volatile TLS accessors for tls_task_group comparisons/reads
src/bthread/bthread.cpp Converts tls_task_group_nosignal to volatile TLS and updates access patterns
src/brpc/span.h Removes raw TLS extern declaration from header
src/brpc/controller.cpp Uses volatile TLS accessors for retry backoff logic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/bthread/bthread.cpp Outdated
Comment on lines 292 to 293
BAIDU_SET_VOLATILE_THREAD_LOCAL(tls_task_group_nosignal, NULL);
return c->choose_one_group(tag)->start_background<true>(tid, attr, fn, arg);
Comment thread src/bthread/mutex.cpp
Comment on lines +582 to +583
BAIDU_VOLATILE_THREAD_LOCAL(TLSPthreadContentionSites, tls_csites,
TLSPthreadContentionSites());
Comment thread src/bthread/bthread.cpp Outdated
}
return g->start_background<true>(tid, attr, fn, arg);
}
BAIDU_SET_VOLATILE_THREAD_LOCAL(tls_task_group_nosignal, NULL);

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.

Why add this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

CHECK_EQ(g->tag(), tag);

Why add this?

I added CHECK_EQ(g->tag(), tag) and removed this line.

@altman08 altman08 force-pushed the master branch 2 times, most recently from a43e2be to 3e4d955 Compare July 13, 2026 03:09
Route accesses to tls_task_group, tls_task_group_nosignal, tls_bls and
several mutex-profiling thread-locals (tls_inside_lock, tls_warn_up,
tls_pthread_lock_count, tls_csites, tls_ever_created_keytable) through
the BAIDU_(GET|SET|GET_PTR)_VOLATILE_THREAD_LOCAL macros instead of
touching the raw __thread variables directly.

On aarch64/clang the compiler can incorrectly cache the address of a
thread_local variable across a suspend point (e.g. when a bthread is
rescheduled onto another worker), so raw accesses can silently read a
stale TaskGroup*/LocalStorage. Going through the accessor macros
everywhere forces a fresh, non-cached load/store on those platforms
while staying a plain variable access elsewhere.

Add bthread::tls_bls_ptr() as the single helper for obtaining the
current LocalStorage*, and update all call sites (bthread.cpp,
butex.cpp, fd.cpp, key.cpp, mutex.cpp, task_control.cpp,
brpc/controller.cpp, test/bthread_unittest.cpp) to use it instead of
declaring their own stale/mistyped extern references to tls_task_group
or tls_bls.

Also hardens borrow_keytable()/return_keytable() in key.cpp: the
KeyTableList pointer obtained under the read lock is no longer reused
after re-acquiring the write lock, since pool->list/pool->destroyed
may change concurrently via bthread_keytable_pool_destroy(); the
thread-local list is drained first before falling back to the pool's
global free list.
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.

4 participants