!threads: split cpuTime into user and system time by oI0ck · Pull Request #813 · phoenix-rtos/phoenix-rtos-kernel

oI0ck · 2026-07-20T19:21:59Z

TASK: RTOS-1382

Description

Motivation and Context

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

Already covered by automatic testing.
New test added: (add PR link here).
Tested by hand on: (list targets here).

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing linter checks and tests passed.
My changes generate no new compilation warnings for any of the targets.

Special treatment

This PR needs additional PRs to work (list the PRs, preferably in merge-order).
I will merge this PR by myself when appropriate.

gemini-code-assist

Code Review

This pull request splits the single thread CPU time tracking into separate user and system CPU times, updating the thread structures, syscall dispatching, and thread iteration logic accordingly. Feedback on the changes highlights a critical bug in the newly added threads_updateCpuTime function, where an inverted NULL check will cause a NULL pointer dereference. Additionally, it is recommended to hold the spinlock for the entire duration of the time update to prevent data races, and to replace the runtime NULL check with a debug assertion.

github-actions · 2026-07-20T19:38:31Z

Unit Test Results

11 305 tests - 2 10 597 ✅ - 2 1h 0m 37s ⏱️ + 8m 10s
690 suites ±0 707 💤 ±0
1 files ±0 1 ❌ ±0

For more details on these failures, see this check.

Results for commit ef90a2b. ± Comparison against base commit fb914a8.

This pull request removes 3 and adds 1 tests. Note that renamed tests count towards both.

phoenix-rtos-tests/mem/mprotect ‑ armv7a9-zynq7000-zedboard:phoenix-rtos-tests/mem/mprotect.test_mprotect.pages_in_child_copied
phoenix-rtos-tests/mem/mprotect ‑ armv7a9-zynq7000-zedboard:phoenix-rtos-tests/mem/mprotect.test_mprotect.pages_in_parent_copied
phoenix-rtos-tests/mem/mprotect ‑ armv7a9-zynq7000-zedboard:phoenix-rtos-tests/mem/mprotect.test_mprotect.test_mprotect_singlecore

phoenix-rtos-tests/mem/mprotect ‑ armv7a9-zynq7000-zedboard:phoenix-rtos-tests/mem/mprotect

♻️ This comment has been updated with latest results.

adamgreloch · 2026-07-21T09:17:32Z

+		hal_spinlockSet(&threads_common.spinlock, &sc);
+		now = _proc_gettimeRaw();
+		hal_spinlockClear(&threads_common.spinlock, &sc);


Since thread_t already tracks lastTime, you may take now directly from hal_timerGetUs() and detect non-monotonicity in DEBUG by comparing lastTime with now, instead of calling _proc_gettimeRaw(). That way you wouldn't need a spinlock.

Then I'm essentially reimplementing what _proc_gettimeRaw does, without writing to threads_common.prev.
I don't really get the point of it; sure, it will save some time, but then, what is this function for?

Why take spinlock twice for each syscall when you can simply not?

_proc_gettimeRaw() checks non-monotonicity in threads.c from a common prev, since there is no other place to store this information. In your case however, you already store lastTime in the thread, so you can track it from there. You are concerned about reimplementing a two line function.

ok, @etiaro (and the tin man) have a point that systemTime and userTime should be synchronized due to use in threadsinfo, so this becomes irrelevant

adamgreloch · 2026-07-21T09:22:14Z

+typedef enum threadTimeKind {
+	kSystem,
+	kUser,
+} threadTimeKind_t;


Not sure about the naming of this enum. Usually named kernel enums follow xyz_t -> values: xyz_a, xyz_b, xyz_c etc.

There are two conventions, one is the one you mentioned, the other one is the one I used.
I don't think we have guidelines regarding enum definition styles.

New enums follow the style I have described, look in the code. Your convention is not used anywhere except for message types, which are exported. Besides, kSystem is simply cryptic.

TASK: RTOS-1382

etiaro

Please ensure all kernel/userspace boundaries are included, as we don't always pass through a syscall return, e.g. through a few calls of hal_jmp.

Consider what should happen on execve syscalls (currently the time will go over to the exec'd process, but maybe that's fine? Is it standardized in any way?).

Another boundaries are user interrupts and exceptions (especially pageFault, as we pretend to support lazy-allocations which would make it happen in regular execution).
Not sure what to do about these, maybe nothing, maybe leave a memo in a comment at least.

oI0ck · 2026-07-22T19:35:39Z

@etiaro Linux differentiates between 'user time' vs 'system time' and 'irq time', we can do that too I think.

oI0ck mentioned this pull request Jul 20, 2026

psh: define cpu time in terms of addition of system and user time phoenix-rtos/phoenix-rtos-utils#270

Open

14 tasks

gemini-code-assist Bot reviewed Jul 20, 2026

View reviewed changes

Comment thread proc/threads.c Outdated

github-advanced-security AI found potential problems Jul 20, 2026

View reviewed changes

Comment thread proc/threads.c Fixed

Comment thread proc/threads.c Fixed

oI0ck force-pushed the michal.lach/system_time branch from 7402e53 to 9859532 Compare July 20, 2026 19:52

oI0ck marked this pull request as ready for review July 20, 2026 20:39

oI0ck requested a review from a team July 20, 2026 20:39

adamgreloch reviewed Jul 21, 2026

View reviewed changes

oI0ck force-pushed the michal.lach/system_time branch from 9859532 to 34a9bd6 Compare July 21, 2026 18:47

oI0ck changed the title ~~threads: split cpuTime into user and system time~~ !threads: split cpuTime into user and system time Jul 21, 2026

!threads: split cpuTime into user and system time

ef90a2b

TASK: RTOS-1382

oI0ck force-pushed the michal.lach/system_time branch from 34a9bd6 to ef90a2b Compare July 22, 2026 15:19

oI0ck requested review from adamgreloch and etiaro July 22, 2026 15:19

etiaro requested changes Jul 22, 2026

View reviewed changes

Uh oh!

Conversation

oI0ck commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

Special treatment

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

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!

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!

etiaro left a comment

Choose a reason for hiding this comment

Uh oh!

oI0ck commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oI0ck commented Jul 20, 2026 •

edited

Loading

github-actions Bot commented Jul 20, 2026 •

edited

Loading