Development #143
Development #143
Conversation
WalkthroughAdds a new EEVDF scheduler (headers and implementation), wires scheduler selection via a CMake cache variable and feature logic, updates the unified Scheduler API to route to EEVDF paths when enabled, includes the new source in multiple build systems, and renames an MLFQ lock symbol. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Boot
participant Sched as Scheduler (API)
participant EEVDF as EEVDF Core
participant RQ as Runqueue
participant Task as Process
participant IRQ as Timer IRQ
Boot->>Sched: SchedulerInit()
alt VF_SCHEDULER == "EEVDF"
Sched->>EEVDF: EEVDFSchedInit()
EEVDF->>RQ: init runqueue, idle task
else
Note over Sched: existing MLFQ/CFS init path
end
Boot->>Sched: CreateProcess(name, entry)
alt VF_SCHEDULER == "EEVDF"
Sched->>EEVDF: EEVDFCreateProcess(...)
EEVDF->>RQ: EEVDFEnqueueTask()
end
Task->>Sched: Yield()
alt VF_SCHEDULER == "EEVDF"
Sched->>EEVDF: EEVDFYield()
EEVDF->>EEVDF: EEVDFUpdateCurr()
EEVDF->>RQ: EEVDFPickNext()
EEVDF-->>Task: context switch to next
end
IRQ-->>Sched: Schedule(regs)
alt VF_SCHEDULER == "EEVDF"
Sched->>EEVDF: EEVDFSchedule(regs)
EEVDF->>RQ: Check preempt, PickNext
EEVDF-->>Task: switch if needed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CMakeLists.txt(1 hunks)cmake/features.cmake(1 hunks)include/Scheduler.h(1 hunks)kernel/sched/EEVDF.c(1 hunks)kernel/sched/EEVDF.h(1 hunks)kernel/sched/MLFQ.c(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T00:55:58.224Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#106
File: kernel/sched/MLFQ.c:691-694
Timestamp: 2025-09-02T00:55:58.224Z
Learning: The VoidFrame MLFQ scheduler has a critical bug where ProcessRuntimePath uses FormatS() which returns a static buffer, causing all processes to share the same path string. This results in process directory cleanup operations affecting the wrong directories - killing one process can delete another process's /Runtime/Processes directory because they all point to the same FormatS static buffer.
Applied to files:
include/Scheduler.h
🧬 Code graph analysis (4)
kernel/sched/MLFQ.c (1)
kernel/atomic/Spinlock.h (2)
ReadLock(100-111)ReadUnlock(113-119)
include/Scheduler.h (2)
kernel/sched/MLFQ.c (6)
MLFQCreateProcess(1174-1176)MLFQGetCurrentProcess(1233-1238)MLFQYield(1001-1004)MLFQSchedule(771-958)MLFQKillProcess(462-464)MLFQGetCurrentProcessByPID(1240-1252)kernel/sched/EEVDF.c (7)
EEVDFSchedInit(756-807)EEVDFCreateProcess(809-913)EEVDFGetCurrentProcess(915-920)EEVDFYield(935-940)EEVDFSchedule(663-754)EEVDFKillProcess(942-945)EEVDFGetCurrentProcessByPID(922-933)
kernel/sched/EEVDF.h (1)
kernel/sched/EEVDF.c (29)
EEVDFSchedInit(756-807)EEVDFCreateProcess(809-913)EEVDFGetCurrentProcess(915-920)EEVDFGetCurrentProcessByPID(922-933)EEVDFYield(935-940)EEVDFSchedule(663-754)EEVDFKillProcess(942-945)EEVDFKillCurrentProcess(947-952)EEVDFGetSystemTicks(118-120)EEVDFGetNanoseconds(114-116)EEVDFUpdateClock(658-661)EEVDFCalcDelta(153-156)EEVDFUpdateCurr(177-198)EEVDFCalcSlice(158-175)EEVDFEnqueueTask(515-532)EEVDFDequeueTask(534-550)EEVDFPickNext(552-562)EEVDFCheckPreempt(1067-1080)EEVDFYieldTask(1082-1095)EEVDFWakeupTask(965-973)EEVDFProcessBlocked(954-963)EEVDFCleanupTerminatedProcess(975-977)EEVDFDumpSchedulerState(983-997)EEVDFListProcesses(999-1033)EEVDFGetProcessStats(1035-1050)EEVDFDumpPerformanceStats(1052-1064)EEVDFSetTaskNice(138-147)EEVDFNiceToWeight(126-130)EEVDFNiceToWmult(132-136)
kernel/sched/EEVDF.c (6)
kernel/atomic/Atomics.c (2)
AtomicInc(10-12)AtomicDec(13-15)kernel/etc/Console.c (2)
PrintKernel(174-192)PrintKernelInt(298-322)kernel/atomic/Spinlock.h (6)
SpinLockIrqSave(154-159)SpinUnlockIrqRestore(161-164)SpinLock(27-61)SpinUnlock(150-152)ReadLock(100-111)ReadUnlock(113-119)mm/MemOps.c (2)
FastMemcpy(212-447)FastMemset(23-210)kernel/etc/Format.c (2)
FormatA(280-286)FormatS(288-295)mm/VMem.c (1)
VMemAllocStack(709-741)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
kernel/sched/EEVDF.c (2)
62-62: Fixpid_lockdeclaration for SpinLock compatibility.
pid_lockis declared asirq_flags_tbut passed toSpinLock/SpinUnlock(lines 833, 843), which expect avolatile int*. This type mismatch causes undefined behavior on 64-bit builds where only the lower 32 bits are atomically touched. Declare it asvolatile intto match the other locks and ensure correct atomic operations.Apply this diff:
-static volatile irq_flags_t pid_lock = 0; +static volatile int pid_lock = 0;
688-718: Compute runtime beforeEEVDFUpdateCurr.
EEVDFUpdateCurr(line 691) resetsprev->exec_starttonow(see line 186), so the subsequentruntime = GetNS() - prev->exec_startat line 700 collapses to approximately zero. This prevents accurate time slice checks, causing tasks to appear as if they've consumed no time, which blocks proper preemption and requeue decisions.Apply this diff to measure runtime before updating accounting:
// Save context FastMemcpy(&prev->context, regs, sizeof(Registers)); + // Snapshot elapsed time before updating accounting + uint64_t runtime = GetNS() - prev->exec_start; + // Update runtime statistics EEVDFUpdateCurr(rq, prev); // Check if task should continue running if (LIKELY(prev->state == PROC_RUNNING)) { prev->state = PROC_READY; ready_process_bitmap |= (1ULL << old_slot); // Re-enqueue if it still has time left and is still the best choice uint64_t slice_ns = EEVDFCalcSlice(rq, prev); - uint64_t runtime = GetNS() - prev->exec_start; if (runtime < slice_ns) {
🧹 Nitpick comments (4)
kernel/sched/EEVDF.c (4)
1038-1053: Nested ReadLock pattern is unusual but likely safe.Line 1039 acquires
ReadLockthen line 1040 callsEEVDFGetCurrentProcessByPID, which also acquires the sameReadLock(line 926). While the rwlock implementation allows multiple readers, this nested pattern adds complexity. Consider refactoring to avoid nested locks or document the reentrancy assumption.
215-228: Consider optimizing RB node allocation.The linear search through the bitmap is O(n) where n =
EEVDF_MAX_PROCESSES. For reasonable process counts this is acceptable, but could be optimized usingFastFFSon 32-bit words of the bitmap, similar to howFindFreeSlotFastusesFastFFSon 64-bit bitmaps.
945-955: Incomplete process termination implementation.
EEVDFKillProcessandEEVDFKillCurrentProcessare stubs with TODO comments. Process termination is essential for resource cleanup and preventing leaks. Implement these functions or ensure they're tracked as high-priority tasks.Do you want me to generate an implementation plan for process termination or open a new issue to track this task?
978-980: Cleanup routine needs implementation.
EEVDFCleanupTerminatedProcessis a stub. Without cleanup, terminated processes leak memory and process slots. This should handle stack deallocation, PID bitmap clearing, and slot freeing.Do you want me to generate an implementation for this cleanup routine based on the termination queue pattern already present in the code?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.gitignore(1 hunks)CMakeLists.txt(3 hunks)cmake/cache.cmake(1 hunks)cmake/features.cmake(1 hunks)kernel/sched/EEVDF.c(1 hunks)kernel/sched/EEVDF.h(1 hunks)meson.build(2 hunks)scripts/mangle.py(0 hunks)xmake.lua(3 hunks)
💤 Files with no reviewable changes (1)
- scripts/mangle.py
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
🧰 Additional context used
🧬 Code graph analysis (2)
kernel/sched/EEVDF.c (7)
kernel/sched/MLFQ.c (2)
FastFFS(81-83)FastCLZ(85-87)kernel/atomic/Atomics.c (2)
AtomicInc(10-12)AtomicDec(13-15)kernel/etc/Console.c (2)
PrintKernel(174-192)PrintKernelInt(298-322)kernel/atomic/Spinlock.h (6)
SpinLockIrqSave(154-159)SpinUnlockIrqRestore(161-164)SpinLock(27-61)SpinUnlock(150-152)ReadLock(100-111)ReadUnlock(113-119)mm/MemOps.c (1)
FastMemcpy(212-447)kernel/etc/Format.c (2)
FormatA(280-286)FormatS(288-295)mm/VMem.c (1)
VMemAllocStack(709-741)
kernel/sched/EEVDF.h (1)
kernel/sched/EEVDF.c (29)
EEVDFSchedInit(759-810)EEVDFCreateProcess(812-916)EEVDFGetCurrentProcess(918-923)EEVDFGetCurrentProcessByPID(925-936)EEVDFYield(938-943)EEVDFSchedule(666-757)EEVDFKillProcess(945-948)EEVDFKillCurrentProcess(950-955)EEVDFGetSystemTicks(118-120)EEVDFGetNanoseconds(114-116)EEVDFUpdateClock(661-664)EEVDFCalcDelta(153-159)EEVDFUpdateCurr(180-201)EEVDFCalcSlice(161-178)EEVDFEnqueueTask(518-535)EEVDFDequeueTask(537-553)EEVDFPickNext(555-565)EEVDFCheckPreempt(1070-1083)EEVDFYieldTask(1085-1098)EEVDFWakeupTask(968-976)EEVDFProcessBlocked(957-966)EEVDFCleanupTerminatedProcess(978-980)EEVDFDumpSchedulerState(986-1000)EEVDFListProcesses(1002-1036)EEVDFGetProcessStats(1038-1053)EEVDFDumpPerformanceStats(1055-1067)EEVDFSetTaskNice(138-147)EEVDFNiceToWeight(126-130)EEVDFNiceToWmult(132-136)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
kernel/sched/EEVDF.c (3)
153-159: LGTM: Zero-load guard added.The divide-by-zero guard (lines 154-156) correctly prevents panics when
lw == 0. This fix addresses the critical issue flagged in previous reviews.
886-897: LGTM: Stack and context setup is correct.The stack pointer calculation, alignment, exit stub placement, and register initialization follow proper x86-64 conventions. The stack grows downward from the top, the exit stub ensures graceful termination, and segment selectors are correct for kernel mode.
245-512: LGTM: Red-black tree implementation is correct.The rotation, insertion, deletion, and fixup operations follow standard red-black tree algorithms. The leftmost pointer optimization enables O(1) access to the minimum vruntime task, which is essential for EEVDF's scheduling decisions.
| static inline uint64_t GetNS(void) { | ||
| // Convert APIC ticks to nanoseconds | ||
| // Assuming APIC_HZ is in Hz, convert to ns | ||
| return (APICticks * 1000000000ULL) / APIC_HZ; | ||
| } |
There was a problem hiding this comment.
Guard against divide-by-zero if APIC not initialized.
If APIC_HZ is zero or uninitialized, line 111 triggers a divide-by-zero exception. This could occur during early boot or if APIC initialization fails.
Apply this diff to guard the division:
static inline uint64_t GetNS(void) {
// Convert APIC ticks to nanoseconds
// Assuming APIC_HZ is in Hz, convert to ns
+ if (UNLIKELY(APIC_HZ == 0)) {
+ return 0;
+ }
return (APICticks * 1000000000ULL) / APIC_HZ;
}🤖 Prompt for AI Agents
In kernel/sched/EEVDF.c around lines 108-112, the GetNS function divides by
APIC_HZ which can be zero/uninitialized; add a guard that checks if APIC_HZ == 0
(or otherwise invalid) before performing the division and return a safe default
(e.g., 0) or fallback value/error path to avoid a divide-by-zero; keep the
conversion logic unchanged when APIC_HZ is valid and consider logging or
asserting when APIC_HZ is zero.
| uint64_t EEVDFCalcSlice(EEVDFRunqueue* rq, EEVDFProcessControlBlock* se) { | ||
| uint32_t nr_running = rq->nr_running; | ||
| if (nr_running == 0) return EEVDF_TIME_SLICE_NS; | ||
|
|
||
| uint64_t slice = (EEVDF_TARGET_LATENCY * se->weight) / rq->load_weight; | ||
|
|
||
| // Ensure minimum granularity | ||
| if (slice < EEVDF_MIN_GRANULARITY) { | ||
| slice = EEVDF_MIN_GRANULARITY; | ||
| } | ||
|
|
||
| // Cap maximum slice | ||
| if (slice > EEVDF_MAX_TIME_SLICE_NS) { | ||
| slice = EEVDF_MAX_TIME_SLICE_NS; | ||
| } | ||
|
|
||
| return slice; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Guard slice calculation against zero load weight.
If rq->load_weight is zero at line 165 while nr_running > 0, a divide-by-zero occurs. While unlikely (tasks should have non-zero weights from the nice-to-weight table), accounting bugs could trigger this. Add a guard for robustness.
Apply this diff:
uint64_t EEVDFCalcSlice(EEVDFRunqueue* rq, EEVDFProcessControlBlock* se) {
uint32_t nr_running = rq->nr_running;
if (nr_running == 0) return EEVDF_TIME_SLICE_NS;
+ if (rq->load_weight == 0) {
+ return EEVDF_MIN_GRANULARITY;
+ }
+
uint64_t slice = (EEVDF_TARGET_LATENCY * se->weight) / rq->load_weight;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint64_t EEVDFCalcSlice(EEVDFRunqueue* rq, EEVDFProcessControlBlock* se) { | |
| uint32_t nr_running = rq->nr_running; | |
| if (nr_running == 0) return EEVDF_TIME_SLICE_NS; | |
| uint64_t slice = (EEVDF_TARGET_LATENCY * se->weight) / rq->load_weight; | |
| // Ensure minimum granularity | |
| if (slice < EEVDF_MIN_GRANULARITY) { | |
| slice = EEVDF_MIN_GRANULARITY; | |
| } | |
| // Cap maximum slice | |
| if (slice > EEVDF_MAX_TIME_SLICE_NS) { | |
| slice = EEVDF_MAX_TIME_SLICE_NS; | |
| } | |
| return slice; | |
| } | |
| uint64_t EEVDFCalcSlice(EEVDFRunqueue* rq, EEVDFProcessControlBlock* se) { | |
| uint32_t nr_running = rq->nr_running; | |
| if (nr_running == 0) | |
| return EEVDF_TIME_SLICE_NS; | |
| if (rq->load_weight == 0) { | |
| return EEVDF_MIN_GRANULARITY; | |
| } | |
| uint64_t slice = (EEVDF_TARGET_LATENCY * se->weight) / rq->load_weight; | |
| // Ensure minimum granularity | |
| if (slice < EEVDF_MIN_GRANULARITY) { | |
| slice = EEVDF_MIN_GRANULARITY; | |
| } | |
| // Cap maximum slice | |
| if (slice > EEVDF_MAX_TIME_SLICE_NS) { | |
| slice = EEVDF_MAX_TIME_SLICE_NS; | |
| } | |
| return slice; | |
| } |
🤖 Prompt for AI Agents
In kernel/sched/EEVDF.c around lines 161 to 178, the slice calculation divides
by rq->load_weight without checking for zero; if load_weight == 0 this causes a
divide-by-zero. Fix by guarding: if rq->load_weight == 0 (and nr_running > 0)
set slice to a safe default (e.g., EEVDF_MIN_GRANULARITY) before the division,
then proceed with the existing min/max clamping and return.
| uint64_t EEVDFCalcDelta(uint64_t delta_exec, uint32_t weight, uint32_t lw); | ||
| void EEVDFUpdateCurr(EEVDFRunqueue* rq, EEVDFProcessControlBlock* curr); | ||
| uint64_t EEVDFCalcSlice(EEVDFRunqueue* rq, EEVDFProcessControlBlock* se); |
There was a problem hiding this comment.
Fix EEVDFCalcSlice denominator to include current weight
Because EEVDFDequeueTask() removes the running entity from the runqueue, rq->load_weight no longer contains its weight. EEVDFCalcSlice() then divides only by the remaining load, so with two equal-weight tasks it hands out the full target latency (6 ms) instead of sharing it, effectively doubling slices and delaying peers. Please fold the current task’s weight into the divisor (and keep the zero guard) before dividing to restore fair slicing.
--- a/kernel/sched/EEVDF.c
+++ b/kernel/sched/EEVDF.c
@@
- uint32_t nr_running = rq->nr_running;
- if (nr_running == 0) return EEVDF_TIME_SLICE_NS;
-
- uint64_t slice = (EEVDF_TARGET_LATENCY * se->weight) / rq->load_weight;
+ uint32_t nr_running = rq->nr_running;
+ if (nr_running == 0) {
+ return EEVDF_TIME_SLICE_NS;
+ }
+
+ uint64_t denom = (uint64_t)rq->load_weight + se->weight;
+ if (denom == 0) {
+ denom = EEVDF_NICE_0_LOAD;
+ }
+
+ uint64_t slice = (EEVDF_TARGET_LATENCY * se->weight) / denom;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint64_t EEVDFCalcDelta(uint64_t delta_exec, uint32_t weight, uint32_t lw); | |
| void EEVDFUpdateCurr(EEVDFRunqueue* rq, EEVDFProcessControlBlock* curr); | |
| uint64_t EEVDFCalcSlice(EEVDFRunqueue* rq, EEVDFProcessControlBlock* se); | |
| uint64_t EEVDFCalcSlice(EEVDFRunqueue* rq, EEVDFProcessControlBlock* se) | |
| { | |
| uint32_t nr_running = rq->nr_running; | |
| if (nr_running == 0) { | |
| return EEVDF_TIME_SLICE_NS; | |
| } | |
| /* include the current task’s weight in the divisor and guard zero */ | |
| uint64_t denom = (uint64_t)rq->load_weight + se->weight; | |
| if (denom == 0) { | |
| denom = EEVDF_NICE_0_LOAD; | |
| } | |
| uint64_t slice = (EEVDF_TARGET_LATENCY * se->weight) / denom; | |
| return slice; | |
| } |
🤖 Prompt for AI Agents
In kernel/sched/EEVDF.h around lines 193 to 195, EEVDFCalcSlice currently
divides by rq->load_weight which no longer includes the running entity's weight
(removed by EEVDFDequeueTask); modify the divisor to fold the current task's
weight into the load (e.g., use total_load = rq->load_weight + se->weight or the
correct weight field name on the EEVDFProcessControlBlock) and keep the existing
zero check before dividing so you don't divide by zero; return the slice
computed using that total_load.
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores