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

ArunmaniAlagarsamy2710 · 2025-02-11T08:50:57Z

Decrease the atomic variable thread_num_dynamic when a thread terminates and exits. Previously, once the maximum thread count was reached, no new threads could be created even after existing threads terminated.

This fix ensures that thread stacks can be reused by osThreadNew, even after the specified number of threads (CONFIG_CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT) has been reached.

github-actions · 2025-02-11T08:51:34Z

Hello @ArunmaniAlagarsamy2710, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

jerome-pouiller · 2025-02-12T10:30:35Z

subsys/portability/cmsis_rtos_v2/thread.c

 	__ASSERT(!k_is_in_isr(), "");
 	tid = osThreadGetId();

+#if CONFIG_CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT


thread_num_dynamic and CONFIG_CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT are always defined, so you can probably write:

if (CONFIG_CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT != 0) { atomic_dec((atomic_t *)&thread_num_dynamic); }

jerome-pouiller · 2025-02-12T10:33:07Z

subsys/portability/cmsis_rtos_v2/thread.c


+#if CONFIG_CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT
+	atomic_dec((atomic_t *)&thread_num_dynamic);
+#endif


This code decrements thread_num_dynamic even if the current thread is not a dynamic thread. I assume, this is not what we expect.

jerome-pouiller · 2025-02-12T14:56:11Z

subsys/portability/cmsis_rtos_v2/thread.c

+		if (thread_num_dynamic) {
+			atomic_dec((atomic_t *)&thread_num_dynamic);
+		}
+	}


Won't work if a static thread ends before a dynamic thread.

BTW, the code that allocate the stack won't work:

this_thread_num_dynamic = atomic_inc((atomic_t *)&thread_num_dynamic); stack = cv2_thread_stack_pool[this_thread_num_dynamic];

Decrease the atomic variable thread_num_dynamic when a thread terminates and exits. Previously, once the maximum thread count was reached, no new threads could be created even after existing threads terminated. This fix ensures that thread stacks can be reused by osThreadNew, even after the specified number of threads (CONFIG_CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT) has been reached. Signed-off-by: Arunmani Alagarsamy <arunmani.a@silabs.com>

ArunmaniAlagarsamy2710 · 2025-03-12T07:20:50Z

Rebased to resolve the conflict

jerome-pouiller

I don't have any knowledge of the Cmsis modules, so my comments are only very generic.

jerome-pouiller · 2025-03-17T09:54:12Z

subsys/portability/cmsis_rtos_v2/thread.c

 	tid = osThreadGetId();

+	decrement_thread_counter(tid);
 	k_thread_abort((k_tid_t)&tid->z_thread);


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

jerome-pouiller · 2025-03-17T09:57:47Z

subsys/portability/cmsis_rtos_v2/thread.c

+				stack_status[stack_idx].is_in_use = true;
+				break;
+			}
+		}


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) ?]

jerome-pouiller · 2025-03-17T10:03:30Z

subsys/portability/cmsis_rtos_v2/thread.c

+	}
+
+	atomic_dec((atomic_t *)&thread_num);
+	return 0;


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).

jerome-pouiller · 2025-03-17T10:21:19Z

subsys/portability/cmsis_rtos_v2/thread.c


+struct stack_usage_info {
+	bool is_in_use;
+} stack_status[CONFIG_CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT];


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].

Zephyr CMSIS implementation has some limitations with dynamically allocated resources[1]. This patch allocates the CMSIS resources statically. Thus: - this allows to workaround the limitation of the current CMSIS implementation until we fix them properly - it is possible to get rid of the dependencies to CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT and CMSIS_V2_THREAD_MAX_COUNT. [1]: zephyrproject-rtos/zephyr#85557 Signed-off-by: Arunmani Alagarsamy <arunmani.a@silabs.com>

Zephyr CMSIS implementation has some limitations with dynamically allocated resources[1]. This patch allocates the CMSIS resources statically. Thus: - this allows to workaround the limitation of the current CMSIS implementation until we fix them properly - it is possible to get rid of the dependencies to CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT and CMSIS_V2_THREAD_MAX_COUNT. [1]: zephyrproject-rtos/zephyr#85557 Upstream-status: Inappropriate [Zephyr specific workaround for stack allocation] Signed-off-by: Arunmani Alagarsamy <arunmani.a@silabs.com>

