Refactor list node for embedded usage #60

vicLin8712 · 2025-12-04T10:35:53Z

Previously, the non-intrusive node design made the lifetime of list nodes ambiguous. To clarify the ownership responsibilities between list users and list operations, this change introduces an intrusive (embedded) node
design.

With this approach, list APIs focus solely on manipulating list nodes, while the container owning the node is responsible for its entire lifecycle.

Summary by cubic

Switched lists to an embedded-node design and removed allocations from pushback/remove/pop. Updated tasks, mutexes, and timers to use embedded nodes and a static timer pool to cut malloc/free overhead in hot paths.

New Features
- Added container_of helpers (container_of, tcb_from_global_node, tcb_from_mutex_node, timer_from_node, timer_from_running_node).
- Embedded list nodes in tcb_t (global_node, mutex_node) and timer_t (t_node, t_running_node).
Refactors
- list_node_t no longer stores data; list_pushback(list, node), list_remove(list, node), and list_pop(list) now operate on caller-provided nodes without allocating or freeing.
- Scheduler, HAL, main, and mutex waiters now use embedded nodes via container_of.
- Timer subsystem uses embedded nodes and a static timer_pool; removed dynamic timer/node allocation and clarified running-timer list handling.
- Renamed test_libc to test_utils; added list tests; updated Makefile target.

^{Written for commit f58f267. Summary will update automatically on new commits.}

cubic-dev-ai

No issues found across 3 files

jserv · 2025-12-04T10:44:31Z

Instead of using test_list.c, we can consolidate everything into test-utils.c, which covers all tests for utilities and helpers.

HeatCrab

I noticed that the original functions and the new helpers share the exact same traversal logic. Would it be better to let the original functions call the new helpers, instead of duplicating the logic?

visitorckw · 2025-12-04T17:40:31Z

The idea looks good for now.

However, in the long run, I wonder if we should consider adopting the Linux kernel approach: embedding the list node within the struct and using container_of().

This would allow us to avoid exposing dual APIs (one with internal allocation and one without). As recent fixes have shown, automatic memory allocation/deallocation can easily lead to oversight and result in memory-related bugs.

vicLin8712 · 2025-12-05T01:54:24Z

The idea looks good for now.

However, in the long run, I wonder if we should consider adopting the Linux kernel approach: embedding the list node within the struct and using container_of().

This would allow us to avoid exposing dual APIs (one with internal allocation and one without). As recent fixes have shown, automatic memory allocation/deallocation can easily lead to oversight and result in memory-related bugs.

Thanks for your feedback.
Using the Linux kernel style for intrusive linked lists looks like the right long-term direction to reduce memory-management bugs. I'll open a new issue to track this structural change.

For this PR, I’ll follow the suggestion from @HeatCrab to consolidate the repetitive API in list.h sections by switching to the new helper functions.

vicLin8712 · 2025-12-05T02:31:45Z

include/lib/list.h

+    list_remove_node(list, target);

-    prev->next = target->next;
    void *data = target->data;


Only free and return data operations are kept in the below section.

vicLin8712 · 2025-12-05T02:33:11Z

include/lib/list.h


    node->data = data;
-    node->next = list->tail;
+    node->next = NULL;


An initialized node should represent a standalone element; its next pointer must be set to NULL.

jserv · 2025-12-05T02:33:50Z

Using the Linux kernel style for intrusive linked lists looks like the right long-term direction to reduce memory-management bugs. I'll open a new issue to track this structural change.

You can refine here to consolidate.

vicLin8712 · 2025-12-07T07:13:14Z

include/lib/list.h

-/* Remove a specific node; returns its data */
-static inline void *list_remove(list_t *list, list_node_t *target)
+/* Remove a specific node form the list */
+static inline list_node_t *list_remove(list_t *list, list_node_t *target)


The return value is required for the exception handling when removing the waiter node from the mutex waiting list.

vicLin8712 · 2025-12-07T07:30:24Z

kernel/mutex.c

