Development #39
Development #39
Conversation
WalkthroughThis update modifies process termination handling, increases the PIT frequency constant, comments out virtual memory initialization in the kernel startup, removes debug prints from memory initialization, and significantly refactors virtual memory management for correctness and synchronization, especially in page freeing and address handling. Changes
Sequence Diagram(s)sequenceDiagram
participant CPU
participant Interrupts
participant ProcessManager
participant Scheduler
CPU->>Interrupts: FatalExceptionHandler()
alt current_process != NULL
Interrupts->>ProcessManager: KillProcess(current_process->pid)
ProcessManager-->>Interrupts: (Process termination)
Interrupts->>Scheduler: RequestSchedule()
else current_process == NULL
Interrupts->>Interrupts: Print error
end
sequenceDiagram
participant Kernel
participant VMem
participant Lock
participant PhysMem
Kernel->>VMem: VMemFree(addr, count)
loop for each page
VMem->>Lock: Acquire page table lock
VMem->>PhysMem: Get physical addr from page table
VMem->>VMem: Clear page table entry, flush TLB, update stats
VMem->>Lock: Release lock
VMem->>PhysMem: Free physical page
end
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: 1
🧹 Nitpick comments (1)
kernel/memory/VMem.c (1)
293-344: Well-designed refactoring improves concurrency in memory freeing.The per-page locking approach reduces lock contention and correctly separates the critical section (page table modification) from the non-critical operation (physical page freeing). The order of operations prevents race conditions.
Consider the performance impact for large memory frees due to repeated lock acquisition/release cycles.
For better performance when freeing large memory regions, consider batching operations to reduce lock acquisition overhead:
void VMemFree(void* vaddr, uint64_t size) { if (!vaddr || size == 0) return; size = PAGE_ALIGN_UP(size); uint64_t start_vaddr = PAGE_ALIGN_DOWN((uint64_t)vaddr); uint64_t num_pages = size / PAGE_SIZE; + + // Collect physical addresses to free + uint64_t* paddrs_to_free = NULL; + uint64_t pages_freed = 0; + + // Consider stack allocation for small frees + if (num_pages <= 16) { + uint64_t stack_paddrs[16]; + paddrs_to_free = stack_paddrs; + } else { + // Allocate array for large frees + // Implementation depends on your allocator + } for (uint64_t i = 0; i < num_pages; i++) { uint64_t current_vaddr = start_vaddr + (i * PAGE_SIZE); // ... existing unmapping code ... - if (paddr != 0) { - FreePage((void*)paddr); - } + if (paddr != 0 && paddrs_to_free) { + paddrs_to_free[pages_freed++] = paddr; + } } + + // Free all physical pages outside the critical section + for (uint64_t i = 0; i < pages_freed; i++) { + FreePage((void*)paddrs_to_free[i]); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
arch/x86_64/interrupts/Interrupts.c(1 hunks)drivers/Pic.h(1 hunks)kernel/core/Kernel.c(1 hunks)kernel/memory/Memory.c(0 hunks)kernel/memory/VMem.c(3 hunks)
💤 Files with no reviewable changes (1)
- kernel/memory/Memory.c
🧰 Additional context used
🧬 Code Graph Analysis (2)
arch/x86_64/interrupts/Interrupts.c (2)
kernel/process/Process.c (1)
KillProcess(257-259)kernel/process/Process.h (1)
KillProcess(117-117)
kernel/memory/VMem.c (7)
kernel/memory/MemOps.h (1)
FastZeroPage(9-9)kernel/memory/MemOps.c (1)
FastZeroPage(63-77)kernel/core/Kernel.c (2)
PrintKernelSuccess(147-151)PrintKernelHex(166-189)kernel/memory/VMem.h (3)
VMemIsPageMapped(103-103)VMemGetPhysAddr(102-102)VMemFlushTLBSingle(112-112)kernel/memory/Memory.c (1)
FreePage(130-146)kernel/memory/Memory.h (1)
FreePage(11-11)kernel/core/Panic.h (1)
Panic(7-7)
🔇 Additional comments (4)
drivers/Pic.h (1)
4-4: Verify the performance impact of quadrupling the PIT frequency.Increasing the PIT frequency from 250Hz to 1000Hz will generate timer interrupts every 1ms instead of every 4ms. While this improves scheduling granularity, it also increases interrupt overhead.
Consider documenting the rationale for this change and ensure it doesn't negatively impact system performance or power consumption.
arch/x86_64/interrupts/Interrupts.c (1)
79-80: Good improvement to use centralized process termination.Using
KillProcess()instead of directly modifying process state ensures proper cleanup and consistent termination handling. The addition ofRequestSchedule()appropriately triggers rescheduling after process termination.kernel/memory/VMem.c (2)
72-82: Good improvement to early boot memory initialization.Zeroing the physical page directly during initialization is appropriate when identity mapping is guaranteed by the bootloader. The added success message with the physical address improves debuggability.
119-123: Excellent addition of memory accessibility validation.The check ensures that newly allocated pages are accessible through
PHYS_TO_VIRTbefore attempting to zero them. This prevents potential crashes and provides clear diagnostic information if the assumption fails.
| ProcessInit(); // void function - assume success | ||
| PrintKernelSuccess("[SYSTEM] Process management initialized\n"); | ||
|
|
||
| // VMemInit(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: Virtual memory initialization is disabled.
Commenting out VMemInit() disables the kernel's virtual memory subsystem initialization. This is a fundamental component that should not be disabled without clear justification.
Please explain why this initialization is being skipped and confirm that the system can operate correctly without it.
🏁 Script executed:
#!/bin/bash
set -e
# Locate all uses of VMemInit across the codebase
rg -n 'VMemInit' .
# Show context around SystemInitialize in Kernel.c
sed -n '330,370p' kernel/core/Kernel.c
# Search for any replacement or new VM initialization functions
rg -n 'MemInit' -n kernel/coreLength of output: 2156
🏁 Script executed:
#!/bin/bash
set -e
# Locate the file with MemoryInit definition
file=$(rg -l 'void MemoryInit' -n kernel/memory)
echo "MemoryInit found in: $file"
# Display the first 200 lines to include the function definition
sed -n '1,200p' "$file"Length of output: 60
🏁 Script executed:
#!/bin/bash
set -e
# Search for all occurrences of MemoryInit to find its definition and usage
rg -n 'MemoryInit' .
# Specifically look for its declaration/definition
rg -n 'void MemoryInit' .Length of output: 304
Re-enable Virtual Memory Initialization or Provide Justification
Disabling VMemInit() in kernel/core/Kernel.c:340 leaves the virtual memory subsystem uninitialized, which will prevent page‐table setup and break fundamental kernel operations. No alternative VM init call was found in the codebase.
• File: kernel/core/Kernel.c
Line: 340
Change:
- VMemInit();
+ // VMemInit();Please either restore the VMemInit() call or document and implement a suitable replacement to ensure the kernel’s virtual memory layer is correctly initialized.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In kernel/core/Kernel.c at line 340, the call to VMemInit() has been commented
out, disabling virtual memory initialization and breaking kernel operations
dependent on page-table setup. To fix this, either uncomment and restore the
original VMemInit() call to re-enable the virtual memory subsystem
initialization or provide a documented and implemented alternative
initialization function that properly sets up the kernel's virtual memory layer.
1 participant
Summary by CodeRabbit
Bug Fixes
Chores