!syscalls/threadsinfo: return more granular thread information by oI0ck · Pull Request #720 · phoenix-rtos/phoenix-rtos-kernel

oI0ck · 2025-12-05T20:39:22Z

Description

This commit means to make threadinfo more flexible, by being able to query a thread identified by tid and query for specific thread attributes as specified in flags bitmask.

Motivation and Context

threadsinfo is not very flexible and causes unnecessary overhead in programs using it.

Moreover, previously there was no was of inexpensively checking amount of present threads in the kernel, therefore the buffer passed to threadsinfo may have been often reallocated to check all threads. Now threadsinfo, if passed a PH_THREADINFO_OPT_THREADCOUNT flag will yield no information in passed buffer, but will return amount of currently present threads.

JIRA: RTOS-1187

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: (test PR is WIP).
Tested by hand on: ia32-generic-qemu, riscv64-generic-qemu, sparcv8leon-generic-qemu, armv7a9-zynq7000-qemu, armv7r5f-zynqmp-qemu.

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).
sys/threads: modify threadsinfo declaration libphoenix#454
psh: modify threadsinfo calls according to kernel changes phoenix-rtos-utils#251
kernel: document threadsinfo modifications phoenix-rtos-doc#247
I will merge this PR by myself when appropriate.

include/sysinfo.h

gemini-code-assist · 2025-12-05T20:39:41Z

Summary of Changes

Hello @oI0ck, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors the threadsinfo system call to improve its flexibility and efficiency. Previously, the call would retrieve a fixed set of information for all threads, leading to potential overhead. The updated implementation now allows users to specify a target thread ID and a bitmask of desired attributes, enabling more granular and performant data retrieval. Additionally, a new option has been introduced to simply count the active threads, addressing a common performance bottleneck.

Highlights

Enhanced threadsinfo flexibility: The threadsinfo syscall can now query specific threads by tid and retrieve only desired attributes using a flags bitmask.
Optimized thread counting: A new PH_THREADINFO_OPT_THREADCOUNT flag allows for efficient retrieval of the total number of threads without populating a buffer, reducing overhead.
Breaking Change: The modifications to the threadsinfo syscall signature and behavior constitute a breaking change.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces a more flexible threadsinfo syscall, which is a great improvement for querying thread information. The refactoring of proc_threadsList into smaller helper functions is also a positive change. However, I've identified a few critical issues that must be addressed. These include a typo that will cause a compilation error, and incomplete validation of user-provided arguments in the syscall handler, which poses a security risk. Please see my detailed comments for each issue.

proc/threads.c

syscalls.c

proc/threads.c

github-actions · 2025-12-08T12:13:00Z

Unit Test Results

9 522 tests - 1 8 895 ✅ - 36 52m 13s ⏱️ - 3m 7s
583 suites ±0 592 💤 ± 0
1 files ±0 35 ❌ +35

For more details on these failures, see this check.

Results for commit 80c5dc5. ± Comparison against base commit 0efd690.

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

phoenix-rtos-tests/sys/perf ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/sys/perf.perf_test_common.start_read_finish
phoenix-rtos-tests/sys/perf ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/sys/perf.perf_test_trace.trace_start_stop_finish

phoenix-rtos-tests/sys/perf ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/sys/perf

♻️ This comment has been updated with latest results.

oI0ck · 2025-12-08T17:05:33Z

CI test fails are a result of the fact that this PR breaks compatibility and it requires correspondent changes in libphoenix and phoenix-rtos-utils, as apparent in PR's description

proc/threads.c

include/sysinfo.h

proc/threads.c

adamgreloch · 2025-12-09T10:30:47Z

proc/threads.c

+}
+
+
+static inline int _proc_calculateVmem(thread_t *thread)


prefixed with _ - does it require any external sync? there is a spinlock inside, so I guess not?

It uses a thread_t *, so we have to guarantee that it won't be deallocated, therefore it should be called with threads_common.lock set, as all the other functions here.

I would make it explicit in the comment then, as the use of thread_t * alone doesn't imply that - usually you just up the refcnt on a thread, guaranteeing that it won't get deallocated when you need it and threadsinfo is an exception.

Please add a comment similar to one preceeding _proc_threadInfo

adamgreloch · 2025-12-09T10:30:54Z

proc/threads.c



-int proc_threadsList(int n, threadinfo_t *info)
+static inline void _proc_makeProcessName(thread_t *thread, char *name, int sz)


prefixed with _ - does it require any external sync?

proc/threads.c

adamgreloch · 2025-12-09T10:42:22Z

proc/threads.c

+				i++;
+				t = lib_idtreeof(thread_t, idlinkage, lib_idtreeNext(&t->idlinkage.linkage));


I wonder if we shouldn't just maintain a global thread count instead

Might be, though I'd say that's good enough for the amount of changes here. We'd have to guarantee that this counter will be kept in sync with node count in tree, which I'd say is besides the point in this PR.

