!syscalls/threadsinfo: return more granular thread information #720

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 462 tests ±0 8 839 ✅ - 34 48m 59s ⏱️ -57s
561 suites ±0 589 💤 ± 0
1 files ±0 34 ❌ +34

For more details on these failures, see this check.

Results for commit e2d308f. ± Comparison against base commit cbfa646.

♻️ 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.

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?

adamgreloch · 2025-12-09T10:35:49Z

proc/threads.c

+	if (tid == PH_THREADINFO_THREADS_ALL) {
+		t = lib_treeof(thread_t, idlinkage, lib_rbMinimum(threads_common.id.root));
+
+		if ((flags & PH_THREADINFO_OPT_THREADCOUNT) == PH_THREADINFO_OPT_THREADCOUNT) {
+			do {
+				i++;
+				t = lib_idtreeof(thread_t, idlinkage, lib_idtreeNext(&t->idlinkage.linkage));
+			} while (t != NULL);
+
+			(void)proc_lockClear(&threads_common.lock);
+
+			return i;
 		}
-		else
-#endif
-				if (map != NULL) {
-			(void)proc_lockSet(&map->lock);
-			entry = lib_treeof(map_entry_t, linkage, lib_rbMinimum(map->tree.root));

-			while (entry != NULL) {
-				info[i].vmem += (int)entry->size;
-				entry = lib_treeof(map_entry_t, linkage, lib_rbNext(&entry->linkage));
-			}
-			(void)proc_lockClear(&map->lock);
+		while (i < n && t != NULL) {
+			proc_threadInfo(t, flags, &info[i]);
+			i++;
+			t = lib_idtreeof(thread_t, idlinkage, lib_idtreeNext(&t->idlinkage.linkage));
 		}
-		else {
-			/* No action required */
+	}
+	else {
+		t = lib_idtreeof(thread_t, idlinkage, lib_idtreeFind(&threads_common.id, tid));
+		if (t == NULL) {
+			(void)proc_lockClear(&threads_common.lock);
+			return -ENOENT;
 		}

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


maybe wrap it in do { ... } while (0); and just break instead of returns inside to always converge into same branch with lockClear?

Right, that is pretty ugly. I've rewritten PH_THREADINFO_THREADS_ALL as a do-while loop and it's better.

Oh, what I meant was that since all your if-else returns were not nested in a loop, you could just wrap everything in the do { ... } while (0); and replace any branch like

if (....) { ... } else { t = lib_idtreeof(thread_t, idlinkage, lib_idtreeFind(&threads_common.id, tid)); if (t == NULL) { (void)proc_lockClear(&threads_common.lock); return -ENOENT; } } (void)proc_lockClear(&threads_common.lock); return i;

with

do { if (....) { ... } else { t = lib_idtreeof(thread_t, idlinkage, lib_idtreeFind(&threads_common.id, tid)); if (t == NULL) { i = -ENOENT; break; } } } while (0); (void)proc_lockClear(&threads_common.lock); return i;

so that the code doesn't diverge inside the if-else. This way there is always one returning path that clears the threads_common.lock and you don't need to remember about in other branches.

Yes, I know what you mean, but it still looks a bit better, since in PH_THREADINFO_THREADS_ALL case, we will have to use the loop either way, so making two loops inside of two branches (when they do mostly the same thing) is kinda ugly and the way I rewrote it also addresses having two return points.

There is still one return in else, that's what confused me, but if you think that forcibly reducing the divergence would clutter the code more than solve anything here, then it's fine by me

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 · 2025-12-18T15:10:55Z

proc/threads.c

+		entry = thread->process->entries;
+		if (entry != NULL) {
+			do {
+				vmem += (int)entry->size;


ik this would break the interface, but is there a reason [threadinfo_t]::vmem is int? if not - could it be changed to unsigned (or size_t) to avoid these awkward casts?

After a quick look: this shouldn't break much, at least in the main repo

+1 - these changes are already breaking, so IMO this is a good opportunity to tidy up types in threadinfo_t and potentially reduce casts

so another thing if this is breaking: signal it with ! in PR title and checkbox in description

I'm dropping the vmem type change, correcting those type may have bigger implications and should be considered on its own.

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.

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.

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

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



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

!syscalls/threadsinfo: return more granular thread information #720

Are you sure you want to change the base?

!syscalls/threadsinfo: return more granular thread information #720

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!

Choose a reason for hiding this comment

Uh oh!

adamgreloch Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

julianuziemblo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

oI0ck commented Dec 5, 2025 •

edited

Loading

github-actions bot commented Dec 8, 2025 •

edited

Loading

adamgreloch Dec 10, 2025 •

edited

Loading

adamgreloch Dec 18, 2025 •

edited

Loading

oI0ck Dec 19, 2025 •

edited

Loading