Zephyr CMSIS implementation has some limitations with dynamically allocated resources[1]. This patch allocates the CMSIS resources statically. Thus: - this allows to workaround the limitation of the current CMSIS implementation until we fix them properly - it is possible to get rid of the dependencies to CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT and CMSIS_V2_THREAD_MAX_COUNT. [1]: zephyrproject-rtos/zephyr#85557 Upstream-status: Inappropriate [Zephyr specific workaround for stack allocation] Signed-off-by: Arunmani Alagarsamy <arunmani.a@silabs.com> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>

github-actions · 2025-05-17T00:37:39Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

Zephyr CMSIS implementation has some limitations with dynamically allocated resources[1]. This patch allocates the CMSIS resources statically. Thus: - this allows to workaround the limitation of the current CMSIS implementation until we fix them properly - it is possible to get rid of the dependencies to CMSIS_V2_THREAD_DYNAMIC_MAX_COUNT and CMSIS_V2_THREAD_MAX_COUNT. [1]: zephyrproject-rtos/zephyr#85557 Upstream-status: Inappropriate [Zephyr specific workaround for stack allocation] Signed-off-by: Arunmani Alagarsamy <arunmani.a@silabs.com> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>

zephyrbot added area: CMSIS API Layer CMSIS-RTOS API Layer area: Portability Standard compliant code, toolchain abstraction labels Feb 11, 2025

zephyrbot requested a review from nashif February 11, 2025 08:51

ArunmaniAlagarsamy2710 force-pushed the fix/cmsis-thread-termination branch from c1eafa2 to 6b095b2 Compare February 11, 2025 10:07

jerome-pouiller requested review from jhedberg and jerome-pouiller February 12, 2025 10:23

jerome-pouiller reviewed Feb 12, 2025

View reviewed changes

ArunmaniAlagarsamy2710 force-pushed the fix/cmsis-thread-termination branch from 6b095b2 to 450734a Compare February 12, 2025 13:42

jerome-pouiller reviewed Feb 12, 2025

View reviewed changes

ArunmaniAlagarsamy2710 force-pushed the fix/cmsis-thread-termination branch 2 times, most recently from cfa400d to 7fd2ce7 Compare February 17, 2025 04:41

ArunmaniAlagarsamy2710 mentioned this pull request Feb 18, 2025

drivers: wifi: siwx91x: Add run time opermode #85919

Closed

jerome-pouiller self-requested a review March 5, 2025 17:41

ArunmaniAlagarsamy2710 force-pushed the fix/cmsis-thread-termination branch from 7fd2ce7 to 91f527c Compare March 12, 2025 07:16

jerome-pouiller reviewed Mar 17, 2025

View reviewed changes

github-actions bot added the Stale label May 17, 2025

github-actions bot closed this May 31, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

ArunmaniAlagarsamy2710 commented Feb 11, 2025

Uh oh!

github-actions bot commented Feb 11, 2025

Uh oh!

jerome-pouiller Feb 12, 2025

Uh oh!

jerome-pouiller Feb 12, 2025

Uh oh!

jerome-pouiller Feb 12, 2025

Uh oh!

jerome-pouiller Feb 12, 2025

Uh oh!

ArunmaniAlagarsamy2710 commented Mar 12, 2025

Uh oh!

jerome-pouiller left a comment

Uh oh!

jerome-pouiller Mar 17, 2025

Uh oh!

jerome-pouiller Mar 17, 2025

Uh oh!

jerome-pouiller Mar 17, 2025

Uh oh!

jerome-pouiller Mar 17, 2025

Uh oh!

github-actions bot commented May 17, 2025

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

ArunmaniAlagarsamy2710 commented Feb 11, 2025

Uh oh!

github-actions bot commented Feb 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArunmaniAlagarsamy2710 commented Mar 12, 2025

Uh oh!

jerome-pouiller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 17, 2025

Uh oh!

Uh oh!