Development #23
Development #23
Conversation
WalkthroughThis update replaces the previous round-robin scheduler with a multi-level feedback queue (MLFQ) scheduler in the kernel. It introduces new process scheduling structures, updates process metadata, and adjusts interrupt and process management logic to support the new scheduler. Integer typedefs and macros in the standard header are also revised. Changes
Sequence Diagram(s)sequenceDiagram
participant TimerInterrupt
participant ProcessScheduler
participant ProcessQueue
participant Process
TimerInterrupt->>ProcessScheduler: FastSchedule(regs)
ProcessScheduler->>ProcessQueue: Select highest-priority ready process
ProcessQueue-->>ProcessScheduler: Next process slot
ProcessScheduler->>Process: Save current context (if any)
ProcessScheduler->>Process: Restore next process context
ProcessScheduler->>ProcessQueue: Demote/boost priorities as needed
ProcessScheduler->>TimerInterrupt: Return to interrupt handler
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
Kernel/Process/Process.c (1)
449-454: Critical: GetCurrentProcess uses wrong variableThe function uses the old
current_processvariable instead ofMLFQscheduler.current_running. This will return incorrect process information.Process* GetCurrentProcess(void) { - if (current_process >= MAX_PROCESSES) { + if (MLFQscheduler.current_running >= MAX_PROCESSES) { Panic("GetCurrentProcess: Invalid current process index"); } - return &processes[current_process]; + return &processes[MLFQscheduler.current_running]; }
🧹 Nitpick comments (2)
Kernel/Process/Process.h (1)
51-53: Consider removing legacy scheduling fieldsThe
weightanddynamic_priority_scorefields are marked as legacy from the previous scheduler. Since you've implemented MLFQ, these fields appear to be unused.Consider removing these fields if they're no longer needed:
- uint32_t weight; // Base weight for scheduling (legacy - can remove later) uint64_t cpu_time_accumulated; // Accumulated CPU time - int32_t dynamic_priority_score; // Score for dynamic adjustment (legacy - can remove later)Kernel/Process/Process.c (1)
12-14: Clean up legacy scheduler variablesSeveral variables from the old round-robin scheduler are still present. Consider removing unused ones and documenting why others remain.
The following appear to be legacy:
current_process: Should be replaced withMLFQscheduler.current_runninglast_scheduled_slot: No longer used with MLFQneed_schedule: Still used byRequestSchedule()but could be integrated into MLFQNote:
active_process_bitmapis still used for slot allocation, so it should remain.Also applies to: 21-22
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Kernel/Core/stdint.h(2 hunks)Kernel/Drivers/Interrupts.c(1 hunks)Kernel/Process/Process.c(6 hunks)Kernel/Process/Process.h(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
Kernel/Drivers/Interrupts.c (1)
Learnt from: assembler-0
PR: assembler-0/VoidFrame#2
File: Kernel/Syscall.c:44-44
Timestamp: 2025-07-08T00:36:50.290Z
Learning: In IdtSetGate function calls, the parameters are: (interrupt_vector, handler_address, selector, flags). The interrupt vector (like 0x80) is the first argument, and the selector (like 0x08 for kernel code segment) is the third argument.
Kernel/Process/Process.c (1)
Learnt from: assembler-0
PR: assembler-0/VoidFrame#2
File: Kernel/Syscall.c:44-44
Timestamp: 2025-07-08T00:36:50.290Z
Learning: In IdtSetGate function calls, the parameters are: (interrupt_vector, handler_address, selector, flags). The interrupt vector (like 0x80) is the first argument, and the selector (like 0x08 for kernel code segment) is the third argument.
🧬 Code Graph Analysis (1)
Kernel/Drivers/Interrupts.c (2)
Kernel/Process/Process.h (1)
ScheduleFromInterrupt(93-93)Kernel/Process/Process.c (1)
ScheduleFromInterrupt(431-433)
🔇 Additional comments (4)
Kernel/Drivers/Interrupts.c (1)
73-77: Good: Scheduling before EOI prevents interrupt nestingMoving the scheduler call before the EOI command ensures that scheduling decisions happen atomically without the risk of nested timer interrupts. This is the correct interrupt handling pattern.
Kernel/Process/Process.h (1)
23-23: PROC_TERMINATED = 0 is correctly handled everywhereAll searches for comparisons and assignments involving
PROC_TERMINATED(value 0) show consistent usage:
- Syscall handlers set
current->state = PROC_TERMINATED;- New process slots check
state == PROC_TERMINATEDor(pid == 0 && state == 0)before reuse- Cleanup and scheduler routines filter out
state == PROC_TERMINATEDGetProcessByPidskips terminated processesNo mismatches or hidden assumptions were found. Approving this change as-is.
Kernel/Process/Process.c (2)
166-230: Consider interrupt safety for scheduler operationsThe
FastSchedulefunction modifies global scheduler state without explicit interrupt protection. While it's called from an interrupt handler (where interrupts are disabled), ensure this remains true for all code paths.Document the interrupt safety assumptions or add explicit critical sections if needed for future-proofing.
298-305: Verify PROC_BLOCKED is the intended state for YieldThe
Yieldfunction marks the current process asPROC_BLOCKED, which triggers priority boosting. Typically, yielding keeps a process in thePROC_READYstate.Is this intentional behavior to boost cooperative processes? If not, consider using
PROC_READYinstead.
| typedef unsigned char int8_t; | ||
| _Static_assert(sizeof(int8_t) == 1, "sizeof(int8_t) != 1"); | ||
|
|
||
| typedef unsigned short int16_t; | ||
| _Static_assert(sizeof(int16_t) == 2, "sizeof(int16_t) != 2"); | ||
|
|
||
| typedef unsigned int int32_t; | ||
| _Static_assert(sizeof(int32_t) == 4, "sizeof(int32_t) != 4"); | ||
|
|
||
| typedef unsigned long long int64_t; | ||
| _Static_assert(sizeof(int64_t) == 8, "sizeof(int64_t) != 8"); |
There was a problem hiding this comment.
Critical: Signed integer types are incorrectly defined as unsigned types
All signed integer typedefs are incorrectly using unsigned base types. This will cause serious issues with negative values, arithmetic operations, and comparisons throughout the kernel.
Apply this diff to fix the type definitions:
-typedef unsigned char int8_t;
+typedef signed char int8_t;
_Static_assert(sizeof(int8_t) == 1, "sizeof(int8_t) != 1");
-typedef unsigned short int16_t;
+typedef short int16_t;
_Static_assert(sizeof(int16_t) == 2, "sizeof(int16_t) != 2");
-typedef unsigned int int32_t;
+typedef int int32_t;
_Static_assert(sizeof(int32_t) == 4, "sizeof(int32_t) != 4");
-typedef unsigned long long int64_t;
+typedef long long int64_t;
_Static_assert(sizeof(int64_t) == 8, "sizeof(int64_t) != 8");📝 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.
| typedef unsigned char int8_t; | |
| _Static_assert(sizeof(int8_t) == 1, "sizeof(int8_t) != 1"); | |
| typedef unsigned short int16_t; | |
| _Static_assert(sizeof(int16_t) == 2, "sizeof(int16_t) != 2"); | |
| typedef unsigned int int32_t; | |
| _Static_assert(sizeof(int32_t) == 4, "sizeof(int32_t) != 4"); | |
| typedef unsigned long long int64_t; | |
| _Static_assert(sizeof(int64_t) == 8, "sizeof(int64_t) != 8"); | |
| typedef signed char int8_t; | |
| _Static_assert(sizeof(int8_t) == 1, "sizeof(int8_t) != 1"); | |
| typedef short int16_t; | |
| _Static_assert(sizeof(int16_t) == 2, "sizeof(int16_t) != 2"); | |
| typedef int int32_t; | |
| _Static_assert(sizeof(int32_t) == 4, "sizeof(int32_t) != 4"); | |
| typedef long long int64_t; | |
| _Static_assert(sizeof(int64_t) == 8, "sizeof(int64_t) != 8"); |
🤖 Prompt for AI Agents
In Kernel/Core/stdint.h around lines 24 to 34, the signed integer typedefs
int8_t, int16_t, int32_t, and int64_t are incorrectly defined as unsigned types.
To fix this, change the base types from unsigned char, unsigned short, unsigned
int, and unsigned long long to signed char, signed short, signed int, and signed
long long respectively, ensuring the types correctly represent signed integers.
| void ProcessInit(void) { | ||
| FastMemset(processes, 0, sizeof(Process) * MAX_PROCESSES); | ||
| active_process_bitmap = 0; | ||
| last_scheduled_slot = 0; | ||
|
|
||
| // Create idle process (PID 0) | ||
| // Initialize idle process | ||
| processes[0].pid = 0; | ||
| processes[0].state = PROC_RUNNING; | ||
| processes[0].priority = 255; | ||
| processes[0].privilege_level = PROC_PRIV_SYSTEM; | ||
| processes[0].is_user_mode = 0; | ||
|
|
||
| // Don't set bit 0 in bitmap - idle process is special | ||
| init_token(&processes[0].token, 0, PROC_PRIV_SYSTEM, 0); | ||
|
|
||
| InitScheduler(); // Initialize new scheduler | ||
| process_count = 1; | ||
| current_process = 0; | ||
| } |
There was a problem hiding this comment.
Initialize current_process variable
The current_process variable is not initialized but is still used by GetCurrentProcess(). This could lead to undefined behavior.
void ProcessInit(void) {
FastMemset(processes, 0, sizeof(Process) * MAX_PROCESSES);
// Initialize idle process
processes[0].pid = 0;
processes[0].state = PROC_RUNNING;
processes[0].privilege_level = PROC_PRIV_SYSTEM;
+ current_process = 0; // Start with idle process
InitScheduler(); // Initialize new scheduler
process_count = 1;
}📝 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.
| void ProcessInit(void) { | |
| FastMemset(processes, 0, sizeof(Process) * MAX_PROCESSES); | |
| active_process_bitmap = 0; | |
| last_scheduled_slot = 0; | |
| // Create idle process (PID 0) | |
| // Initialize idle process | |
| processes[0].pid = 0; | |
| processes[0].state = PROC_RUNNING; | |
| processes[0].priority = 255; | |
| processes[0].privilege_level = PROC_PRIV_SYSTEM; | |
| processes[0].is_user_mode = 0; | |
| // Don't set bit 0 in bitmap - idle process is special | |
| init_token(&processes[0].token, 0, PROC_PRIV_SYSTEM, 0); | |
| InitScheduler(); // Initialize new scheduler | |
| process_count = 1; | |
| current_process = 0; | |
| } | |
| void ProcessInit(void) { | |
| FastMemset(processes, 0, sizeof(Process) * MAX_PROCESSES); | |
| // Initialize idle process | |
| processes[0].pid = 0; | |
| processes[0].state = PROC_RUNNING; | |
| processes[0].privilege_level = PROC_PRIV_SYSTEM; | |
| current_process = 0; // Start with idle process | |
| InitScheduler(); // Initialize new scheduler | |
| process_count = 1; | |
| } |
🤖 Prompt for AI Agents
In Kernel/Process/Process.c around lines 329 to 339, the current_process
variable is not initialized in ProcessInit(), which can cause undefined behavior
when accessed by GetCurrentProcess(). Initialize current_process to point to the
idle process (processes[0]) after setting up the idle process and before calling
InitScheduler().
| processes[slot].priority = (privilege == PROC_PRIV_SYSTEM) ? 10 : 100; | ||
| processes[slot].is_user_mode = (privilege != PROC_PRIV_SYSTEM); | ||
| processes[slot].weight = (privilege == PROC_PRIV_SYSTEM) ? 100 : 50; | ||
| processes[slot].cpu_time_accumulated = 0; | ||
| processes[slot].dynamic_priority_score = 0; |
There was a problem hiding this comment.
Fix incorrect priority initialization
The priority values (10 for system, 100 for user) exceed the valid range (0-3 for MLFQ). Also, the legacy scheduling fields are initialized but unused.
processes[slot].stack = stack;
processes[slot].privilege_level = privilege;
- processes[slot].priority = (privilege == PROC_PRIV_SYSTEM) ? 10 : 100;
+ processes[slot].priority = (privilege == PROC_PRIV_SYSTEM) ? 0 : 1;
processes[slot].is_user_mode = (privilege != PROC_PRIV_SYSTEM);
- processes[slot].weight = (privilege == PROC_PRIV_SYSTEM) ? 100 : 50;
- processes[slot].cpu_time_accumulated = 0;
- processes[slot].dynamic_priority_score = 0;📝 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.
| processes[slot].priority = (privilege == PROC_PRIV_SYSTEM) ? 10 : 100; | |
| processes[slot].is_user_mode = (privilege != PROC_PRIV_SYSTEM); | |
| processes[slot].weight = (privilege == PROC_PRIV_SYSTEM) ? 100 : 50; | |
| processes[slot].cpu_time_accumulated = 0; | |
| processes[slot].dynamic_priority_score = 0; | |
| processes[slot].stack = stack; | |
| processes[slot].privilege_level = privilege; | |
| processes[slot].priority = (privilege == PROC_PRIV_SYSTEM) ? 0 : 1; | |
| processes[slot].is_user_mode = (privilege != PROC_PRIV_SYSTEM); |
🤖 Prompt for AI Agents
In Kernel/Process/Process.c around lines 399 to 403, the priority values
assigned (10 for system, 100 for user) exceed the valid MLFQ range of 0 to 3,
and legacy scheduling fields like weight, cpu_time_accumulated, and
dynamic_priority_score are initialized but unused. Fix this by setting priority
to valid values within 0-3, for example 0 for system and 3 for user, and remove
or comment out the initialization of the unused legacy scheduling fields to
clean up the code.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores