proc: Add futexes by kamilaboba123 · Pull Request #668 · phoenix-rtos/phoenix-rtos-kernel

kamilaboba123 · 2025-07-10T10:32:43Z

Implement futexes

Description

Implement futexes, add new syscalls: futex_wait(), futex_wakeup()

Motivation and Context

Futexes fix performance issues with the current mutex implementation, by reducing
the amount of syscalls.

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

ia32-generic-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).
I will merge this PR by myself when appropriate.

gemini-code-assist

Summary of Changes

Hello @kamilaboba123, 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 enhances the operating system's synchronization capabilities by integrating futexes. The primary goal is to boost the performance of existing mutex implementations by minimizing costly kernel transitions, thereby making inter-process and inter-thread communication more efficient. This is a foundational change that provides a low-level primitive for building more performant synchronization mechanisms.

Highlights

New Feature: Futexes: I've introduced the core implementation of Futexes (Fast Userspace Mutexes) to improve synchronization performance by reducing the number of kernel-mode context switches for common mutex operations. This includes the definition of futex structures and their associated sleep queue management.
New Syscalls: Two new syscalls, futex_wait and futex_wakeup, have been added to allow userspace processes to interact with the new futex mechanism. These syscalls enable threads to wait on a futex address and to wake up threads waiting on a specific futex address, respectively.
Process and Thread Integration: The process_t structure has been extended to include an array of futex_sleepqueue_t to manage futex waits per process. Additionally, a new proc_threadSleep2 function was introduced in threads.c to provide a more granular sleep mechanism for futex operations, allowing the raw errno from rescheduling to be returned.
Build System Update: The proc/Makefile has been updated to include the new futex.o object file, ensuring the futex implementation is compiled and linked into the system.

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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

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 issue 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 is currently in preview and 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 to provide feedback.

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 implements futexes, adding new syscalls futex_wait() and futex_wakeup(). Several areas in proc/futex.c require locking to prevent race conditions. The function name proc_threadSleep2 in proc/threads.c could be more descriptive.

proc/futex.c

proc/threads.c

github-actions · 2025-07-10T11:01:25Z

Unit Test Results

8 201 tests +162 7 677 ✅ +162 44m 11s ⏱️ + 3m 20s
479 suites ± 0 524 💤 ± 0
1 files ± 0 0 ❌ ± 0

Results for commit 47aa3a5. ± Comparison against base commit 675342c.

♻️ This comment has been updated with latest results.

proc/threads.c

proc/futex.c

proc/process.c

proc/threads.h

proc/futex.c

proc/futex.h

proc/futex.c

ziemleszcz · 2025-08-04T09:22:27Z

proc/futex.c

+		LIST_REMOVE(&sleepqueue->waitctxs, wc);
+		tmp = atomic_load(&wc->thread);
+		atomic_store(&wc->thread, NULL);
+		if (proc_threadWakeupOne(tmp)) {


What if this thread was killed while it was still waiting? Are we handling that case properly here?

proc/process.h

proc/userintr.c

syscalls.c

proc/userintr.h

proc/userintr.c

proc/futex.c

ziemleszcz · 2025-08-11T07:37:41Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces futexes to the kernel, a significant feature that should improve synchronization performance. The implementation includes new syscalls for futex operations and refactors user-level interrupt handling to use futexes instead of condition variables. The old mutex and condition variable syscalls have been correctly removed.

Overall, the changes are well-structured. However, I've identified a critical bug in the proc_futexWakeup function that would cause it to wake up an incorrect number of threads. I've also included a comment on a potential design limitation regarding the fixed size of the futex hash table.

proc/futex.c

gemini-code-assist · 2025-08-11T07:39:05Z

proc/futex.h

+#define FUTEX_SLEEPQUEUES_BITS 6
+#define FUTEX_SLEEPQUEUES_SIZE (1U << FUTEX_SLEEPQUEUES_BITS)
+#define FUTEX_SLEEPQUEUES_MASK (FUTEX_SLEEPQUEUES_SIZE - 1)


The size of the futex sleep queue hash table is fixed at 64 entries per process. If a process uses more than 64 distinct futexes concurrently, _proc_allocFutexSleepQueue will fail, and futex operations will return -ENOMEM. While this might be sufficient for many use cases, it could be a limitation for complex applications with many synchronization objects.

Consider if this size should be larger or configurable to avoid this potential resource exhaustion.

proc/futex.h

syscalls.c

proc/futex.c

github-actions · 2025-08-25T16:41:50Z

proc/userintr.c

+#include "futex.h"
+
+
+#define COND_BROADCAST (((u32)-1) / 2)


[clang-format-pr] _{reported by reviewdog 🐶}
suggested fix

Suggested change

#define COND_BROADCAST (((u32)-1) / 2)

#define COND_BROADCAST (((u32) - 1) / 2)

Darchiv

I would prefer this change to be split into: 1) a commit which adds futex syscalls in a backward-compatible manner (provides just the new waiting mechanism); 2) a commit which makes current sync mechanisms (mutex etc.) use futexes. This way it will be easier to test futexes alone, without forcing the whole system to use them.

Darchiv · 2025-08-27T08:32:09Z

include/syscalls.h

+	ID(sys_uname) \
+	\
+	ID(phFutexWait) \
+	ID(phFutexWakeup)


FYI: You remove syscalls, so it doesn't matter if you add new ones in a backward compatible manner (as last) or not, because it is already a breaking change which requires a rebuild. phFutexWait etc. can be placed by similar primitive syscalls, e.g. phInterrupt.

proc/futex.h

Darchiv · 2025-08-27T09:28:27Z

proc/futex.c

+
+	key = address >> 3;
+	key ^= key >> FUTEX_SLEEPQUEUES_BITS;
+	idx = key & FUTEX_SLEEPQUEUES_MASK;


Not obvious what this bit-mangling does. Add a comment which describes that this is a simple hash-table implementation or something.

Or make generic macros which will contain these operations (and deduplicate with _proc_getFutexSleepQueue()), with a self-documenting name (as we all love self-documenting code) - this way a comment will not be necessary. Yes, the implementation should be comprehensible by just looking at the code.

Darchiv · 2025-08-27T09:32:54Z

proc/futex.h

+
+int proc_futexWait(_Atomic(u32) *address, u32 value, time_t timeout, int clockType);
+int proc_futexWakeup(struct _process_t *process, _Atomic(u32) *address, u32 wakeCount);
+futex_sleepqueue_t *_proc_getFutexSleepQueue(struct _process_t *process, addr_t address);


Why export if not used outside of futex.c?

Darchiv · 2025-08-27T09:40:03Z

proc/futex.c

