From b339a7b5b0017c46c19115614730245e54403a13 Mon Sep 17 00:00:00 2001 From: MJY-HUST Date: Thu, 23 May 2024 02:01:52 +0800 Subject: [PATCH 1/8] update --- src/bthread/key.cpp | 100 ++++++++++++++++++++++------------ src/bthread/types.h | 11 +++- test/bthread_key_unittest.cpp | 7 +-- 3 files changed, 77 insertions(+), 41 deletions(-) diff --git a/src/bthread/key.cpp b/src/bthread/key.cpp index 8f850f7f63..e6e0385e71 100644 --- a/src/bthread/key.cpp +++ b/src/bthread/key.cpp @@ -22,6 +22,7 @@ #include #include "butil/macros.h" #include "butil/atomicops.h" +#include "butil/thread_key.h" #include "bvar/passive_status.h" #include "bthread/errno.h" // EAGAIN #include "bthread/task_group.h" // TaskGroup @@ -204,14 +205,54 @@ class BAIDU_CACHELINE_ALIGNMENT KeyTable { SubKeyTable* _subs[KEY_1STLEVEL_SIZE]; }; +class KeyTableList { +public: + KeyTableList() { + keytable = NULL; + } + ~KeyTableList() { + bthread::TaskGroup* const g = bthread::tls_task_group; + bthread::KeyTable* old_kt = bthread::tls_bls.keytable; + while (keytable) { + bthread::KeyTable* kt = keytable; + keytable = kt->next; + bthread::tls_bls.keytable = kt; + if (g) { + g->current_task()->local_storage.keytable = kt; + } + delete kt; + if (old_kt == kt) { + old_kt = NULL; + } + } + bthread::tls_bls.keytable = old_kt; + if(g) { + g->current_task()->local_storage.keytable = old_kt; + } + } + KeyTable* keytable; +}; + static KeyTable* borrow_keytable(bthread_keytable_pool_t* pool) { - if (pool != NULL && pool->free_keytables) { - BAIDU_SCOPED_LOCK(pool->mutex); - KeyTable* p = (KeyTable*)pool->free_keytables; + if (pool != NULL && (pool->list->get()->keytable || pool->free_keytables)) { + pthread_rwlock_rdlock(&pool->rwlock); + KeyTable* p = pool->list->get()->keytable; if (p) { - pool->free_keytables = p->next; + pool->list->get()->keytable = p->next; + pthread_rwlock_unlock(&pool->rwlock); return p; } + pthread_rwlock_unlock(&pool->rwlock); + if (pool->free_keytables) { + pthread_rwlock_wrlock(&pool->rwlock); + p = (KeyTable*)pool->free_keytables; + if (p) { + pool->free_keytables = p->next; + pthread_rwlock_unlock(&pool->rwlock); + return p; + } + pthread_rwlock_unlock(&pool->rwlock); + } } return NULL; } @@ -226,14 +267,15 @@ void return_keytable(bthread_keytable_pool_t* pool, KeyTable* kt) { delete kt; return; } - std::unique_lock mu(pool->mutex); + pthread_rwlock_rdlock(&pool->rwlock); if (pool->destroyed) { - mu.unlock(); + pthread_rwlock_unlock(&pool->rwlock); delete kt; return; } - kt->next = (KeyTable*)pool->free_keytables; - pool->free_keytables = kt; + kt->next = pool->list->get()->keytable; + pool->list->get()->keytable = kt; + pthread_rwlock_unlock(&pool->rwlock); } static void cleanup_pthread(void* arg) { @@ -279,7 +321,8 @@ int bthread_keytable_pool_init(bthread_keytable_pool_t* pool) { LOG(ERROR) << "Param[pool] is NULL"; return EINVAL; } - pthread_mutex_init(&pool->mutex, NULL); + pthread_rwlock_init(&pool->rwlock, NULL); + pool->list = new butil::ThreadLocal(); pool->free_keytables = NULL; pool->destroyed = 0; return 0; @@ -291,33 +334,18 @@ int bthread_keytable_pool_destroy(bthread_keytable_pool_t* pool) { return EINVAL; } bthread::KeyTable* saved_free_keytables = NULL; - { - BAIDU_SCOPED_LOCK(pool->mutex); - if (pool->free_keytables) { - saved_free_keytables = (bthread::KeyTable*)pool->free_keytables; - pool->free_keytables = NULL; - } - pool->destroyed = 1; - } - // Cheat get/setspecific and destroy the keytables. - bthread::TaskGroup* const g = bthread::tls_task_group; - bthread::KeyTable* old_kt = bthread::tls_bls.keytable; - while (saved_free_keytables) { + pthread_rwlock_wrlock(&pool->rwlock); + pool->destroyed = 1; + delete pool->list; + saved_free_keytables = (bthread::KeyTable*)pool->free_keytables; + pool->free_keytables = NULL; + pthread_rwlock_unlock(&pool->rwlock); + while(saved_free_keytables) { bthread::KeyTable* kt = saved_free_keytables; saved_free_keytables = kt->next; - bthread::tls_bls.keytable = kt; - if (g) { - g->current_task()->local_storage.keytable = kt; - } delete kt; - if (old_kt == kt) { - old_kt = NULL; - } - } - bthread::tls_bls.keytable = old_kt; - if (g) { - g->current_task()->local_storage.keytable = old_kt; } + return 0; // TODO: return_keytable may race with this function, we don't destroy // the mutex right now. // pthread_mutex_destroy(&pool->mutex); @@ -330,11 +358,12 @@ int bthread_keytable_pool_getstat(bthread_keytable_pool_t* pool, LOG(ERROR) << "Param[pool] or Param[stat] is NULL"; return EINVAL; } - std::unique_lock mu(pool->mutex); + pthread_rwlock_wrlock(&pool->rwlock); size_t count = 0; bthread::KeyTable* p = (bthread::KeyTable*)pool->free_keytables; for (; p; p = p->next, ++count) {} stat->nfree = count; + pthread_rwlock_unlock(&pool->rwlock); return 0; } @@ -365,14 +394,15 @@ void bthread_keytable_pool_reserve(bthread_keytable_pool_t* pool, kt->set_data(key, data); } // else append kt w/o data. - std::unique_lock mu(pool->mutex); + pthread_rwlock_wrlock(&pool->rwlock); if (pool->destroyed) { - mu.unlock(); + pthread_rwlock_unlock(&pool->rwlock); delete kt; break; } kt->next = (bthread::KeyTable*)pool->free_keytables; pool->free_keytables = kt; + pthread_rwlock_unlock(&pool->rwlock); if (data == NULL) { break; } diff --git a/src/bthread/types.h b/src/bthread/types.h index d91b85aab3..b1b1aba67b 100644 --- a/src/bthread/types.h +++ b/src/bthread/types.h @@ -83,8 +83,17 @@ inline std::ostream& operator<<(std::ostream& os, bthread_key_t key) { } #endif // __cplusplus +namespace bthread{ +class KeyTableList; +} + +namespace butil { +template class ThreadLocal; +} + typedef struct { - pthread_mutex_t mutex; + pthread_rwlock_t rwlock; + butil::ThreadLocal* list; void* free_keytables; int destroyed; } bthread_keytable_pool_t; diff --git a/test/bthread_key_unittest.cpp b/test/bthread_key_unittest.cpp index 7b6bd8d807..66a49128bf 100644 --- a/test/bthread_key_unittest.cpp +++ b/test/bthread_key_unittest.cpp @@ -339,13 +339,12 @@ TEST(KeyTest, set_tls_before_creating_any_bthread) { struct PoolData { bthread_key_t key; - PoolData* expected_data; + PoolData* data; int seq; int end_seq; }; static void pool_thread_impl(PoolData* data) { - ASSERT_EQ(data->expected_data, (PoolData*)bthread_getspecific(data->key)); if (NULL == bthread_getspecific(data->key)) { ASSERT_EQ(0, bthread_setspecific(data->key, data)); } @@ -385,19 +384,17 @@ TEST(KeyTest, using_pool) { ASSERT_EQ(0, bthread_start_urgent(&bth, &attr, pool_thread, &bth_data)); ASSERT_EQ(0, bthread_join(bth, NULL)); ASSERT_EQ(0, bth_data.seq); - ASSERT_EQ(1, bthread_keytable_pool_size(&pool)); PoolData bth2_data = { key, &bth_data, 0, 3 }; bthread_t bth2; ASSERT_EQ(0, bthread_start_urgent(&bth2, &attr2, pool_thread, &bth2_data)); ASSERT_EQ(0, bthread_join(bth2, NULL)); ASSERT_EQ(0, bth2_data.seq); - ASSERT_EQ(1, bthread_keytable_pool_size(&pool)); ASSERT_EQ(0, bthread_keytable_pool_destroy(&pool)); EXPECT_EQ(bth_data.end_seq, bth_data.seq); - EXPECT_EQ(0, bth2_data.seq); + EXPECT_EQ(bth_data.end_seq, bth2_data.seq); ASSERT_EQ(0, bthread_key_delete(key)); } From c707aa3a1f1f4650c282a034627c9316414e5e88 Mon Sep 17 00:00:00 2001 From: MJY-HUST Date: Mon, 27 May 2024 02:33:02 +0800 Subject: [PATCH 2/8] update by comments --- src/bthread/key.cpp | 16 +++++++++------- src/bthread/types.h | 10 +--------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/bthread/key.cpp b/src/bthread/key.cpp index e6e0385e71..93ec0a163b 100644 --- a/src/bthread/key.cpp +++ b/src/bthread/key.cpp @@ -234,11 +234,12 @@ class KeyTableList { }; static KeyTable* borrow_keytable(bthread_keytable_pool_t* pool) { - if (pool != NULL && (pool->list->get()->keytable || pool->free_keytables)) { + butil::ThreadLocal* list = (butil::ThreadLocal*)pool->list; + if (pool != NULL && (list->get()->keytable || pool->free_keytables)) { pthread_rwlock_rdlock(&pool->rwlock); - KeyTable* p = pool->list->get()->keytable; + KeyTable* p = list->get()->keytable; if (p) { - pool->list->get()->keytable = p->next; + list->get()->keytable = p->next; pthread_rwlock_unlock(&pool->rwlock); return p; } @@ -273,8 +274,9 @@ void return_keytable(bthread_keytable_pool_t* pool, KeyTable* kt) { delete kt; return; } - kt->next = pool->list->get()->keytable; - pool->list->get()->keytable = kt; + butil::ThreadLocal* list = (butil::ThreadLocal*)pool->list; + kt->next = list->get()->keytable; + list->get()->keytable = kt; pthread_rwlock_unlock(&pool->rwlock); } @@ -336,7 +338,7 @@ int bthread_keytable_pool_destroy(bthread_keytable_pool_t* pool) { bthread::KeyTable* saved_free_keytables = NULL; pthread_rwlock_wrlock(&pool->rwlock); pool->destroyed = 1; - delete pool->list; + delete (butil::ThreadLocal*)pool->list; saved_free_keytables = (bthread::KeyTable*)pool->free_keytables; pool->free_keytables = NULL; pthread_rwlock_unlock(&pool->rwlock); @@ -358,7 +360,7 @@ int bthread_keytable_pool_getstat(bthread_keytable_pool_t* pool, LOG(ERROR) << "Param[pool] or Param[stat] is NULL"; return EINVAL; } - pthread_rwlock_wrlock(&pool->rwlock); + pthread_rwlock_rdlock(&pool->rwlock); size_t count = 0; bthread::KeyTable* p = (bthread::KeyTable*)pool->free_keytables; for (; p; p = p->next, ++count) {} diff --git a/src/bthread/types.h b/src/bthread/types.h index b1b1aba67b..4b4f0565f5 100644 --- a/src/bthread/types.h +++ b/src/bthread/types.h @@ -83,17 +83,9 @@ inline std::ostream& operator<<(std::ostream& os, bthread_key_t key) { } #endif // __cplusplus -namespace bthread{ -class KeyTableList; -} - -namespace butil { -template class ThreadLocal; -} - typedef struct { pthread_rwlock_t rwlock; - butil::ThreadLocal* list; + void* list; void* free_keytables; int destroyed; } bthread_keytable_pool_t; From ddb253fbdffbcd3262a383e37f8791a1287068da Mon Sep 17 00:00:00 2001 From: MJY-HUST Date: Mon, 27 May 2024 20:42:19 +0800 Subject: [PATCH 3/8] update --- src/bthread/key.cpp | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/bthread/key.cpp b/src/bthread/key.cpp index 93ec0a163b..5a4e4bde54 100644 --- a/src/bthread/key.cpp +++ b/src/bthread/key.cpp @@ -234,11 +234,12 @@ class KeyTableList { }; static KeyTable* borrow_keytable(bthread_keytable_pool_t* pool) { - butil::ThreadLocal* list = (butil::ThreadLocal*)pool->list; - if (pool != NULL && (list->get()->keytable || pool->free_keytables)) { + if (pool != NULL && (pool->list || pool->free_keytables)) { + KeyTable* p; + auto list = (butil::ThreadLocal*)pool->list; pthread_rwlock_rdlock(&pool->rwlock); - KeyTable* p = list->get()->keytable; - if (p) { + if (list && list->get()->keytable) { + p = list->get()->keytable; list->get()->keytable = p->next; pthread_rwlock_unlock(&pool->rwlock); return p; @@ -274,7 +275,7 @@ void return_keytable(bthread_keytable_pool_t* pool, KeyTable* kt) { delete kt; return; } - butil::ThreadLocal* list = (butil::ThreadLocal*)pool->list; + auto list = (butil::ThreadLocal*)pool->list; kt->next = list->get()->keytable; list->get()->keytable = kt; pthread_rwlock_unlock(&pool->rwlock); @@ -340,13 +341,23 @@ int bthread_keytable_pool_destroy(bthread_keytable_pool_t* pool) { pool->destroyed = 1; delete (butil::ThreadLocal*)pool->list; saved_free_keytables = (bthread::KeyTable*)pool->free_keytables; + pool->list = NULL; pool->free_keytables = NULL; pthread_rwlock_unlock(&pool->rwlock); + + // Cheat get/setspecific and destroy the keytables. + bthread::TaskGroup* const g = bthread::tls_task_group; + bthread::KeyTable* old_kt = bthread::tls_bls.keytable; while(saved_free_keytables) { bthread::KeyTable* kt = saved_free_keytables; saved_free_keytables = kt->next; + bthread::tls_bls.keytable = kt; delete kt; } + bthread::tls_bls.keytable = old_kt; + if (g) { + g->current_task()->local_storage.keytable = old_kt; + } return 0; // TODO: return_keytable may race with this function, we don't destroy // the mutex right now. From 0fb0655c5b81af51614cd1b74d86967db9f751c9 Mon Sep 17 00:00:00 2001 From: MJY-HUST Date: Tue, 28 May 2024 21:43:34 +0800 Subject: [PATCH 4/8] fix --- src/bthread/key.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/bthread/key.cpp b/src/bthread/key.cpp index 5a4e4bde54..7495694dc3 100644 --- a/src/bthread/key.cpp +++ b/src/bthread/key.cpp @@ -211,7 +211,7 @@ class KeyTableList { keytable = NULL; } ~KeyTableList() { - bthread::TaskGroup* const g = bthread::tls_task_group; + bthread::TaskGroup* g = bthread::tls_task_group; bthread::KeyTable* old_kt = bthread::tls_bls.keytable; while (keytable) { bthread::KeyTable* kt = keytable; @@ -224,6 +224,7 @@ class KeyTableList { if (old_kt == kt) { old_kt = NULL; } + g = bthread::tls_task_group; } bthread::tls_bls.keytable = old_kt; if(g) { @@ -236,8 +237,8 @@ class KeyTableList { static KeyTable* borrow_keytable(bthread_keytable_pool_t* pool) { if (pool != NULL && (pool->list || pool->free_keytables)) { KeyTable* p; - auto list = (butil::ThreadLocal*)pool->list; pthread_rwlock_rdlock(&pool->rwlock); + auto list = (butil::ThreadLocal*)pool->list; if (list && list->get()->keytable) { p = list->get()->keytable; list->get()->keytable = p->next; @@ -346,13 +347,17 @@ int bthread_keytable_pool_destroy(bthread_keytable_pool_t* pool) { pthread_rwlock_unlock(&pool->rwlock); // Cheat get/setspecific and destroy the keytables. - bthread::TaskGroup* const g = bthread::tls_task_group; + bthread::TaskGroup* g = bthread::tls_task_group; bthread::KeyTable* old_kt = bthread::tls_bls.keytable; while(saved_free_keytables) { bthread::KeyTable* kt = saved_free_keytables; saved_free_keytables = kt->next; bthread::tls_bls.keytable = kt; + if (g) { + g->current_task()->local_storage.keytable = kt; + } delete kt; + g = bthread::tls_task_group; } bthread::tls_bls.keytable = old_kt; if (g) { From d27932479fb64a86ba174c60dfc812cc880fa751 Mon Sep 17 00:00:00 2001 From: MJY-HUST Date: Wed, 29 May 2024 01:34:49 +0800 Subject: [PATCH 5/8] update test for using_pool --- test/bthread_key_unittest.cpp | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/test/bthread_key_unittest.cpp b/test/bthread_key_unittest.cpp index 66a49128bf..60d613d0b5 100644 --- a/test/bthread_key_unittest.cpp +++ b/test/bthread_key_unittest.cpp @@ -344,9 +344,13 @@ struct PoolData { int end_seq; }; +bool use_same_keytable = false; + static void pool_thread_impl(PoolData* data) { if (NULL == bthread_getspecific(data->key)) { ASSERT_EQ(0, bthread_setspecific(data->key, data)); + } else { + use_same_keytable = true; } }; @@ -385,16 +389,20 @@ TEST(KeyTest, using_pool) { ASSERT_EQ(0, bthread_join(bth, NULL)); ASSERT_EQ(0, bth_data.seq); - PoolData bth2_data = { key, &bth_data, 0, 3 }; + PoolData bth2_data = { key, NULL, 0, 3 }; bthread_t bth2; ASSERT_EQ(0, bthread_start_urgent(&bth2, &attr2, pool_thread, &bth2_data)); ASSERT_EQ(0, bthread_join(bth2, NULL)); ASSERT_EQ(0, bth2_data.seq); ASSERT_EQ(0, bthread_keytable_pool_destroy(&pool)); - - EXPECT_EQ(bth_data.end_seq, bth_data.seq); - EXPECT_EQ(bth_data.end_seq, bth2_data.seq); + if(use_same_keytable) { + EXPECT_EQ(bth_data.end_seq, bth_data.seq); + EXPECT_EQ(0, bth2_data.seq); + } else { + EXPECT_EQ(bth_data.end_seq, bth_data.seq); + EXPECT_EQ(bth_data.end_seq, bth2_data.seq); + } ASSERT_EQ(0, bthread_key_delete(key)); } @@ -412,7 +420,7 @@ static void lid_dtor(void* tls) { static void lid_worker_impl(bthread_key_t key) { ASSERT_EQ(NULL, bthread_getspecific(key)); - ASSERT_EQ(0, bthread_setspecific(key, (void*)seq.fetch_add(1))); + ASSERT_EQ(0, bthread_setspecific(key, (void*)lid_seq.fetch_add(1))); } static void* lid_worker(void* arg) { From c2bc88b13e0a13eb03e46b77a3c1142392522071 Mon Sep 17 00:00:00 2001 From: MJY-HUST Date: Wed, 29 May 2024 22:09:38 +0800 Subject: [PATCH 6/8] fix format --- test/bthread_key_unittest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/bthread_key_unittest.cpp b/test/bthread_key_unittest.cpp index 60d613d0b5..c01ae7fe29 100644 --- a/test/bthread_key_unittest.cpp +++ b/test/bthread_key_unittest.cpp @@ -396,7 +396,7 @@ TEST(KeyTest, using_pool) { ASSERT_EQ(0, bth2_data.seq); ASSERT_EQ(0, bthread_keytable_pool_destroy(&pool)); - if(use_same_keytable) { + if (use_same_keytable) { EXPECT_EQ(bth_data.end_seq, bth_data.seq); EXPECT_EQ(0, bth2_data.seq); } else { From 98edccf873c9db9b5fdc0824c4ab0f44e3785f91 Mon Sep 17 00:00:00 2001 From: MJY-HUST Date: Thu, 6 Jun 2024 00:31:47 +0800 Subject: [PATCH 7/8] format --- src/bthread/key.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/bthread/key.cpp b/src/bthread/key.cpp index 7495694dc3..2dc3ba9068 100644 --- a/src/bthread/key.cpp +++ b/src/bthread/key.cpp @@ -349,7 +349,7 @@ int bthread_keytable_pool_destroy(bthread_keytable_pool_t* pool) { // Cheat get/setspecific and destroy the keytables. bthread::TaskGroup* g = bthread::tls_task_group; bthread::KeyTable* old_kt = bthread::tls_bls.keytable; - while(saved_free_keytables) { + while (saved_free_keytables) { bthread::KeyTable* kt = saved_free_keytables; saved_free_keytables = kt->next; bthread::tls_bls.keytable = kt; @@ -363,7 +363,6 @@ int bthread_keytable_pool_destroy(bthread_keytable_pool_t* pool) { if (g) { g->current_task()->local_storage.keytable = old_kt; } - return 0; // TODO: return_keytable may race with this function, we don't destroy // the mutex right now. // pthread_mutex_destroy(&pool->mutex); From efcaba729c138ae2aed749b8fa05d1524e43455b Mon Sep 17 00:00:00 2001 From: MJY-HUST Date: Wed, 12 Jun 2024 00:49:54 +0800 Subject: [PATCH 8/8] Change KeyTableList from class to struct --- src/bthread/key.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/bthread/key.cpp b/src/bthread/key.cpp index 2dc3ba9068..433e1e1409 100644 --- a/src/bthread/key.cpp +++ b/src/bthread/key.cpp @@ -205,8 +205,7 @@ class BAIDU_CACHELINE_ALIGNMENT KeyTable { SubKeyTable* _subs[KEY_1STLEVEL_SIZE]; }; -class KeyTableList { -public: +struct KeyTableList { KeyTableList() { keytable = NULL; }