RW lock improvements, EXT2 delete #114
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds reentrant per-owner reader/writer locks and applies them across EXT2 and scheduler; refactors RTC to unified read/write and BCD helpers; introduces a VMware SVGA II driver and integrates it into boot; updates build and docs/version strings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Thread
participant V as Ext2Volume
participant FS as EXT2 Ops
T->>V: ReadLock(volume.lock, owner_id)
T->>FS: Ext2ReadInode/ReadFile/ListDir/IsDir
FS-->>T: data / error
T->>V: ReadUnlock(volume.lock, owner_id)
opt write operations
T->>V: WriteLock(volume.lock, owner_id)
T->>FS: Ext2CreateFile/CreateDir/WriteFile/Delete/AddDirEntry
FS-->>T: status / error
T->>V: WriteUnlock(volume.lock, owner_id)
end
sequenceDiagram
autonumber
participant K as Kernel
participant PCI as PCI Bus
participant SVGA as SVGAII Device
participant WM as WindowManager
K->>PCI: Probe for SVGA II (PCI IDs)
PCI-->>K: Device found / BAR info
K->>SVGA: Map MMIO (FB/FIFO), negotiate ID
SVGA-->>K: Init OK / Fail
alt SVGA OK
K->>K: g_svgaII_active = true
else fallback
K->>K: initialize VBE
end
K->>WM: Start if display available
sequenceDiagram
autonumber
participant OS as Kernel
participant RTC as CMOS/RTC
OS->>RTC: Wait for UIP low (Rtc_IsUpdating)
OS->>RTC: Read time registers + century + statusB
RTC-->>OS: raw fields
OS->>OS: Convert BCD/binary, handle 12/24h, compose full year
OS-->>OS: RtcDateTime ready
Note over OS,RTC: RtcSetTime mirrors with updates disabled during writes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (8)
✨ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
kernel/atomic/Spinlock.h (1)
97-108: Race condition in ReadLock between owner check and reader increment.There's a race condition where the write lock owner could change between checking ownership and incrementing the reader count, potentially allowing a non-owner to bypass waiting for the write lock to be released.
The owner check needs to be atomic with the reader state modification:
static inline void ReadLock(rwlock_t* lock, uint32_t owner_id) { - if (lock->writer && lock->owner == owner_id) { - // The current process holds the write lock, so it can "read" - return; - } while (1) { while (lock->writer) __builtin_ia32_pause(); + + // Check ownership after confirming no writer + if (lock->writer && lock->owner == owner_id) { + // The current process holds the write lock, so it can "read" + return; + } + __sync_fetch_and_add(&lock->readers, 1); if (!lock->writer) break; __sync_fetch_and_sub(&lock->readers, 1); } }Actually, this fix is incorrect. A better approach would be to atomically check the writer state:
static inline void ReadLock(rwlock_t* lock, uint32_t owner_id) { if (lock->writer && lock->owner == owner_id) { // The current process holds the write lock, so it can "read" + // Memory barrier to ensure visibility + __atomic_thread_fence(__ATOMIC_ACQUIRE); return; } while (1) { while (lock->writer) __builtin_ia32_pause(); __sync_fetch_and_add(&lock->readers, 1); if (!lock->writer) break; __sync_fetch_and_sub(&lock->readers, 1); } }kernel/sched/MLFQ.c (1)
1945-1959: Same incorrect lock owner issue in MLFQGetProcessStats.Similar to the previous issue, the function uses the searched PID as the lock owner instead of the current process's PID.
void MLFQGetProcessStats(uint32_t pid, uint32_t* cpu_time, uint32_t* io_ops, uint32_t* preemptions) { - ReadLock(&process_table_rwlock, pid); + MLFQProcessControlBlock* current = MLFQGetCurrentProcess(); + ReadLock(&process_table_rwlock, current->pid); MLFQProcessControlBlock* proc = MLFQGetCurrentProcessByPID(pid); if (!proc) { if (cpu_time) *cpu_time = 0; if (io_ops) *io_ops = 0; if (preemptions) *preemptions = 0; + ReadUnlock(&process_table_rwlock, current->pid); return; } if (cpu_time) *cpu_time = (uint32_t)proc->cpu_time_accumulated; if (io_ops) *io_ops = proc->io_operations; if (preemptions) *preemptions = proc->preemption_count; - ReadUnlock(&process_table_rwlock, pid); + ReadUnlock(&process_table_rwlock, current->pid); }Note: There's also a missing unlock in the early return path when
procis NULL.fs/EXT/Ext2.c (2)
76-144: Deadlock: WriteLock acquired before lock initialization.The
Ext2Initfunction acquires aWriteLockonvolume.lockbefore initializing it. This could lead to undefined behavior or deadlock.The lock should be initialized before attempting to acquire it:
int Ext2Init(uint8_t drive) { - WriteLock(&volume.lock, MLFQGetCurrentProcess()->pid); + volume.lock = (rwlock_t){0}; + WriteLock(&volume.lock, MLFQGetCurrentProcess()->pid); if (ext2_initialized) { WriteUnlock(&volume.lock); return 0; } - volume.lock = (rwlock_t){0}; -
565-599: Missing lock release in helper functions.The
Ext2AllocateInodeandExt2AllocateBlockfunctions acquireWriteLockinternally, but they're called from functions that already hold the write lock (e.g.,Ext2CreateFile,Ext2CreateDir). This will cause recursive locking.These helper functions should either:
- Not acquire locks (assuming caller holds the lock) - preferred
- Use recursive write lock capabilities consistently
Since the rwlock supports recursion, this actually works correctly due to the recursion handling in WriteLock. However, this should be documented:
+// Note: Requires caller to hold WriteLock on volume.lock static uint32_t Ext2AllocateInode() { - WriteLock(&volume.lock, MLFQGetCurrentProcess()->pid); + // Caller must hold WriteLock - this will be recursive + WriteLock(&volume.lock, MLFQGetCurrentProcess()->pid);Also applies to: 602-636
🧹 Nitpick comments (4)
fs/EXT/Ext2.c (4)
31-50: Incorrect locking pattern - using ReadLock for internal writes.
Ext2ReadBlockusesReadLockbut performs writes to thevolumestructure's statistics counters internally. While the current code only reads, this creates a maintenance hazard if statistics or state tracking is added later.Consider documenting that this function truly performs only reads, or use WriteLock if any internal state might be modified in the future.
639-733: Same recursive locking pattern in Ext2AddDirEntry.Similar to the allocation functions,
Ext2AddDirEntryacquires a write lock but is called from functions already holding the lock.Document the recursive locking behavior or refactor to avoid it:
+// Note: May be called with WriteLock already held (recursive locking supported) static int Ext2AddDirEntry(uint32_t dir_inode_num, const char* name, uint32_t file_inode_num, uint8_t file_type) { WriteLock(&volume.lock, MLFQGetCurrentProcess()->pid);
1037-1037: Missing TODO: Handle indirect blocks in Ext2Delete.The delete function doesn't handle indirect blocks, which could lead to storage leaks for large files.
The TODO comment indicates indirect block handling is missing. Would you like me to create an issue to track implementing proper indirect block cleanup in the deletion path?
81-81: Consider using designated initializer macro.Following the suggestion for Spinlock.h, consider using a macro for initialization.
- volume.lock = (rwlock_t){0}; + volume.lock = (rwlock_t){.readers = 0, .writer = 0, .owner = 0, .recursion = 0};Or if the RWLOCK_INIT macro is defined:
- volume.lock = (rwlock_t){0}; + volume.lock = RWLOCK_INIT;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
fs/EXT/Ext2.c(31 hunks)kernel/atomic/Spinlock.h(2 hunks)kernel/sched/MLFQ.c(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#101
File: kernel/etc/Shell.c:829-837
Timestamp: 2025-08-31T07:06:37.003Z
Learning: assembler-0 is working on optimizing ramfs performance in VoidFrame kernel and wants to implement proper filesystem characteristics like faster lookups, better memory management, and advanced features like copy-on-write and caching.
🧬 Code graph analysis (2)
fs/EXT/Ext2.c (2)
kernel/atomic/Spinlock.h (4)
ReadLock(97-108)ReadUnlock(110-116)WriteLock(118-131)WriteUnlock(133-138)kernel/sched/MLFQ.c (1)
MLFQGetCurrentProcess(1232-1237)
kernel/sched/MLFQ.c (1)
kernel/atomic/Spinlock.h (2)
ReadLock(97-108)ReadUnlock(110-116)
🔇 Additional comments (1)
fs/EXT/Ext2.c (1)
420-420: Good change: Fixed signedness issue in bytes_written.Changing
bytes_writtenfromuint32_ttointproperly handles error cases where -1 is returned.
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
kernel/atomic/Spinlock.h (1)
112-116: Resolved: release fence added to early ReadUnlock pathThis addresses the missing synchronization previously flagged.
🧹 Nitpick comments (5)
drivers/RTC/Rtc.h (1)
19-21: Align public API with implementation types (RtcDateTime vs rtc_time_t)Implementation uses
RtcDateTimealias; header usesrtc_time_t. They’re compatible iftypedef rtc_time_t RtcDateTime;exists in the C file, but exposing the alias in the header avoids confusion.typedef struct { ... } rtc_time_t; + +// Public alias used by the implementation for clarity/consistency +typedef rtc_time_t RtcDateTime; -void RtcReadTime(rtc_time_t* rtc_time); -void RtcSetTime(const rtc_time_t *dateTime); +void RtcReadTime(rtc_time_t* rtc_time); +void RtcSetTime(const RtcDateTime *dateTime); uint8_t Rtc_BinaryToBcd(uint8_t binary); int Rtc_BcdToBinary(uint8_t bcd);drivers/RTC/Rtc.c (4)
49-51: Busy-wait without bound; add timeout or yield.If UIP stays set due to HW issue, this loop spins forever. Consider a bounded retry with a short pause.
Example:
- while (Rtc_IsUpdating()); + for (int i = 0; i < 100000 && Rtc_IsUpdating(); ++i) { /* pause or cpu_relax(); */ }
22-30: Register access helpers risk unintended NMI state; clarify selection vs write.The helpers don’t control NMI (bit7 of 0x70) and their names encourage misuse (see
Rtc_IsUpdating). Add an explicit selector and use it in reads/writes.+static inline void Rtc_Select(uint8_t reg, bool nmi_mask) { + outb(RTC_CMOS_ADDRESS, (nmi_mask ? 0x80 : 0x00) | reg); +} + static uint8_t Rtc_ReadRegister(uint8_t reg) { - outb(RTC_CMOS_ADDRESS, reg); - return inb(RTC_CMOS_DATA); + Rtc_Select(reg, true); + return inb(RTC_CMOS_DATA); } static void Rtc_WriteRegister(uint8_t reg, uint8_t value) { - outb(RTC_CMOS_ADDRESS, reg); - outb(RTC_CMOS_DATA, value); + Rtc_Select(reg, true); + outb(RTC_CMOS_DATA, value); }
59-60: Century register 0x32 is not universal; guard or fallback.Not all RTCs expose 0x32. Reading/writing it blindly can return garbage or poke undefined registers. Gate with a config or fall back to a heuristic (e.g., base on year < 70).
Sketch:
- century = Rtc_ReadRegister(RTC_CENTURY); // Read century byte +#ifdef RTC_HAVE_CENTURY + century = Rtc_ReadRegister(RTC_CENTURY); +#else + century = 20; // fallback; adjust per platform +#endif ... - Rtc_WriteRegister(RTC_CENTURY, century); +#ifdef RTC_HAVE_CENTURY + Rtc_WriteRegister(RTC_CENTURY, century); +#endifPlease confirm whether this platform defines RTC_HAVE_CENTURY or otherwise provides a reliable century source.
Also applies to: 126-126 --- `3-3`: **Use angle brackets for standard headers.** ```diff -#include "stdbool.h" +#include <stdbool.h>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
drivers/RTC/Rtc.c(1 hunks)drivers/RTC/Rtc.h(2 hunks)fs/VFS.c(1 hunks)kernel/atomic/Spinlock.h(2 hunks)kernel/sched/MLFQ.c(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- kernel/sched/MLFQ.c
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#101
File: kernel/etc/Shell.c:829-837
Timestamp: 2025-08-31T07:06:37.003Z
Learning: assembler-0 is working on optimizing ramfs performance in VoidFrame kernel and wants to implement proper filesystem characteristics like faster lookups, better memory management, and advanced features like copy-on-write and caching.
📚 Learning: 2025-09-01T02:35:28.353Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: fs/VFS.c:331-335
Timestamp: 2025-09-01T02:35:28.353Z
Learning: VfsIsFile function was initially implemented as a placeholder that only checked for mount existence. The proper implementation should follow the same pattern as VfsIsDir: find mount, strip mount path, then use filesystem-specific functions to check if the path points to a file (FS_FILE for RAMFS, Fat12 functions for FAT12).
Applied to files:
fs/VFS.c
🧬 Code graph analysis (3)
drivers/RTC/Rtc.h (1)
drivers/RTC/Rtc.c (3)
RtcSetTime(89-131)Rtc_BinaryToBcd(36-38)Rtc_BcdToBinary(32-34)
fs/VFS.c (1)
drivers/Serial.c (1)
SerialWrite(129-138)
drivers/RTC/Rtc.c (1)
include/Io.h (2)
outb(6-8)inb(10-14)
⏰ 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 (5)
kernel/atomic/Spinlock.h (3)
93-95: Owner/recursion fields: addition looks goodPublic struct extensions are clear and minimal.
120-124: WriteLock recursion relies on correct owner identityReentrancy is fine, but only if
owner_iduniquely identifies a thread. Please confirm PID≠TID in this kernel; if multi-threading exists, switch to TID to avoid cross-thread recursion.
131-133: Owner assignment on first acquisition: LGTMSetting
ownerand initializingrecursionafter exclusivity is acquired is correct.drivers/RTC/Rtc.c (2)
85-87: Year field semantics: ensureRtcDateTime.yearcan hold 4-digit year.You compute
year += century*100but retaincentury. Confirm the struct uses a 16/32-bit year to avoid overflow and that callers expect a 4-digityear.
20-20: RtcDateTime alias is safe
rtc_time_t is defined only once in Rtc.h and aliased in Rtc.c with identical fields—no duplicate definitions or mismatches detected.
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (6)
meson.build (1)
70-71: Include path for vmware is fine; consider feature-gating.drivers/vmware is always in the include path even when SVGA II is disabled. Not harmful, but you can gate this under the same feature flag as the source to keep the build surface minimal.
- src_root + '/drivers/vmware', + #ifdef VF_CONFIG_ENABLE_VMWARE_SVGA_II + src_root + '/drivers/vmware', + #endifkernel/core/Kernel.c (3)
32-33: Header include should be gated by the feature macro.Avoid pulling in driver headers when the feature is off.
-#include "SVGAII.h" +#ifdef VF_CONFIG_ENABLE_VMWARE_SVGA_II +#include "SVGAII.h" +#endif
46-46: Visibility of g_svgaII_active.Global is fine for now, but if only used here, make it static to avoid symbol leaks.
-bool g_svgaII_active = false; +static bool g_svgaII_active = false;
794-799: Guard WindowManager startup when neither VBE nor SVGA II is actually initialized.This check is correct, but VBEShowSplash was called earlier even if VBEInit failed. Consider gating that call too to avoid unintended accesses.
#ifndef VF_CONFIG_EXCLUDE_EXTRA_OBJECTS - VBEShowSplash(); + if (VBEIsInitialized()) VBEShowSplash(); #endifdrivers/vmware/SVGAII.c (2)
2-5: Prefer include paths consistent with project includes.Since meson adds include dirs, use logical includes instead of relative paths.
-#include "../../drivers/PCI/PCI.h" // Assuming PCI.h is in the parent directory -#include "../../include/Io.h" // Assuming Io.h is in include/ -#include "../../mm/VMem.h" // Assuming VMem.h is in mm/ +#include "PCI/PCI.h" +#include "Io.h" +#include "VMem.h"
152-163: Rectangle fill: O(n) per pixel; consider FIFO-accelerated rectangle or memset when 32bpp.Not critical now, but easy win later.
- for (uint32_t j = y; j < y + height; ++j) { - for (uint32_t i = x; i < x + width; ++i) { - SVGAII_PutPixel(i, j, color); - } - } + if (svgaII_device.bpp == 32 && svgaII_device.pitch == svgaII_device.width * 4) { + for (uint32_t j = 0; j < height; ++j) { + uint32_t* row = svgaII_device.framebuffer + (y + j) * (svgaII_device.pitch / 4) + x; + for (uint32_t i = 0; i < width; ++i) row[i] = color; + } + } else { + for (uint32_t j = y; j < y + height; ++j) + for (uint32_t i = x; i < x + width; ++i) + SVGAII_PutPixel(i, j, color); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
drivers/vmware/SVGAII.c(1 hunks)drivers/vmware/SVGAII.h(1 hunks)kernel/core/Kernel.c(6 hunks)meson.build(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#101
File: kernel/etc/Shell.c:829-837
Timestamp: 2025-08-31T07:06:37.003Z
Learning: assembler-0 is working on optimizing ramfs performance in VoidFrame kernel and wants to implement proper filesystem characteristics like faster lookups, better memory management, and advanced features like copy-on-write and caching.
📚 Learning: 2025-08-23T10:06:02.997Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#95
File: kernel/core/Panic.c:65-67
Timestamp: 2025-08-23T10:06:02.997Z
Learning: In VoidFrame kernel's panic handler, the VBE graphics path intentionally mixes VBEShowPanic() with console text operations (PrintKernel*, PrintKernelAt) to overlay stack traces and debugging information on top of the panic image. This overlap is deliberate to ensure critical diagnostic information remains visible during kernel panics, prioritizing debugging capability over visual presentation.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, after VBEShowPanic() draws a panic image to the framebuffer, continuing to use text console operations like PrintKernelAt() causes display corruption because it tries to render text on top of the raw pixel data. The panic handler must choose either pure VBE graphics mode (show image only) or pure text console mode, but not mix both.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T02:19:46.948Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#61
File: include/Font.h:36-46
Timestamp: 2025-08-10T02:19:46.948Z
Learning: In VoidFrame kernel, the PS/2 keyboard controller needs proper initialization including buffer flushing and error handling. Without flushing the controller buffer in KeyboardInit() and handling overflow errors (status bits 6-7) in KeyboardHandler(), the keyboard can stop working if keys are pressed rapidly during boot due to controller buffer overflow.
Applied to files:
kernel/core/Kernel.c
🧬 Code graph analysis (3)
drivers/vmware/SVGAII.h (1)
drivers/vmware/SVGAII.c (5)
SVGAII_DetectAndInitialize(21-93)SVGAII_SetMode(96-115)SVGAII_PutPixel(118-127)SVGAII_UpdateScreen(130-150)SVGAII_FillRect(153-163)
drivers/vmware/SVGAII.c (3)
include/Io.h (2)
outl(26-28)inl(30-34)drivers/PCI/PCI.c (3)
PciFindDevice(141-156)PciConfigReadDWord(14-25)PciConfigWriteDWord(32-48)mm/VMem.c (2)
VMemMapMMIO(587-672)VMemUnmapMMIO(674-738)
kernel/core/Kernel.c (3)
kernel/etc/Console.c (2)
PrintKernelError(220-225)PrintKernelSuccess(213-218)drivers/vmware/SVGAII.c (1)
SVGAII_DetectAndInitialize(21-93)drivers/Vesa.c (1)
VBEIsInitialized(325-327)
🔇 Additional comments (3)
meson.build (1)
158-159: Decide: enable the feature or remove dead wiring.The config define is commented out here but used in Kernel.c. Either uncomment to exercise the path or remove the Kernel-side checks until ready.
Do you want this feature enabled by default for this PR? If yes, uncomment here; if not, we should guard Kernel.c includes/usage too.
kernel/core/Kernel.c (1)
435-438: Nice: clearer VBE init messages.drivers/vmware/SVGAII.c (1)
80-87: Use SVGA_FIFO_ macros for FIFO indices and write SVGA_REG_CONFIG_DONE*
No definitions for SVGA_FIFO_MIN/MAX/NEXT_CMD/STOP or SVGA_REG_CONFIG_DONE were found—please verify or add them in the appropriate header and, after initializing the FIFO with those macros, callSVGAII_WriteReg(SVGA_REG_CONFIG_DONE, 1);
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation