From f7ab9dd3cd8fd09072ce991e93e3aa9c8c227818 Mon Sep 17 00:00:00 2001 From: BiteTheDDDDt Date: Wed, 2 Apr 2025 18:58:52 +0800 Subject: [PATCH 1/6] add attribute noinline to tls_bls update --- src/bthread/task_group.cpp | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/bthread/task_group.cpp b/src/bthread/task_group.cpp index 0c924ac3d8..37cf39cd1d 100644 --- a/src/bthread/task_group.cpp +++ b/src/bthread/task_group.cpp @@ -297,6 +297,17 @@ int TaskGroup::init(size_t runqueue_capacity) { return 0; } +// use noinline to avoid read wrong tls due to compiler optimization +// https://github.com/apache/brpc/issues/1776 +__attribute__((noinline)) void clean_tls_bls(TaskMeta* const m) { + KeyTable* kt = tls_bls.keytable; + if (kt != NULL) { + return_keytable(m->attr.keytable_pool, kt); + tls_bls.keytable = NULL; + m->local_storage.keytable = NULL; // optional + } +} + #ifdef BUTIL_USE_ASAN void TaskGroup::asan_task_runner(intptr_t) { // This is a new thread, and it doesn't have the fake stack yet. ASan will @@ -372,13 +383,7 @@ void TaskGroup::task_runner(intptr_t skip_remained) { // Clean tls variables, must be done before changing version_butex // otherwise another thread just joined this thread may not see side // effects of destructing tls variables. - KeyTable* kt = tls_bls.keytable; - if (kt != NULL) { - return_keytable(m->attr.keytable_pool, kt); - // After deletion: tls may be set during deletion. - tls_bls.keytable = NULL; - m->local_storage.keytable = NULL; // optional - } + clean_tls_bls(m); // During running the function in TaskMeta and deleting the KeyTable in // return_KeyTable, the group is probably changed. From 007aebdfe4f1c2ef4456e0b70bc9f6f70af6f2fa Mon Sep 17 00:00:00 2001 From: BiteTheDDDDt Date: Fri, 4 Apr 2025 11:55:22 +0800 Subject: [PATCH 2/6] update --- src/bthread/task_group.cpp | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/src/bthread/task_group.cpp b/src/bthread/task_group.cpp index 37cf39cd1d..75545ab456 100644 --- a/src/bthread/task_group.cpp +++ b/src/bthread/task_group.cpp @@ -58,7 +58,7 @@ BAIDU_VOLATILE_THREAD_LOCAL(TaskGroup*, tls_task_group, NULL); // Sync with TaskMeta::local_storage when a bthread is created or destroyed. // During running, the two fields may be inconsistent, use tls_bls as the // groundtruth. -__thread LocalStorage tls_bls = BTHREAD_LOCAL_STORAGE_INITIALIZER; +BAIDU_VOLATILE_THREAD_LOCAL(LocalStorage, tls_bls, BTHREAD_LOCAL_STORAGE_INITIALIZER); // defined in bthread/key.cpp extern void return_keytable(bthread_keytable_pool_t*, KeyTable*); @@ -79,7 +79,7 @@ void* run_create_span_func() { if (g_create_span_func) { return g_create_span_func(); } - return tls_bls.rpcz_parent_span; + return BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls).rpcz_parent_span; } int TaskGroup::get_attr(bthread_t tid, bthread_attr_t* out) { @@ -297,17 +297,6 @@ int TaskGroup::init(size_t runqueue_capacity) { return 0; } -// use noinline to avoid read wrong tls due to compiler optimization -// https://github.com/apache/brpc/issues/1776 -__attribute__((noinline)) void clean_tls_bls(TaskMeta* const m) { - KeyTable* kt = tls_bls.keytable; - if (kt != NULL) { - return_keytable(m->attr.keytable_pool, kt); - tls_bls.keytable = NULL; - m->local_storage.keytable = NULL; // optional - } -} - #ifdef BUTIL_USE_ASAN void TaskGroup::asan_task_runner(intptr_t) { // This is a new thread, and it doesn't have the fake stack yet. ASan will @@ -383,7 +372,14 @@ void TaskGroup::task_runner(intptr_t skip_remained) { // Clean tls variables, must be done before changing version_butex // otherwise another thread just joined this thread may not see side // effects of destructing tls variables. - clean_tls_bls(m); + LocalStorage* tls_bls_ptr = BAIDU_GET_PTR_VOLATILE_THREAD_LOCAL(tls_bls); + KeyTable* kt = tls_bls_ptr->keytable; + if (kt != NULL) { + return_keytable(m->attr.keytable_pool, kt); + // After deletion: tls may be set during deletion. + tls_bls_ptr->keytable = NULL; + m->local_storage.keytable = NULL; // optional + } // During running the function in TaskMeta and deleting the KeyTable in // return_KeyTable, the group is probably changed. @@ -702,8 +698,8 @@ void TaskGroup::sched_to(TaskGroup** pg, TaskMeta* next_meta, bool cur_ending) { if (__builtin_expect(next_meta != cur_meta, 1)) { g->_cur_meta = next_meta; // Switch tls_bls - cur_meta->local_storage = tls_bls; - tls_bls = next_meta->local_storage; + cur_meta->local_storage = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls); + BAIDU_SET_VOLATILE_THREAD_LOCAL(tls_bls, next_meta->local_storage); // Logging must be done after switching the local storage, since the logging lib // use bthread local storage internally, or will cause memory leak. From e5ea02ac437288b3b2f21cfafdbc294b3268d919 Mon Sep 17 00:00:00 2001 From: BiteTheDDDDt Date: Fri, 4 Apr 2025 12:06:53 +0800 Subject: [PATCH 3/6] fix compile error --- src/butil/thread_local.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/butil/thread_local.h b/src/butil/thread_local.h index 8c83947ce2..c33c7285f1 100644 --- a/src/butil/thread_local.h +++ b/src/butil/thread_local.h @@ -60,7 +60,7 @@ extern void set_##var_name(type v) #else #define BAIDU_GET_VOLATILE_THREAD_LOCAL(var_name) var_name -#define BAIDU_GET_PTR_VOLATILE_THREAD_LOCAL(var_name) &##var_name +#define BAIDU_GET_PTR_VOLATILE_THREAD_LOCAL(var_name) &var_name #define BAIDU_SET_VOLATILE_THREAD_LOCAL(var_name, value) var_name = value #define EXTERN_BAIDU_VOLATILE_THREAD_LOCAL(type, var_name) \ extern BAIDU_THREAD_LOCAL type var_name From 1a6837844b32a96a670c4297ecb313485ba6df61 Mon Sep 17 00:00:00 2001 From: BiteTheDDDDt Date: Tue, 8 Apr 2025 16:18:01 +0800 Subject: [PATCH 4/6] update --- src/bthread/task_group.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/bthread/task_group.cpp b/src/bthread/task_group.cpp index 43cdcc6174..cdddc44e17 100644 --- a/src/bthread/task_group.cpp +++ b/src/bthread/task_group.cpp @@ -372,12 +372,11 @@ void TaskGroup::task_runner(intptr_t skip_remained) { // Clean tls variables, must be done before changing version_butex // otherwise another thread just joined this thread may not see side // effects of destructing tls variables. - LocalStorage* tls_bls_ptr = BAIDU_GET_PTR_VOLATILE_THREAD_LOCAL(tls_bls); - KeyTable* kt = tls_bls_ptr->keytable; + KeyTable* kt = BAIDU_GET_PTR_VOLATILE_THREAD_LOCAL(tls_bls)->keytable; if (kt != NULL) { return_keytable(m->attr.keytable_pool, kt); // After deletion: tls may be set during deletion. - tls_bls_ptr->keytable = NULL; + BAIDU_GET_PTR_VOLATILE_THREAD_LOCAL(tls_bls)->keytable = NULL; m->local_storage.keytable = NULL; // optional } From 7a7d2933e64ba9f3e083e5cd8385bbce30ddf239 Mon Sep 17 00:00:00 2001 From: BiteTheDDDDt Date: Tue, 8 Apr 2025 16:21:25 +0800 Subject: [PATCH 5/6] update --- src/bthread/task_group.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/bthread/task_group.cpp b/src/bthread/task_group.cpp index cdddc44e17..a60ba7e99c 100644 --- a/src/bthread/task_group.cpp +++ b/src/bthread/task_group.cpp @@ -372,11 +372,13 @@ void TaskGroup::task_runner(intptr_t skip_remained) { // Clean tls variables, must be done before changing version_butex // otherwise another thread just joined this thread may not see side // effects of destructing tls variables. + LocalStorage* tls_bls_ptr = BAIDU_GET_PTR_VOLATILE_THREAD_LOCAL(tls_bls); KeyTable* kt = BAIDU_GET_PTR_VOLATILE_THREAD_LOCAL(tls_bls)->keytable; if (kt != NULL) { return_keytable(m->attr.keytable_pool, kt); // After deletion: tls may be set during deletion. - BAIDU_GET_PTR_VOLATILE_THREAD_LOCAL(tls_bls)->keytable = NULL; + tls_bls_ptr = BAIDU_GET_PTR_VOLATILE_THREAD_LOCAL(tls_bls); + tls_bls_ptr->keytable = NULL; m->local_storage.keytable = NULL; // optional } From c8d5469947177b2572c6518b0bc7f4708bd7d06d Mon Sep 17 00:00:00 2001 From: BiteTheDDDDt Date: Tue, 8 Apr 2025 17:39:40 +0800 Subject: [PATCH 6/6] update --- src/bthread/task_group.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bthread/task_group.cpp b/src/bthread/task_group.cpp index a60ba7e99c..6abc444baf 100644 --- a/src/bthread/task_group.cpp +++ b/src/bthread/task_group.cpp @@ -373,7 +373,7 @@ void TaskGroup::task_runner(intptr_t skip_remained) { // otherwise another thread just joined this thread may not see side // effects of destructing tls variables. LocalStorage* tls_bls_ptr = BAIDU_GET_PTR_VOLATILE_THREAD_LOCAL(tls_bls); - KeyTable* kt = BAIDU_GET_PTR_VOLATILE_THREAD_LOCAL(tls_bls)->keytable; + KeyTable* kt = tls_bls_ptr->keytable; if (kt != NULL) { return_keytable(m->attr.keytable_pool, kt); // After deletion: tls may be set during deletion.