Yeah, you're right. Maybe add a TODO/REVISIT then for now?

julianuziemblo

MISRA nits (for a better (worse) evening)

include/sysinfo.h

julianuziemblo · 2025-12-18T14:55:05Z

proc/threads.c

-	int i = 0, argc;
-	unsigned int len, space;
-	thread_t *t;
+	int i, len, left = sz;


I'd leave left as int (or maybe long?) and change type of i and len and sz to size_t or unsigned int, as sz is always passed from sizeof.

Then of course remember to cast to int when assiging to left as for MISRA...

Changed all of them to size_t as well as parameter sz, since we always pass sizeof

Now the test in line 2103 (left <= 0) is pointless because left cannot be < 0. You should change back the type as this probably introduces an error

eh, right, was tired when commiting that, i'm changing left to long

proc/threads.c

julianuziemblo

More comments to threadsinfo_t data structure. Some of its members seem to have improper types with no explanation and we could fix it while at it.

julianuziemblo · 2025-12-19T07:04:51Z

include/sysinfo.h


 typedef struct _threadinfo_t {
 	pid_t pid;
 	unsigned int tid;


tid seems to sometimes be signed, and sometimes unsigned. Broader question here - do you happen to know if there's a reason for this? Even proc_getTid() returns int. @adamgreloch

Internally tid is an idtree key which is signed to support POSIX handles

A signed thread identifier makes little sense as a key in a tree structure. By convention, values such as PID are unique ID based on a counter, so making it signed is seemingly pointless, unless to signal a possibility of returning an error within the negative domain.

Yes, I agree. My (vague) comment's intention was to explain why tid sometimes happens to be an int and AFAIK it's purely a consequence of idtree implementation.

We should probably clean this up, make it consistently unsigned int across the kernel or even introduce tid_t to control the types even further (outside of this PR).

julianuziemblo · 2025-12-19T07:05:49Z

include/sysinfo.h

 	int priority;
 	int state;


state and priority seem to be bitmasks too, could probably be changed to unsigned int

priority is an index into thread_common.ready, which is an array of arrays containing threads divided by their priority. Signedness of this value is more engraved in the system, as apparent by proc_threadPriority and the priority syscall, changing this seems unnecessary, at least as a part of this PR.

state is an enum with 3 possible values, READY (0U), SLEEP (1U), GHOST (2U), so while, sure, we could change this type here, it is not only breaking, but it might be, that in the future, someone may want to introduce a state signalling an error as a negative value, which would the require changing it back to int, so further breaking changes.

I'd personally stray from making further breaking changes within this PR, as it will be harder to merge it, reason about implications of it, and so on and so forth. The fact that it is already breaking should not be a signal to break even more, without a proper discussion. Review comments are a place to talk through changes made in a specific PR, to probably make it better, not introduce more changes unrelated to the PR.

This should be talked through in-person or on our issue tracker, so if you are keen making those changes, please create a new ticket, I'll be glad to participate in the discussion.

Valid. From our perspective however, there isn't really ever better time for this discussions than these PRs. But it's hard to tell sometimes how much the discussion is connected with the topic. The recent MISRA changes also affect this mindset somewhat.
These points were brought up here because you're already changing the interface and fixing it in other subrepos, so I wouldn't agree these changes are hard to reason about as there are little users to change.
But, if you think that this is too much of a breaking change, for me it can stay as is for now - the types here seem really weird though, that's what my comments are about.

@oI0ck makes a good point. I invite you both to RTOS-1196 :)

include/sysinfo.h

Darchiv · 2026-01-12T16:36:34Z

threadsinfo was already tinkered with. See: https://github.com/phoenix-rtos/phoenix-rtos-kernel/pull/661/changes/BASE..c74eaa2757cedadb6b6d42e33fcfff97209a563e (proc_threadsList -> proc_threadsIter + process_getName)

The referenced change will be reviewed and merged first and then this PR should be changed accordingly

perf/trace.c

proc/threads.c

include/sysinfo.h

threadsinfo is not very flexible, takes a long time and causes unnecessary overhead in programs using it. This commit means to make it more flexible, by being able to query a thread identified by tid and query for specific thread attributes as specified in flags bitmask. Moreover, previously there was no was of inexpensively checking amount of present threads in the kernel, therefore the buffer passed to threadinfo may have been often reallocated to check all threads. Now threadsinfo, if passed a PH_THREADINFO_OPT_THREADCOUNT flag will yield no information in passed buffer, but will return amount of currently present threads. JIRA: RTOS-1187

JIRA: RTOS-1088

julianuziemblo

nits, otherwise LGTM

julianuziemblo · 2026-02-18T08:45:43Z

proc/threads.c

+	now = _proc_gettimeRaw();
+	if (thread->process != NULL) {
+		info->pid = process_getPid(thread->process);
+		info->ppid = 0;


Nit: set info->ppid to 0 outside of if - else as it's always set

julianuziemblo · 2026-02-18T09:25:24Z

proc/threads.h



-typedef void (*proc_threadsListCb_t)(void *arg, int i, threadinfo_t *info);
+typedef void (*proc_threadsListCb_t)(void *arg, threadinfo_t *info);


Nit: new coding standard recommends Fn_t suffix for function types

github-actions bot reviewed Dec 5, 2025

View reviewed changes

include/sysinfo.h Outdated Show resolved Hide resolved

include/sysinfo.h Outdated Show resolved Hide resolved

oI0ck force-pushed the michal.lach/threadsinfo branch from 55881ff to c647754 Compare December 5, 2025 20:45

This was referenced Dec 5, 2025

sys/threads: modify threadsinfo declaration phoenix-rtos/libphoenix#454

Open

psh: modify threadsinfo calls according to kernel changes phoenix-rtos/phoenix-rtos-utils#251

Open

time: handle CLOCK_THREAD_CPUTIME_ID phoenix-rtos/libphoenix#455

Draft

gemini-code-assist bot reviewed Dec 5, 2025

View reviewed changes

proc/threads.c Outdated Show resolved Hide resolved

syscalls.c Show resolved Hide resolved

oI0ck force-pushed the michal.lach/threadsinfo branch from c647754 to 5104b56 Compare December 5, 2025 20:50

oI0ck requested review from Darchiv, adamgreloch, agkaminski and nalajcie December 5, 2025 20:52

oI0ck force-pushed the michal.lach/threadsinfo branch 2 times, most recently from 3c6ba52 to 93985e7 Compare December 8, 2025 11:45

github-actions bot reviewed Dec 8, 2025

View reviewed changes

oI0ck force-pushed the michal.lach/threadsinfo branch from 93985e7 to fbb7eaf Compare December 8, 2025 11:57

oI0ck force-pushed the michal.lach/threadsinfo branch from fbb7eaf to e3f8392 Compare December 8, 2025 17:01

oI0ck marked this pull request as ready for review December 8, 2025 17:05

oI0ck force-pushed the michal.lach/threadsinfo branch from e3f8392 to 36b7461 Compare December 8, 2025 18:11

adamgreloch reviewed Dec 9, 2025

View reviewed changes

oI0ck force-pushed the michal.lach/threadsinfo branch from 36b7461 to bad1fa0 Compare December 9, 2025 20:29

julianuziemblo reviewed Dec 18, 2025

View reviewed changes

oI0ck force-pushed the michal.lach/threadsinfo branch from bad1fa0 to ee5ddc6 Compare December 18, 2025 20:17

julianuziemblo reviewed Dec 19, 2025

View reviewed changes

adamgreloch changed the title ~~syscalls/threadsinfo: return more granular thread information~~ !syscalls/threadsinfo: return more granular thread information Dec 19, 2025

oI0ck force-pushed the michal.lach/threadsinfo branch from ee5ddc6 to e2d308f Compare December 19, 2025 17:32

oI0ck requested a review from ziemleszcz January 5, 2026 11:03

oI0ck force-pushed the michal.lach/threadsinfo branch from e2d308f to 80c5dc5 Compare February 10, 2026 18:23

github-advanced-security bot found potential problems Feb 10, 2026

View reviewed changes

julianuziemblo reviewed Feb 17, 2026

View reviewed changes

include/sysinfo.h Show resolved Hide resolved

oI0ck added 2 commits February 17, 2026 10:54

syscalls/threadinfo: misrify

bd88e9f

JIRA: RTOS-1088

oI0ck force-pushed the michal.lach/threadsinfo branch from b482830 to bd88e9f Compare February 17, 2026 09:55

julianuziemblo approved these changes Feb 18, 2026

View reviewed changes



		int proc_threadsList(int n, threadinfo_t *info)
		static inline void _proc_makeProcessName(thread_t thread, char name, int sz)

		i++;
		t = lib_idtreeof(thread_t, idlinkage, lib_idtreeNext(&t->idlinkage.linkage));



		typedef void (proc_threadsListCb_t)(void arg, int i, threadinfo_t *info);
		typedef void (proc_threadsListCb_t)(void arg, threadinfo_t *info);

Conversation

oI0ck commented Dec 5, 2025 • 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!

Uh oh!

Uh oh!

gemini-code-assist bot commented Dec 5, 2025

Summary of Changes

Highlights

Footnotes

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

oI0ck commented Dec 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

julianuziemblo left a comment

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!

Uh oh!

Uh oh!

Uh oh!

julianuziemblo left a comment

Choose a reason for hiding this comment

oI0ck commented Dec 5, 2025 •

edited

Loading

github-actions bot commented Dec 8, 2025 •

edited

Loading

oI0ck Dec 19, 2025 •

edited

Loading