-    while (curr && curr != waiters->tail) {
-        if (curr->data == self) {
-            list_remove(waiters, curr);
+    list_node_t *curr_mutex_node = waiters->head->next;


This name is better aligned with the embedded list node.

Introduce a simple container operation to support embedded node design for the timer and scheduler subsystems This change allows container access from the embedded list node.

cubic-dev-ai

5 issues found across 13 files

Prompt for AI agents (all 5 issues)


Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="app/test_utils.c">

<violation number="1" location="app/test_utils.c:328">
P2: Incorrect expected value in assertion. After pushing three nodes in order (1, 2, 3), `list-&gt;head-&gt;next` still points to the first node (val=1), not the third. `list_pushback` adds to the tail, so the head-&gt;next remains the first inserted node.</violation>

<violation number="2" location="app/test_utils.c:333">
P2: Incorrect expected value in assertion. After removing the second node, the list is [first, third], so `list-&gt;head-&gt;next` points to first (val=1), not val=2.</violation>

<violation number="3" location="app/test_utils.c:338">
P2: Incorrect expected length after `list_pop`. The pop operation decrements length, so after popping from a list of 2 elements, length should be 1, not 2.</violation>
</file>

<file name="include/lib/list.h">

<violation number="1" location="include/lib/list.h:108">
P3: Typo in comment: &quot;form&quot; should be &quot;from&quot;.</violation>
</file>

<file name="kernel/timer.c">

<violation number="1" location="kernel/timer.c:142">
P0: Wrong `container_of` macro used. This function operates on `kcb-&gt;timer_list` (the running timer list) which contains `t_running_node` embedded nodes. Using `timer_from_node` (which offsets from `t_node`) instead of `timer_from_running_node` will compute an incorrect pointer, causing undefined behavior when accessing `deadline_ticks`.</violation>
</file>

_{Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR}

app/test_utils.c

include/lib/list.h

kernel/timer.c

Remove the data member from list_node to support an embedded-node design. List nodes are now embedded directly into container structures, eliminating the need to manage list node lifecycles during pushback or remove operations. The lifecycle is instead owned by the container itself. This change updates existing list helpers to align with the embedded design and reduces unnecessary memory operations during list manipulation.

Previously, timer management relied on list nodes provided by timer_node_pool. Timer creation and cancellation could trigger allocation or deallocation of timer or list nodes when no pool entries were available. This change pre-allocates timers and embeds list nodes directly within timer structures, reducing memory operations during timer lifecycle transitions and simplifying timer management.

This change introduces helper macros to retrieve timer containers from embedded list nodes. Timer resources are now statically allocated from timer_pool, eliminating dynamic memory operations during timer usage. Timer allocation and release are tracked only via the timer pool bitmask. To avoid ambiguity, the suffix `_running_list` is added to explicitly indicate lists holding active timers.

Timer APIs no longer require memory operations now that timers are statically allocated with embedded list nodes. This change removes redundant allocation paths and uses the timer_from_node() helper to align timer access with the embedded list node design.

Previously, expired timers were placed into the expired_timers array and their list nodes were returned to timer_node_pool. With embedded list nodes, a timer only needs to be removed from the running timer list to stop further handling. This change updates timer handling to operate directly on embedded list nodes, aligning the handler logic with the embedded node design.

The previous non-intrusive list node approach for tcb_t introduced unnecessary memory operations and ambiguity around list node lifecycle management. This change switches task list management to an embedded list node design, aligning task handling with the updated list operation model.

Add unit tests for general list operations using the new embedded-node design. Rename test_libc to test_utils so it can host unit tests for common helpers and consolidate reusable test code.

The condition check is removed because the node is embedded in the tcb, no need to confirm whether tcb is existing anymore. The access of tcb_t is replaced by the marco to align with the embedded node design.

Add timer cancellation APIs to validate timer behavior across different cancellation scenarios, including task destruction, explicit timer cancellation, and one-shot timers. These APIs help ensure cancelled or destroyed timers never fire and that one-shot timers handle tick expiration correctly.

vicLin8712 force-pushed the add-list-helper branch from e027d81 to 9a49d30 Compare December 4, 2025 10:38

cubic-dev-ai bot reviewed Dec 4, 2025

View reviewed changes

jserv requested review from HeatCrab and visitorckw December 4, 2025 10:42

vicLin8712 force-pushed the add-list-helper branch from 9a49d30 to 1266621 Compare December 4, 2025 12:13

HeatCrab reviewed Dec 4, 2025

View reviewed changes

vicLin8712 force-pushed the add-list-helper branch 2 times, most recently from 7307a00 to 948d586 Compare December 5, 2025 02:29

vicLin8712 commented Dec 5, 2025

View reviewed changes

vicLin8712 force-pushed the add-list-helper branch from 948d586 to 91dc543 Compare December 5, 2025 02:35

vicLin8712 marked this pull request as draft December 5, 2025 02:37

vicLin8712 force-pushed the add-list-helper branch 3 times, most recently from a98c684 to 50fe97c Compare December 7, 2025 06:37

vicLin8712 changed the title ~~Add list helper without memory operations~~ Refactor list node for embedded usage Dec 7, 2025

vicLin8712 commented Dec 7, 2025

View reviewed changes

Add container_of helpers for embedded node design

c5b57be

Introduce a simple container operation to support embedded node design for the timer and scheduler subsystems This change allows container access from the embedded list node.

vicLin8712 force-pushed the add-list-helper branch from bd9cfe7 to 308ae12 Compare December 7, 2025 07:36

vicLin8712 marked this pull request as ready for review December 7, 2025 07:40

vicLin8712 requested a review from HeatCrab December 7, 2025 07:40

cubic-dev-ai bot reviewed Dec 7, 2025

View reviewed changes

app/test_utils.c Outdated Show resolved Hide resolved

app/test_utils.c Outdated Show resolved Hide resolved

app/test_utils.c Show resolved Hide resolved

include/lib/list.h Outdated Show resolved Hide resolved

kernel/timer.c Outdated Show resolved Hide resolved

vicLin8712 added 3 commits December 7, 2025 15:53

vicLin8712 added 6 commits December 7, 2025 15:53

Add unit tests for list operations

6c55516

Add unit tests for general list operations using the new embedded-node design. Rename test_libc to test_utils so it can host unit tests for common helpers and consolidate reusable test code.

Refactor edf_sched to align with embedded node

dbf68ef

The condition check is removed because the node is embedded in the tcb, no need to confirm whether tcb is existing anymore. The access of tcb_t is replaced by the marco to align with the embedded node design.

vicLin8712 force-pushed the add-list-helper branch from 308ae12 to f58f267 Compare December 7, 2025 07:53

Refactor list node for embedded usage #60

Are you sure you want to change the base?

Refactor list node for embedded usage #60

Conversation

vicLin8712 commented Dec 4, 2025 • edited by cubic-dev-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by cubic

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

jserv commented Dec 4, 2025

Uh oh!

HeatCrab left a comment

Choose a reason for hiding this comment

Uh oh!

visitorckw commented Dec 4, 2025

Uh oh!

vicLin8712 commented Dec 5, 2025

Uh oh!

vicLin8712 Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

vicLin8712 Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

jserv commented Dec 5, 2025

Uh oh!

vicLin8712 Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

vicLin8712 Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vicLin8712 commented Dec 4, 2025 •

edited by cubic-dev-ai bot

Loading