Skip to content

subsys: portability: cmsis_rtos_v2: Fix dynamic thread stack reuse #85557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

subsys: portability: cmsis_rtos_v2: Fix dynamic thread stack reuse #85557

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 46 additions & 2 deletions subsys/portability/cmsis_rtos_v2/thread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ static K_THREAD_STACK_ARRAY_DEFINE(cmsis_rtos_thread_stack_pool,
CONFIG_CMSIS_V2_THREAD_DYNAMIC_STACK_SIZE);
#endif

struct stack_usage_info {
bool is_in_use;
} stack_status[CONFIG_CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT];
Copy link
Contributor

Choose a reason for hiding this comment

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

This declaration is closely related to cmsis_rtos_thread_stack_pool. So, I suggest:

  • place it just after cmsis_rtos_thread_stack_pool in the if CONFIG_CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT != 0 block (even if this condition looks useless, I agree).
  • I wonder if declaring a struct with only one member is not just increasing complexity for free.
  • stack_usage_info vs stack_status vs is_in_use. Having several names for the same purpose make code more complex.
  • change the name to outline the link with cmsis_rtos_thread_stack_pool. eg. cmsis_rtos_thread_stack_in_use.

Therefore, I would use bool cmsis_rtos_thread_stack_in_use[CONFIG_CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT].


static inline int _is_thread_cmsis_inactive(struct k_thread *thread)
{
uint8_t state = thread->base.thread_state;
Expand Down Expand Up @@ -104,7 +108,7 @@ osThreadId_t osThreadNew(osThreadFunc_t threadfunc, void *arg, const osThreadAtt
osPriority_t cv2_prio;
struct cmsis_rtos_thread_cb *tid;
static uint32_t one_time;
void *stack;
void *stack = NULL;
size_t stack_size;
uint32_t this_thread_num;
uint32_t this_dynamic_cb;
Expand Down Expand Up @@ -174,7 +178,14 @@ osThreadId_t osThreadNew(osThreadFunc_t threadfunc, void *arg, const osThreadAtt
"dynamic stack size must be configured to be non-zero\n");
this_dynamic_stack = atomic_inc(&num_dynamic_stack);
stack_size = CONFIG_CMSIS_V2_THREAD_DYNAMIC_STACK_SIZE;
stack = cmsis_rtos_thread_stack_pool[this_dynamic_stack];

for (int stack_idx = this_dynamic_stack; stack_idx >= 0; stack_idx--) {
if (!stack_status[stack_idx].is_in_use) {
stack = cmsis_rtos_thread_stack_pool[stack_idx];
stack_status[stack_idx].is_in_use = true;
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't dive in the detail of this algorithm, but it seems weird we use the value of num_dynamic_stack to find a free resource.

[EDIT: Ok, I think it works, but the loop is unusual. Why not just: ARRAY_FOR_EACH(stack_status, i) ?]

} else
#endif
{
Expand Down Expand Up @@ -477,6 +488,37 @@ osStatus_t osThreadJoin(osThreadId_t thread_id)
return (ret == 0) ? osOK : osErrorResource;
}

/**
* @brief Decrement the thread count based on thread type.
*/
int decrement_thread_counter(struct cmsis_rtos_thread_cb *tid)
{
if (tid == NULL) {
return -EINVAL;
}

#if CONFIG_CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT != 0
for (int stack_idx = 0; stack_idx < CONFIG_CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT; stack_idx++) {
if (tid->z_thread.stack_info.start ==
(uintptr_t)cmsis_rtos_thread_stack_pool[stack_idx]) {
atomic_dec(&num_dynamic_stack);
stack_status[stack_idx].is_in_use = false;
break;
}
}
#endif

for (int cb_idx = 0; cb_idx < CONFIG_CMSIS_V2_THREAD_MAX_COUNT; cb_idx++) {
if (tid == &cmsis_rtos_thread_cb_pool[cb_idx]) {
atomic_dec(&num_dynamic_cb);
break;
}
}

atomic_dec((atomic_t *)&thread_num);
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes about thread_num and num_dynamic_cb. Don't seems directly related to dynamic threads. I believe it makes sense to place that in a separated commit. Then, it will be easier to explain why you need to change it.

(style note: when the loop are simple, call the loop indexes i and j. Every developers understand i and j are loop indexes).

}

/**
* @brief Terminate execution of current running thread.
*/
Expand All @@ -487,6 +529,7 @@ __NO_RETURN void osThreadExit(void)
__ASSERT(!k_is_in_isr(), "");
tid = osThreadGetId();

decrement_thread_counter(tid);
k_thread_abort((k_tid_t)&tid->z_thread);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel you have to abort the thread before to release the stack, no?


CODE_UNREACHABLE;
Expand All @@ -511,6 +554,7 @@ osStatus_t osThreadTerminate(osThreadId_t thread_id)
return osErrorResource;
}

decrement_thread_counter(tid);
k_thread_abort((k_tid_t)&tid->z_thread);
return osOK;
}
Expand Down
Loading