Development #120
Development #120
Conversation
WalkthroughAdds a unified scheduler header that conditionally delegates to MLFQ/CFS, introduces a kernel-started flag, changes MLFQ to store per-process names and accept name on creation, updates ELF/Shell callers to pass names, extends IPC with priority/request-response semantics, spinlock-protected queues, larger payloads, and richer metadata, and adds CD-ROM auto-detection. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Caller (ELF / Shell / Subsystem)
participant SchedulerAPI as include/Scheduler.h
participant MLFQ as MLFQ Backend
participant PCB as Process Table
Caller->>SchedulerAPI: CreateProcess(name, entry)
alt VF_CONFIG_SCHED_MLFQ
SchedulerAPI->>MLFQ: MLFQCreateProcess(name, entry)
MLFQ->>PCB: allocate slot, init context, copy name into PCB
MLFQ-->>SchedulerAPI: return pid
else CFS/Default
SchedulerAPI-->>Caller: stub/no-op or void
end
SchedulerAPI-->>Caller: pid / error
sequenceDiagram
autonumber
actor Sender as Sender Process
actor Receiver as Receiver Process
participant IPC as kernel/ipc
participant MQ as Receiver.MessageQueue
Sender->>IPC: IpcSendRequest(target_pid, data, size, &req_id)
IPC->>MQ: spinlock lock
alt MQ has space
IPC->>MQ: append message, assign sequence_id & timestamp, update priority_bitmap
MQ-->>IPC: IPC_SUCCESS
else MQ full and new higher-priority
IPC->>MQ: locate lowest-priority, replace/drop, increment dropped_count
MQ-->>IPC: IPC_SUCCESS (with drop)
else MQ full and lower/equal priority
MQ-->>IPC: IPC_ERROR_QUEUE_FULL
end
IPC->>MQ: spinlock unlock
Note right of Receiver: Receiver later calls IpcReceiveMessage / IpcReceiveMessageType to dequeue highest-priority (oldest tie-breaker)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing Touches
🧪 Generate unit tests
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: 2
🧹 Nitpick comments (5)
kernel/core/Kernel.c (1)
49-49: Expose and document g_HasKernelStarted; consider volatility or barriers.If other modules will read this “public” flag, add an extern declaration in Kernel.h and clarify its intended usage. If read from ISRs or other CPUs later, mark volatile or use memory barriers to ensure visibility.
Proposed header addition (Kernel.h):
+// True once KernelMainHigherHalf has completed core init and enabled IRQs +extern bool g_HasKernelStarted;kernel/elf/ELFloader.c (1)
266-266: Prefer options->process_name (fallback to filename).Keeps semantics if callers supply a friendly name distinct from the path; default opts already mirror filename.
- uint32_t pid = MLFQCreateProcess(filename, (void (*)(void))adjusted_entry); + const char* pname = options && options->process_name ? options->process_name : filename; + uint32_t pid = MLFQCreateProcess(pname, (void (*)(void))adjusted_entry);Also ensure the scheduler takes ownership (copies) of pname to avoid pointer lifetime bugs.
#!/bin/bash rg -nP 'process_name\s*=' -C2 rg -nP '\bMLFQCreateProcess\s*\(' -n -C3include/Scheduler.h (3)
5-5: Include backends conditionally and guard against multiple selections.Prevents accidental coupling to MLFQ when building other schedulers and fails fast if both are enabled.
-#include "MLFQ.h" +#if defined(VF_CONFIG_SCHED_MLFQ) +#include "MLFQ.h" +#endif + +#if defined(VF_CONFIG_SCHED_MLFQ) && defined(VF_CONFIG_SCHED_CFS) +#error "Select exactly one scheduler (MLFQ or CFS), not both." +#endif
61-68: Minor: void wrappers don’t need explicit returns.Harmless, but you can omit redundant returns for readability.
- return MLFQYield(); + MLFQYield();
52-59: Delegate name: consider aligning function name.Wrapper GetProcessByPID maps to MLFQGetCurrentProcessByPID. Consider renaming the MLFQ function to MLFQGetProcessByPID for clarity (non-blocking).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
include/Scheduler.h(1 hunks)kernel/core/Kernel.c(2 hunks)kernel/elf/ELFloader.c(1 hunks)kernel/etc/Shell.c(4 hunks)kernel/sched/MLFQ.c(11 hunks)kernel/sched/MLFQ.h(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#111
File: kernel/sched/MLFQ.c:1514-1514
Timestamp: 2025-09-06T05:31:15.868Z
Learning: In the MLFQ scheduler, removing MLFQYield() from critical system processes like DynamoX and Astra improves system responsiveness, indicating that the yield mechanism may be too aggressive for system-critical processes that need quick response times.
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.
📚 Learning: 2025-09-06T05:31:15.868Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#111
File: kernel/sched/MLFQ.c:1514-1514
Timestamp: 2025-09-06T05:31:15.868Z
Learning: In the MLFQ scheduler, removing MLFQYield() from critical system processes like DynamoX and Astra improves system responsiveness, indicating that the yield mechanism may be too aggressive for system-critical processes that need quick response times.
Applied to files:
kernel/sched/MLFQ.hkernel/sched/MLFQ.c
📚 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:
kernel/sched/MLFQ.hkernel/elf/ELFloader.cinclude/Scheduler.hkernel/sched/MLFQ.c
📚 Learning: 2025-08-12T02:13:46.266Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#72
File: arch/x86_64/syscall/SyscallEntry.asm:24-30
Timestamp: 2025-08-12T02:13:46.266Z
Learning: User assembler-0 has existing ELF infrastructure in VoidFrame kernel with Elf.h header definitions and CreateProcess() function that properly sets up process context including stack allocation, ProcessExitStub return address, and register initialization. The issue was missing ELF loader implementation, not compilation problems.
Applied to files:
kernel/elf/ELFloader.c
📚 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 ProcessRuntimePath bug occurs because: 1) ProcessRuntimePath is defined as char* (not char array), 2) All processes get assigned the same FormatS static buffer pointer, 3) Directory cleanup via VfsDelete happens during TerminateProcess() not final cleanup, 4) When terminating a process, VfsDelete uses whatever path is currently in the shared static buffer, causing wrong directories to be deleted. Fix requires either changing ProcessRuntimePath to char array or using dynamic allocation.
Applied to files:
kernel/elf/ELFloader.ckernel/sched/MLFQ.c
📚 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: In VoidFrame's MLFQ.c, line 1076 assigns ProcessRuntimePath using FormatS() which returns a static buffer, causing all processes to share the same path pointer. During cleanup, VfsDelete() can delete the wrong process directory because all ProcessRuntimePath fields point to the same static buffer containing the path of the most recently created process. This causes directory cleanup operations to affect unintended processes.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-08-10T08:52:13.313Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#63
File: kernel/etc/Shell.c:24-28
Timestamp: 2025-08-10T08:52:13.313Z
Learning: VoidFrame's TerminateProcess function in Process.c only checks if the caller has PROC_PRIV_SYSTEM privilege but doesn't compare caller vs target privilege levels, allowing supervisor processes like the shell to incorrectly kill system-level processes.
Applied to files:
kernel/sched/MLFQ.c
🧬 Code graph analysis (5)
kernel/sched/MLFQ.h (1)
kernel/sched/MLFQ.c (1)
MLFQCreateProcess(1172-1174)
kernel/elf/ELFloader.c (1)
kernel/sched/MLFQ.c (1)
MLFQCreateProcess(1172-1174)
kernel/etc/Shell.c (3)
kernel/sched/MLFQ.c (1)
MLFQCreateProcess(1172-1174)drivers/PCI/PCI.c (1)
PciEnumerate(141-148)drivers/xHCI/xHCI.c (1)
xHCIEnumerate(852-928)
include/Scheduler.h (1)
kernel/sched/MLFQ.c (7)
MLFQSchedInit(1716-1796)MLFQCreateProcess(1172-1174)MLFQGetCurrentProcess(1233-1238)MLFQGetCurrentProcessByPID(1240-1252)MLFQYield(999-1002)MLFQSchedule(769-956)MLFQKillProcess(460-462)
kernel/sched/MLFQ.c (3)
kernel/etc/Shell.c (1)
ShellProcess(1216-1244)kernel/core/Compositor.c (1)
VFCompositor(204-253)kernel/etc/Console.c (1)
PrintKernel(181-199)
⏰ 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 (15)
kernel/core/Kernel.c (1)
813-813: Good placement: set started flag before sti().Setting g_HasKernelStarted before enabling interrupts avoids transient races in early ISRs that might consult startup state. LGTM.
kernel/sched/MLFQ.h (1)
212-213: API change looks consistent; verify all call sites updated.Signature with name parameter aligns with Shell and ELF loader changes. Please confirm any out-of-tree callers are updated.
Run:
#!/bin/bash rg -nP '\bMLFQCreateProcess\s*\(' -C2kernel/etc/Shell.c (1)
147-148: Passing explicit process names: good improvement.String literals here are safe. For non-literals elsewhere, ensure the scheduler copies the name into owned storage (see PCB comment) to avoid lifetime issues.
To confirm names are copied (not just pointer-assigned), run:
#!/bin/bash # Inspect name handling in process creation path rg -nP 'name\s*=' kernel/sched/MLFQ.c -C3 rg -nP '\bCreateSecureProcess\s*\(' -n -C3 rg -nP '\bKernelStringDup|\bstrdup\b|FormatA\s*\(.*name' -nAlso applies to: 334-335, 376-377, 1038-1039
kernel/sched/MLFQ.c (12)
1098-1098: Process naming support looks good!The addition of the
namefield to store human-readable process names is a nice enhancement for observability. The implementation properly initializes the name for both the idle process and system processes.
1019-1019: API signature change implemented correctly.The
MLFQCreateProcessfunction now properly accepts aconst char* nameparameter and forwards it toCreateSecureProcess. All internal calls have been updated accordingly.Also applies to: 1172-1174
1615-1615: System process creation with explicit names.All system processes (Astra, VFShell, DynamoX, VFCompositor) are now created with descriptive string names, improving process management and debugging capabilities.
Also applies to: 1747-1747, 1763-1763, 1778-1778, 1814-1814
1871-1872: Enhanced process listing UI with name column.The process list output now includes a "Name" column with proper spacing adjustments. This makes it much easier to identify processes at a glance.
Also applies to: 1897-1899, 1902-1902
631-647: Improved load balancing logic reduces starvation risk.The changes to use
LOAD_BALANCE_ACTUAL_THRESHOLDand reduce aggressive queue hopping should help prevent process starvation while maintaining fairness.
782-798: More conservative fairness boosting prevents scheduling chaos.Using
FAIRNESS_BOOST_ACTUAL_INTERVALwith much higher thresholds and respecting RT boundaries is a good improvement. This should reduce unnecessary priority adjustments while still preventing starvation.
842-853: Less aggressive preemption improves system stability.The higher bias for critical RT processes and the new priority gap requirement (
PREEMPTION_MIN_PRIORITY_GAP) should reduce unnecessary context switches while maintaining responsiveness for critical tasks.
866-885: Smart priority adjustments based on CPU burst behavior.The logic to demote only when a process uses its full quantum and boost truly interactive processes that yield early is well thought out. This should better differentiate between CPU-bound and I/O-bound processes.
923-941: Refined quantum adjustment with less punishment for CPU-intensive tasks.The quantum adjustments based on I/O operations and CPU burst history are more balanced now, providing better fairness without overly penalizing CPU-intensive processes.
697-699: Anti-starvation bridge promotes to RT tier for guaranteed scheduling.The comment clearly explains why starved processes must be promoted to priority 0 (RT tier) to break out of starvation cycles. This is a critical fairness mechanism.
978-996: Conservative I/O boosting for interactive processes.When a process blocks (likely due to I/O), boosting it to the highest user-space priority (
RT_PRIORITY_THRESHOLD) is appropriate for maintaining interactivity without over-privileging user processes.
729-732: Verify the ProcessRuntimePath validation fix.Good to see the ProcessRuntimePath validation is using a local buffer now instead of the problematic
FormatS()that was identified in previous learnings. This should prevent the shared static buffer issue.Let me verify that the FormatS issue has been properly addressed throughout the codebase:
#!/bin/bash # Check if FormatS is still being used for ProcessRuntimePath anywhere rg -n "ProcessRuntimePath.*FormatS" --type c
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
fs/Iso9660.c (3)
147-153: Fix size-mismatch causing potential buffer overflow in path_copy.You allocate len+1 but copy with size 256; if len+1 < 256 this overflows.
Apply:
- char* path_copy = KernelMemoryAlloc(FastStrlen(path, 256) + 1); + uint32_t path_len = FastStrlen(path, 255); + char* path_copy = KernelMemoryAlloc(path_len + 1); @@ - FastStrCopy(path_copy, path, 256); + FastStrCopy(path_copy, path, path_len + 1);
295-301: Same overflow risk in Iso9660ListDir path handling.Allocate and copy consistently.
Apply:
- char* path_copy = KernelMemoryAlloc(FastStrlen(path, 256) + 1); + uint32_t path_len = FastStrlen(path, 255); + char* path_copy = KernelMemoryAlloc(path_len + 1); @@ - FastStrCopy(path_copy, path, 256); + FastStrCopy(path_copy, path, path_len + 1);
233-259: Avoid per-iteration alloc/free hot path in file read loop.Allocating a 2KB buffer every sector thrashes the heap and hurts IO throughput. Allocate once, reuse, free once.
Apply:
- while (bytes_read < to_read) { + uint8_t* temp_sector = KernelMemoryAlloc(ISO9660_SECTOR_SIZE); + if (!temp_sector) { + if (current_entry != root_entry) KernelFree(current_entry); + KernelFree(pvd); + return -1; + } + while (bytes_read < to_read) { @@ - uint8_t* temp_sector = KernelMemoryAlloc(ISO9660_SECTOR_SIZE); - if (!temp_sector) { - if (current_entry != root_entry) KernelFree(current_entry); - KernelFree(pvd); - return -1; - } - if (ReadSector(sector_to_read, temp_sector) != 0) { - KernelFree(temp_sector); if (current_entry != root_entry) KernelFree(current_entry); KernelFree(pvd); + KernelFree(temp_sector); return -1; } @@ - KernelFree(temp_sector); } + KernelFree(temp_sector);kernel/ipc/Ipc.h (1)
91-97: ToData overflows IPC payload and does not set metadata.Copies past IPC_MAX_PAYLOAD, doesn’t set type/size, and doesn’t terminate strings.
Apply this diff to make it safe and self-describing:
static inline bool ToData(IpcMessage * data, const char * msg) { - const size_t msg_len = StringLength(msg); - for (int i = 0; i < msg_len; i++) { - data->payload.data[i] = msg[i]; - } - return true; + const size_t msg_len = StringLength(msg); + const size_t max_copy = (IPC_MAX_PAYLOAD - 1); + const size_t n = msg_len > max_copy ? max_copy : msg_len; + for (size_t i = 0; i < n; i++) { + data->payload.data[i] = msg[i]; + } + data->payload.data[n] = '\0'; + data->type = IPC_TYPE_DATA; + data->priority = IPC_PRIORITY_NORMAL; + data->size = n + 1; // includes NUL + return true; }
🧹 Nitpick comments (5)
fs/Iso9660.c (2)
13-35: Cache “no drive” and reduce noisy rescans; minor thread-safety.If detection fails, you rescan on every ReadSector. Cache a sentinel and bail fast; mark cdrom_drive volatile. Optional: guard with a spinlock if DetectCdromDrive can be called concurrently.
Apply:
-static uint8_t cdrom_drive = 0xFF; // Auto-detect CD-ROM drive +static volatile uint8_t cdrom_drive = 0xFF; // Auto-detect CD-ROM drive +static const uint8_t DRIVE_NOT_FOUND = 0xFE; @@ - if (cdrom_drive != 0xFF) return cdrom_drive; + if (cdrom_drive == DRIVE_NOT_FOUND) return -1; + if (cdrom_drive != 0xFF) return cdrom_drive; @@ - return -1; + cdrom_drive = DRIVE_NOT_FOUND; + return -1;
80-81: Gate verbose prints behind a debug flag.The added tracing is useful but very chatty in normal operation; consider wrapping with a compile-time DEBUG_ISO9660 or runtime log level.
Also applies to: 109-116, 121-123, 164-165, 187-189, 215-217, 218-220, 411-439
kernel/ipc/Ipc.h (1)
11-29: Naming collision risk: “URGENT” appears in both type and priority.Having IPC_TYPE_URGENT and IPC_PRIORITY_URGENT is easy to confuse at call sites.
Consider renaming IPC_TYPE_URGENT to IPC_TYPE_ALERT or dropping it if priority already conveys urgency.
kernel/ipc/Ipc.c (2)
248-256: Reset dropped_count when flushing.Flush should clear accounting to avoid misleading diagnostics.
Apply:
SpinLock(&queue->lock); queue->head = queue->tail = queue->count = 0; queue->priority_bitmap = 0; + queue->dropped_count = 0; SpinUnlock(&queue->lock);
184-202: Bounds check uses magic ‘8’.Ties to request_id+flags size; safer to reference struct fields for maintainability.
Define IPC_REQUEST_OVERHEAD = sizeof(((IpcMessage*)0)->payload.request) - sizeof(((IpcMessage*)0)->payload.request.request_data) and use it in checks and size assignments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
fs/Iso9660.c(9 hunks)kernel/ipc/Ipc.c(1 hunks)kernel/ipc/Ipc.h(2 hunks)kernel/sched/MLFQ.c(11 hunks)kernel/sched/MLFQ.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- kernel/sched/MLFQ.c
- kernel/sched/MLFQ.h
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#111
File: kernel/sched/MLFQ.c:1514-1514
Timestamp: 2025-09-06T05:31:15.868Z
Learning: In the MLFQ scheduler, removing MLFQYield() from critical system processes like DynamoX and Astra improves system responsiveness, indicating that the yield mechanism may be too aggressive for system-critical processes that need quick response times.
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.
📚 Learning: 2025-09-06T07:51:33.648Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#111
File: kernel/sched/MLFQ.c:999-1002
Timestamp: 2025-09-06T07:51:33.648Z
Learning: In ISO9660 filesystem implementation, the Primary Volume Descriptor (PVD) id field is exactly 5 bytes ['C','D','0','0','1'] with NO null terminator. Using FastStrCmp() or other null-terminated string functions will fail because they expect null terminators. Use exact byte-by-byte comparison instead: candidate->id[0] == 'C' && candidate->id[1] == 'D' etc.
Applied to files:
fs/Iso9660.c
🧬 Code graph analysis (3)
kernel/ipc/Ipc.h (1)
kernel/ipc/Ipc.c (9)
IpcSendMessage(42-100)IpcReceiveMessage(102-146)IpcReceiveMessageType(148-182)IpcSendRequest(184-202)IpcSendResponse(204-221)IpcGetQueueCount(223-226)IpcHasMessages(228-230)IpcHasMessageType(232-246)IpcFlushQueue(248-256)
fs/Iso9660.c (3)
drivers/Ide.c (1)
IdeReadSector(145-189)mm/MemOps.c (1)
FastMemcmp(204-240)kernel/etc/Console.c (2)
PrintKernelF(260-267)PrintKernelError(220-225)
kernel/ipc/Ipc.c (3)
kernel/sched/MLFQ.c (4)
MLFQGetCurrentProcessByPID(1240-1252)MLFQGetSystemTicks(162-165)MLFQGetCurrentProcess(1233-1238)MLFQYield(999-1002)kernel/atomic/Spinlock.h (2)
SpinLock(27-61)SpinUnlock(150-152)mm/MemOps.c (1)
FastMemcpy(75-162)
🔇 Additional comments (3)
fs/Iso9660.c (1)
125-126: Correct PVD signature check (non-NUL-terminated). LGTM.Switching to FastMemcmp(..., "CD001", 5) with type==1 matches spec and avoids false negatives.
Also applies to: 281-283
kernel/ipc/Ipc.c (2)
223-230: Non-atomic queue count read.IpcGetQueueCount() reads count without locking; acceptable for heuristics, but avoid using it for correctness decisions.
If any callers gate blocking/wakeup logic on this value, take the queue lock or document it as best-effort.
17-21: Incorrect highest-priority calculation.Using clz() ^ 31 is wrong; use 31 - clz().
Patch:
- return (IpcPriority)(__builtin_clz(queue->priority_bitmap) ^ 31); + return (IpcPriority)(31 - __builtin_clz(queue->priority_bitmap));Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kernel/ipc/Ipc.h (1)
91-97: Buffer overflow risk in ToData functionThe
ToDatafunction copies a string todata->payload.datawithout bounds checking. Ifmsg_lenexceedsIPC_MAX_PAYLOAD, this will cause a buffer overflow.Apply this fix to add bounds checking:
static inline bool ToData(IpcMessage * data, const char * msg) { const size_t msg_len = StringLength(msg); + if (msg_len > IPC_MAX_PAYLOAD) { + return false; + } for (int i = 0; i < msg_len; i++) { data->payload.data[i] = msg[i]; } + data->size = msg_len; return true; }
♻️ Duplicate comments (1)
kernel/ipc/Ipc.h (1)
77-77: Missing implementations will break linkingThe functions
IpcReceiveMessageTimeoutandIpcBroadcastare declared but commented out, and their implementations are missing in Ipc.c. If other code references these symbols, the build will fail at link time.Let me verify if these functions are referenced elsewhere in the codebase:
#!/bin/bash # Check if IpcReceiveMessageTimeout or IpcBroadcast are referenced anywhere echo "=== Searching for IpcReceiveMessageTimeout references ===" rg -n "IpcReceiveMessageTimeout" --type=c --type=h echo -e "\n=== Searching for IpcBroadcast references ===" rg -n "IpcBroadcast" --type=c --type=hAlso applies to: 83-83
🧹 Nitpick comments (1)
kernel/ipc/Ipc.c (1)
220-223: Potential race condition in IpcGetQueueCountReading
queue->countwithout holding the lock could return stale or torn values if another CPU is modifying it concurrently.Apply this fix to ensure thread-safe access:
uint32_t IpcGetQueueCount(void) { MLFQProcessControlBlock* current = MLFQGetCurrentProcess(); - return current->ipc_queue.count; + MessageQueue* queue = ¤t->ipc_queue; + SpinLock(&queue->lock); + uint32_t count = queue->count; + SpinUnlock(&queue->lock); + return count; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kernel/ipc/Ipc.c(1 hunks)kernel/ipc/Ipc.h(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#111
File: kernel/sched/MLFQ.c:1514-1514
Timestamp: 2025-09-06T05:31:15.868Z
Learning: In the MLFQ scheduler, removing MLFQYield() from critical system processes like DynamoX and Astra improves system responsiveness, indicating that the yield mechanism may be too aggressive for system-critical processes that need quick response times.
🧬 Code graph analysis (2)
kernel/ipc/Ipc.h (1)
kernel/ipc/Ipc.c (9)
IpcSendMessage(42-94)IpcReceiveMessage(96-138)IpcReceiveMessageType(140-179)IpcSendRequest(181-199)IpcSendResponse(201-218)IpcGetQueueCount(220-223)IpcHasMessages(225-227)IpcHasMessageType(229-243)IpcFlushQueue(245-253)
kernel/ipc/Ipc.c (3)
kernel/sched/MLFQ.c (4)
MLFQGetCurrentProcessByPID(1240-1252)MLFQGetSystemTicks(162-165)MLFQGetCurrentProcess(1233-1238)MLFQYield(999-1002)kernel/atomic/Spinlock.h (2)
SpinLock(27-61)SpinUnlock(150-152)mm/MemOps.c (1)
FastMemcpy(75-162)
🔇 Additional comments (5)
kernel/ipc/Ipc.c (5)
42-94: LGTM! Priority-based message dropping is well-implementedThe queue-full handling with priority-based message dropping is correctly implemented. The in-place overwrite preserves ring buffer integrity, and the priority bitmap is properly recomputed after any message replacement.
96-138: LGTM! Proper lock ordering prevents race conditionsThe implementation correctly sets
PROC_BLOCKEDwhile holding the queue lock, preventing the sleep/wakeup race where a sender could enqueue between unlock and blocking.
140-179: LGTM! Type-specific receive with proper state managementThe implementation correctly maintains the priority bitmap after message removal and properly orders the PROC_BLOCKED state change under lock to prevent race conditions.
181-199: LGTM! Request/response pattern well-structuredThe implementation properly validates input parameters, constructs the message with appropriate metadata, and delegates to the core send function.
17-20: No issue with bitwise logic in get_highest_priorityThe expression
__builtin_clz(queue->priority_bitmap) ^ 31computes31 – clzand correctly yields the index of the highest set bit (0–3) as the priority value. No change needed.
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores