Skip to content

Commit

Permalink
Generalize MAX_THREADS (#636)
Browse files Browse the repository at this point in the history 
Enable any number of MAX_THREADS
  • Loading branch information
@rtjohnso
rtjohnso authored Dec 25, 2024
1 parent 525299b commit 38258e1
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 137 deletions.
80 changes: 47 additions & 33 deletions src/task.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 *
Expand All @@ -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.
*/
Expand All @@ -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

Expand All @@ -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));
}
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
8 changes: 3 additions & 5 deletions src/task.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down Expand Up @@ -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);
101 changes: 2 additions & 99 deletions tests/unit/task_system_test.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
};

/*
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}

/*
Expand Down Expand Up @@ -533,52 +492,18 @@ 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,
"exp_thread_idx=%lu, this_threads_idx=%lu\n",
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())
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 38258e1

Please sign in to comment.