From 38258e197dd05c057ff8bbf70dbfa09ed515b5f0 Mon Sep 17 00:00:00 2001 From: Rob Johnson Date: Tue, 24 Dec 2024 23:11:42 -0800 Subject: [PATCH] Generalize MAX_THREADS (#636) Enable any number of MAX_THREADS --- src/task.c | 80 ++++++++++++++++----------- src/task.h | 8 +-- tests/unit/task_system_test.c | 101 +--------------------------------- 3 files changed, 52 insertions(+), 137 deletions(-) diff --git a/src/task.c b/src/task.c index b7df785ca..9404bc78c 100644 --- a/src/task.c +++ b/src/task.c @@ -23,21 +23,19 @@ _Static_assert((ARRAY_SIZE(task_type_name) == NUM_TASK_TYPES), static void task_init_tid_bitmask(uint64 *tid_bitmask) { - // We use a 64 bit word to act as the thread bitmap. - _Static_assert(MAX_THREADS == 64, "Max threads should be 64"); /* * This is a special bitmask where 1 indicates free and 0 indicates * allocated. So, we set all bits to 1 during init. */ for (int i = 0; i < MAX_THREADS; i++) { - *tid_bitmask |= (1ULL << i); + tid_bitmask[i / 64] |= (1ULL << (i % 64)); } } static inline uint64 * task_system_get_tid_bitmask(task_system *ts) { - return &ts->tid_bitmask; + return ts->tid_bitmask; } static threadid * @@ -46,15 +44,6 @@ task_system_get_max_tid(task_system *ts) return &ts->max_tid; } -/* - * Return the bitmasks of tasks active. Mainly intended as a testing hook. - */ -uint64 -task_active_tasks_mask(task_system *ts) -{ - return *task_system_get_tid_bitmask(ts); -} - /* * Allocate a threadid. Returns INVALID_TID when no tid is available. */ @@ -66,22 +55,42 @@ task_allocate_threadid(task_system *ts) uint64 old_bitmask; uint64 new_bitmask; - do { - old_bitmask = *tid_bitmask; + while (!__sync_lock_test_and_set(&ts->tid_bitmask_lock, 1)) { + // spin + } + + int i; + for (i = 0; i < (MAX_THREADS + 63) / 64; i++) { + old_bitmask = tid_bitmask[i]; // first bit set to 1 starting from LSB. uint64 pos = __builtin_ffsl(old_bitmask); // If all threads are in-use, bitmask will be all 0s. if (pos == 0) { - return INVALID_TID; + continue; } // builtin_ffsl returns the position plus 1. tid = pos - 1; // set bit at that position to 0, indicating in use. new_bitmask = (old_bitmask & ~(1ULL << (pos - 1))); - } while ( - !__sync_bool_compare_and_swap(tid_bitmask, old_bitmask, new_bitmask)); + if (__sync_bool_compare_and_swap( + &tid_bitmask[i], old_bitmask, new_bitmask)) + { + break; + } + } + + __sync_lock_release(&ts->tid_bitmask_lock); + + if (i == 2) { + return INVALID_TID; + } + + tid += i * 64; + if (tid >= MAX_THREADS) { + return INVALID_TID; + } // Invariant: we have successfully allocated tid @@ -103,20 +112,22 @@ task_deallocate_threadid(task_system *ts, threadid tid) { uint64 *tid_bitmask = task_system_get_tid_bitmask(ts); - uint64 bitmask_val = *tid_bitmask; + uint64 bitmask_val = tid_bitmask[tid / 64]; // Ensure that caller is only clearing for a thread that's in-use. - platform_assert(!(bitmask_val & (1ULL << tid)), + platform_assert(!(bitmask_val & (1ULL << (tid % 64))), "Thread [%lu] is expected to be in-use. Bitmap: 0x%lx", tid, bitmask_val); // set bit back to 1 to indicate a free slot. - uint64 tmp_bitmask = *tid_bitmask; - uint64 new_value = tmp_bitmask | (1ULL << tid); - while (!__sync_bool_compare_and_swap(tid_bitmask, tmp_bitmask, new_value)) { - tmp_bitmask = *tid_bitmask; - new_value = tmp_bitmask | (1ULL << tid); + uint64 tmp_bitmask = tid_bitmask[tid / 64]; + uint64 new_value = tmp_bitmask | (1ULL << (tid % 64)); + while (!__sync_bool_compare_and_swap( + tid_bitmask + tid / 64, tmp_bitmask, new_value)) + { + tmp_bitmask = tid_bitmask[tid / 64]; + new_value = tmp_bitmask | (1ULL << (tid % 64)); } } @@ -877,10 +888,11 @@ task_system_create(platform_heap_id hid, *system = NULL; return STATUS_NO_MEMORY; } - ts->cfg = cfg; - ts->ioh = ioh; - ts->heap_id = hid; - task_init_tid_bitmask(&ts->tid_bitmask); + ts->cfg = cfg; + ts->ioh = ioh; + ts->heap_id = hid; + ts->tid_bitmask_lock = 0; + task_init_tid_bitmask(ts->tid_bitmask); // task initialization register_standard_hooks(ts); @@ -935,12 +947,14 @@ task_system_destroy(platform_heap_id hid, task_system **ts_in) if (tid != INVALID_TID) { task_deregister_this_thread(ts); } - if (ts->tid_bitmask != ((uint64)-1)) { - platform_error_log( + for (int i = 0; i < ARRAY_SIZE(ts->tid_bitmask); i++) { + platform_assert( + ts->tid_bitmask[i] == ((uint64)-1), "Destroying task system that still has some registered threads." - ", tid=%lu, tid_bitmask=0x%lx\n", + ", tid=%lu, tid_bitmask[%d] = %lx\n", tid, - ts->tid_bitmask); + i, + ts->tid_bitmask[i]); } platform_free(hid, ts); *ts_in = (task_system *)NULL; diff --git a/src/task.h b/src/task.h index 1b3bea17b..139d6e21a 100644 --- a/src/task.h +++ b/src/task.h @@ -112,12 +112,13 @@ struct task_system { platform_io_handle *ioh; platform_heap_id heap_id; /* - * bitmask used for generating and clearing thread id's. + * bitmask used for allocating thread id's. * If a bit is set to 0, it means we have an in use thread id for that * particular position, 1 means it is unset and that thread id is available * for use. */ - uint64 tid_bitmask; + uint64 tid_bitmask_lock; + uint64 tid_bitmask[(MAX_THREADS + 63) / 64]; // max thread id so far. threadid max_tid; void *thread_scratch[MAX_THREADS]; @@ -270,8 +271,5 @@ task_wait_for_completion(task_system *ts); threadid task_get_max_tid(task_system *ts); -uint64 -task_active_tasks_mask(task_system *ts); - void task_print_stats(task_system *ts); diff --git a/tests/unit/task_system_test.c b/tests/unit/task_system_test.c index 82771c67a..d2389a18e 100644 --- a/tests/unit/task_system_test.c +++ b/tests/unit/task_system_test.c @@ -38,7 +38,6 @@ typedef struct { task_system *tasks; platform_thread this_thread_id; // OS-generated thread ID threadid exp_thread_idx; // Splinter-generated expected thread index - uint64 active_threads_bitmask; } thread_config; // Configuration for worker threads used in lock-step testing exercise @@ -89,8 +88,6 @@ CTEST_DATA(task_system) // Following get setup pointing to allocated memory platform_io_handle *ioh; // Only prerequisite needed to setup task system task_system *tasks; - - uint64 active_threads_bitmask; }; /* @@ -138,22 +135,6 @@ CTEST_SETUP(task_system) // Main task (this thread) is at index 0 ASSERT_EQUAL(0, platform_get_tid()); - - // Main thread should now be marked as being active in the bitmask. - // Active threads have their bit turned -OFF- in this bitmask. - uint64 task_bitmask = task_active_tasks_mask(data->tasks); - uint64 all_threads_inactive_mask = (~0L); - uint64 this_thread_active_mask = (~0x1L); - uint64 exp_bitmask = (all_threads_inactive_mask & this_thread_active_mask); - - ASSERT_EQUAL(exp_bitmask, - task_bitmask, - "exp_bitmask=0x%x, task_bitmask=0x%x\n", - exp_bitmask, - task_bitmask); - - // Save it off, so it can be used for verification in a test case. - data->active_threads_bitmask = exp_bitmask; } // Teardown function for suite, called after every test in suite @@ -209,8 +190,7 @@ CTEST2(task_system, test_one_thread_using_lower_apis) thread_cfg.tasks = data->tasks; // Main thread is at index 0 - thread_cfg.exp_thread_idx = 1; - thread_cfg.active_threads_bitmask = task_active_tasks_mask(data->tasks); + thread_cfg.exp_thread_idx = 1; platform_status rc = STATUS_OK; @@ -264,8 +244,7 @@ CTEST2(task_system, test_one_thread_using_extern_apis) thread_cfg.tasks = data->tasks; // Main thread is at index 0 - thread_cfg.exp_thread_idx = 1; - thread_cfg.active_threads_bitmask = task_active_tasks_mask(data->tasks); + thread_cfg.exp_thread_idx = 1; platform_status rc = STATUS_OK; @@ -363,26 +342,6 @@ CTEST2(task_system, test_task_system_creation_with_bg_threads) task_system_destroy(data->hid, &data->tasks); platform_status rc = create_task_system_with_bg_threads(data, 2, 4); ASSERT_TRUE(SUCCESS(rc)); - - uint64 all_threads_inactive_mask = (~0L); - - // Construct known bit-mask for active threads knowing that the background - // threads are started up when task system is created w/bg threads. - threadid max_thread_id = task_get_max_tid(data->tasks); - uint64 active_threads_mask = 0; - for (int tid = 0; tid < max_thread_id; tid++) { - active_threads_mask |= (1L << tid); - } - active_threads_mask = ~active_threads_mask; - - uint64 exp_bitmask = (all_threads_inactive_mask & active_threads_mask); - uint64 task_bitmask = task_active_tasks_mask(data->tasks); - - ASSERT_EQUAL(exp_bitmask, - task_bitmask, - "exp_bitmask=0x%x, task_bitmask=0x%x\n", - exp_bitmask, - task_bitmask); } /* @@ -533,29 +492,11 @@ exec_one_thread_use_lower_apis(void *arg) { thread_config *thread_cfg = (thread_config *)arg; - uint64 task_bitmask_before_register = - task_active_tasks_mask(thread_cfg->tasks); - - // Verify that the state of active-threads bitmask (managed by Splinter) has - // not changed just by creating this pthread. It should be the same as what - // we had recorded just prior to platform_thread_create(). - ASSERT_EQUAL(thread_cfg->active_threads_bitmask, - task_bitmask_before_register); - - CTEST_LOG_INFO("active_threads_bitmask=0x%lx\n", - thread_cfg->active_threads_bitmask); - // This is the important call to initialize thread-specific stuff in // Splinter's task-system, which sets up the thread-id (index) and records // this thread as active with the task system. task_register_this_thread(thread_cfg->tasks, trunk_get_scratch_size()); - uint64 task_bitmask_after_register = - task_active_tasks_mask(thread_cfg->tasks); - - // _Now, the active tasks bitmask should have changed. - ASSERT_NOT_EQUAL(task_bitmask_before_register, task_bitmask_after_register); - threadid this_threads_idx = platform_get_tid(); ASSERT_EQUAL(thread_cfg->exp_thread_idx, this_threads_idx, @@ -563,22 +504,6 @@ exec_one_thread_use_lower_apis(void *arg) thread_cfg->exp_thread_idx, this_threads_idx); - // This thread is recorded as 'being active' by clearing its bit from the - // mask. - uint64 exp_bitmask = (0x1L << this_threads_idx); - exp_bitmask = (task_bitmask_before_register & ~exp_bitmask); - - CTEST_LOG_INFO("this_threads_idx=%lu" - ", task_bitmask_before_register=0x%lx" - ", task_bitmask_after_register=0x%lx" - ", exp_bitmask=0x%lx\n", - this_threads_idx, - task_bitmask_before_register, - task_bitmask_after_register, - exp_bitmask); - - ASSERT_EQUAL(task_bitmask_after_register, exp_bitmask); - // Registration should have allocated some scratch space memory. ASSERT_TRUE( task_system_get_thread_scratch(thread_cfg->tasks, platform_get_tid()) @@ -591,12 +516,6 @@ exec_one_thread_use_lower_apis(void *arg) task_deregister_this_thread(thread_cfg->tasks); - uint64 task_bitmask_after_deregister = - task_active_tasks_mask(thread_cfg->tasks); - - // De-registering this task removes it from the active tasks mask - ASSERT_EQUAL(task_bitmask_before_register, task_bitmask_after_deregister); - // Deregistration releases scratch space memory. ASSERT_TRUE( task_system_get_thread_scratch(thread_cfg->tasks, this_threads_idx) @@ -628,25 +547,9 @@ exec_one_thread_use_extern_apis(void *arg) { thread_config *thread_cfg = (thread_config *)arg; - uint64 bitmask_before_thread_create = thread_cfg->active_threads_bitmask; - - uint64 bitmask_after_thread_create = - task_active_tasks_mask(thread_cfg->tasks); - - // The task_thread_create() -> task_invoke_with_hooks() also registers this - // thread with Splinter. First, confirm that the bitmask has changed from - // what it was before this thread was invoked. - ASSERT_NOT_EQUAL(bitmask_before_thread_create, bitmask_after_thread_create); - threadid this_threads_idx = platform_get_tid(); ASSERT_EQUAL(thread_cfg->exp_thread_idx, this_threads_idx); - // This thread is recorded as 'being active' by clearing its bit from the - // mask. Verify the expected task bitmask. - uint64 exp_bitmask = (0x1L << this_threads_idx); - exp_bitmask = (bitmask_before_thread_create & ~exp_bitmask); - ASSERT_EQUAL(bitmask_after_thread_create, exp_bitmask); - /* * Dead Code Warning! * The interface used here has already registered this thread. An attempt to