+	int err = EOK;
+	time_t waitTime = 0, offs;
+
+	if (clockType != PH_CLOCK_REALTIME && clockType != PH_CLOCK_RELATIVE && clockType != PH_CLOCK_MONOTONIC) {


This can be replaced by just adding a default case for the switch-case below.

clockType is used only when timeout != 0, so it makes sense to only check it in this case (performance + code size). Also, if not using timeout, the user should be able to use this function without looking up valid values for clockType, i.e. just pass 0 or -1. If PH_CLOCK_* values were completely arbitrary and did not include 0, then the user would be forced to use a specific PH_CLOCK_* value even if not using timeout or an additional value would have to be added (e.g. PH_CLOCK_NONE).

Darchiv · 2025-08-27T10:02:38Z

proc/futex.c

+	hal_spinlockSet(&current->process->futexSqSpinlock, &sqSc);
+	sq = _proc_getFutexSleepQueue(current->process, (addr_t)address);
+	if (sq == NULL) {
+		sq = _proc_allocFutexSleepQueue(current->process, (addr_t)address);


So, basically, once we allocate a sleep queue, we never free it.

I assume that you wanted to mimic futex syscall from other OSes, but can't we actually have a futexCreate syscall which will allocate a sleep queue (kernel side of futex, not including the userspace context) on idtree and release it via resourcesDestroy()?

Darchiv · 2025-08-28T09:39:00Z

proc/futex.c

+#include "futex.h"
+
+
+u32 _proc_futexTableHash(addr_t address)


Not exported, used internally. Should be static

Darchiv · 2025-08-28T09:54:09Z

proc/futex.c

+
+	if (atomic_load(address) != value) {
+		err = -EAGAIN;
+		goto done;


Please avoid using goto (see: coding convention on docs). If you really need to use the non-structured paradigm (or it vastly increases readability), consider using a do-while. For example:

phoenix-rtos-kernel/posix/unix.c

Lines 407 to 463 in e35fd78

do {

if (s->type != SOCK_STREAM && s->type != SOCK_SEQPACKET) {

err = -EOPNOTSUPP;

break;

}

if (!(s->state & US_LISTENING)) {

err = -EINVAL;

break;

}

if (s->nonblock != 0 && s->connecting == NULL) {

err = -EWOULDBLOCK;

break;

}

new = unixsock_alloc(&newid, s->type, nonblock);

if (new == NULL) {

err = -ENOMEM;

break;

}

if ((v = vm_kmalloc(new->buffsz)) == NULL) {

unixsock_put(new);

unixsock_put(new);

err = -ENOMEM;

break;

}

_cbuffer_init(&new->buffer, v, new->buffsz);

hal_spinlockSet(&s->spinlock, &sc);

s->state |= US_ACCEPTING;

while (s->connecting == NULL) {

proc_threadWait(&s->queue, &s->spinlock, 0, &sc);

}

r = s->connecting;

LIST_REMOVE(&s->connecting, r);

s->state &= ~US_ACCEPTING;

hal_spinlockClear(&s->spinlock, &sc);

/* FIXME: handle connecting socket removal */

hal_spinlockSet(&r->spinlock, &sc);

r->state &= ~(US_PEER_CLOSED | US_CONNECTING);

r->remote = new;

new->remote = r;

proc_threadWakeup(&r->queue);

hal_spinlockClear(&r->spinlock, &sc);

err = new->id;

unixsock_put(new);

} while (0);

But it seems an overkill for just one branch (goto).

proc/futex.h

JIRA: RTOS-1074

TODO: - Remove condition variables and mutexes from resources and their implementations (proc/mutex.c, proc/cond.c) - Remove syscall implementations for phMutexCreate, phMutexLock, mutexTry, mutexUnlock, phCondCreate, phCondWait, condSignal, condBroadcast from syscalls.c JIRA: RTOS-1074

gemini-code-assist bot reviewed Jul 10, 2025

View reviewed changes

proc/futex.c Outdated Show resolved Hide resolved

proc/futex.c Outdated Show resolved Hide resolved

proc/futex.c Outdated Show resolved Hide resolved

proc/futex.c Outdated Show resolved Hide resolved

proc/threads.c Outdated Show resolved Hide resolved

ziemleszcz marked this pull request as draft July 10, 2025 10:43

ziemleszcz assigned kamilaboba123 Jul 10, 2025

kamilaboba123 force-pushed the kamilaboba123/RTOS-1074 branch 3 times, most recently from 8ca0353 to 6029fe3 Compare July 10, 2025 14:37

github-actions bot reviewed Jul 10, 2025

View reviewed changes

proc/threads.c Outdated Show resolved Hide resolved

kamilaboba123 force-pushed the kamilaboba123/RTOS-1074 branch 5 times, most recently from 24d3ff7 to 6d26238 Compare July 10, 2025 15:31

ziemleszcz reviewed Jul 10, 2025

View reviewed changes

proc/futex.c Outdated Show resolved Hide resolved

proc/futex.c Outdated Show resolved Hide resolved

proc/futex.c Outdated Show resolved Hide resolved

proc/futex.c Outdated Show resolved Hide resolved

github-actions bot reviewed Jul 14, 2025

View reviewed changes

proc/futex.c Show resolved Hide resolved

kamilaboba123 force-pushed the kamilaboba123/RTOS-1074 branch from b3aea9c to 4194625 Compare July 14, 2025 09:38

github-actions bot reviewed Jul 15, 2025

View reviewed changes

kamilaboba123 force-pushed the kamilaboba123/RTOS-1074 branch 2 times, most recently from b0cba44 to 9caff4e Compare July 15, 2025 11:00

github-actions bot reviewed Jul 15, 2025

View reviewed changes

proc/process.c Outdated Show resolved Hide resolved

proc/process.c Outdated Show resolved Hide resolved

github-actions bot reviewed Jul 15, 2025

View reviewed changes

proc/process.c Outdated Show resolved Hide resolved

proc/process.c Outdated Show resolved Hide resolved

ziemleszcz reviewed Jul 21, 2025

View reviewed changes

kamilaboba123 force-pushed the kamilaboba123/RTOS-1074 branch 4 times, most recently from 678318f to c15f0bf Compare July 21, 2025 15:58

github-actions bot reviewed Jul 21, 2025

View reviewed changes

proc/futex.c Outdated Show resolved Hide resolved

kamilaboba123 force-pushed the kamilaboba123/RTOS-1074 branch from c15f0bf to 09f272a Compare July 21, 2025 16:00

ziemleszcz reviewed Jul 22, 2025

View reviewed changes

proc/futex.c Outdated Show resolved Hide resolved

proc/futex.c Outdated Show resolved Hide resolved

proc/futex.c Outdated Show resolved Hide resolved

proc/futex.c Outdated Show resolved Hide resolved

kamilaboba123 force-pushed the kamilaboba123/RTOS-1074 branch from 47aa3a5 to c5b8000 Compare August 1, 2025 12:10

ziemleszcz reviewed Aug 4, 2025

View reviewed changes

syscalls.c Show resolved Hide resolved

syscalls.c Show resolved Hide resolved

proc/userintr.h Outdated Show resolved Hide resolved

proc/userintr.c Outdated Show resolved Hide resolved

kamilaboba123 force-pushed the kamilaboba123/RTOS-1074 branch from c5b8000 to 198d23b Compare August 5, 2025 08:48

ziemleszcz reviewed Aug 5, 2025

View reviewed changes

proc/futex.c Outdated Show resolved Hide resolved

proc/futex.c Outdated Show resolved Hide resolved

kamilaboba123 force-pushed the kamilaboba123/RTOS-1074 branch 5 times, most recently from fc4bd9a to 5d194f6 Compare August 8, 2025 12:51

gemini-code-assist bot reviewed Aug 11, 2025

View reviewed changes

ziemleszcz reviewed Aug 11, 2025

View reviewed changes

proc/futex.h Outdated Show resolved Hide resolved

syscalls.c Outdated Show resolved Hide resolved

proc/futex.c Show resolved Hide resolved

proc/futex.c Outdated Show resolved Hide resolved

proc/futex.c Show resolved Hide resolved

kamilaboba123 force-pushed the kamilaboba123/RTOS-1074 branch 4 times, most recently from 6b06aa6 to 33bd2eb Compare August 14, 2025 16:45

ziemleszcz reviewed Aug 18, 2025

View reviewed changes

proc/futex.c Show resolved Hide resolved

kamilaboba123 force-pushed the kamilaboba123/RTOS-1074 branch 3 times, most recently from 003ec29 to 580ed66 Compare August 25, 2025 16:41

github-actions bot reviewed Aug 25, 2025

View reviewed changes

Darchiv reviewed Aug 27, 2025

View reviewed changes

kamilaboba123 force-pushed the kamilaboba123/RTOS-1074 branch from 580ed66 to f6fd125 Compare August 28, 2025 09:33

Darchiv reviewed Aug 28, 2025

View reviewed changes

kamilaboba123 force-pushed the kamilaboba123/RTOS-1074 branch 2 times, most recently from 77b0b56 to 0e79cfc Compare August 28, 2025 11:00

kamilaboba123 added 2 commits August 28, 2025 13:24

proc: Implement futexes

e3719aa

JIRA: RTOS-1074

kamilaboba123 force-pushed the kamilaboba123/RTOS-1074 branch from 0e79cfc to ac4f6a1 Compare August 28, 2025 11:24

	#define COND_BROADCAST (((u32)-1) / 2)
	#define COND_BROADCAST (((u32) - 1) / 2)

	do {
	if (s->type != SOCK_STREAM && s->type != SOCK_SEQPACKET) {
	err = -EOPNOTSUPP;
	break;
	}

	if (!(s->state & US_LISTENING)) {
	err = -EINVAL;
	break;
	}

	if (s->nonblock != 0 && s->connecting == NULL) {
	err = -EWOULDBLOCK;
	break;
	}

	new = unixsock_alloc(&newid, s->type, nonblock);
	if (new == NULL) {
	err = -ENOMEM;
	break;
	}

	if ((v = vm_kmalloc(new->buffsz)) == NULL) {
	unixsock_put(new);
	unixsock_put(new);
	err = -ENOMEM;
	break;
	}

	_cbuffer_init(&new->buffer, v, new->buffsz);

	hal_spinlockSet(&s->spinlock, &sc);
	s->state \|= US_ACCEPTING;

	while (s->connecting == NULL) {
	proc_threadWait(&s->queue, &s->spinlock, 0, &sc);
	}
	r = s->connecting;

	LIST_REMOVE(&s->connecting, r);
	s->state &= ~US_ACCEPTING;
	hal_spinlockClear(&s->spinlock, &sc);

	/* FIXME: handle connecting socket removal */

	hal_spinlockSet(&r->spinlock, &sc);

	r->state &= ~(US_PEER_CLOSED \| US_CONNECTING);
	r->remote = new;
	new->remote = r;

	proc_threadWakeup(&r->queue);
	hal_spinlockClear(&r->spinlock, &sc);

	err = new->id;
	unixsock_put(new);
	} while (0);

Conversation

kamilaboba123 commented Jul 10, 2025

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

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!

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

Uh oh!

Unit Test Results

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ziemleszcz commented Aug 11, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 10, 2025 •

edited

Loading