Feat/slab allocation - #83
Conversation
descriptor entries with a union over the next pointers that are not used on free entires.
size as the buddy meta.
📝 WalkthroughWalkthroughThe PR adds the slab crate to the workspace, replaces ChangesSlab allocator redesign
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SlabAllocator
participant SlabCache
participant BuddyArena
participant Page
participant SlabDescriptor
SlabAllocator->>SlabCache: kmalloc selects a typed cache
SlabCache->>SlabDescriptor: allocate an object
SlabAllocator->>BuddyArena: kfree locks buddy storage
BuddyArena->>Page: locate the virtual page
Page->>SlabDescriptor: retrieve the used descriptor
SlabAllocator->>SlabCache: deallocate by object index
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/common/src/address_types.rs (1)
166-176: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe range check and the canonical form now disagree.
newaccepts onlyaddress < (1 << 48).new_uncheckedthen sign-extends bit 47, so an input in[2^47, 2^48)returns a value at or above0xFFFF_8000_0000_0000. A caller that passes an already-canonical higher-half address, such asKERNEL_OFFSET, getsNone. Accept both encodings, or document thatnewtakes only the low 48 bits.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/common/src/address_types.rs` around lines 166 - 176, The range check in new must match the canonicalization performed by new_unchecked: accept valid low-half addresses and already-canonical higher-half addresses such as KERNEL_OFFSET, while rejecting non-canonical values. Update new’s validation accordingly, or explicitly document and enforce that it accepts only low 48-bit inputs; preserve the existing unsafe construction path.
🟡 Minor comments (1)
crates/memory/page/src/lib.rs-20-26 (1)
20-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Debugreads an inactive union field and uses a stale name.
fmtalways readsself.buddy. If the page holds a slab descriptor, the output is meaningless. The struct name string is still"PageMeta", but the type is nowPage. Rename the label. Mark the buddy output as unchecked, or add a discriminant so the formatter can select the active field.🐛 Proposed fix for the stale name
- f.debug_struct("PageMeta") - .field("buddy", unsafe { &self.buddy }) + f.debug_struct("Page") + // SAFETY: assumes the page is in buddy state. + .field("buddy", unsafe { &self.buddy }) .finish()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/memory/page/src/lib.rs` around lines 20 - 26, Update Page’s Debug implementation in fmt to use the current struct label "Page" and avoid unconditionally reading the inactive buddy union field. Either mark the buddy read as unchecked or, preferably, add/use a discriminant so fmt selects and reports only the active buddy or slab descriptor field.
🧹 Nitpick comments (5)
crates/memory/slab/src/cache.rs (1)
12-18: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd
#[repr(C)]toSlabCacheto make the transmutes defensible.
with::<T>()(Line 35) andas_unit()(Line 50) transmute betweenSlabCache<()>andSlabCache<T>. The default Rust representation gives no layout guarantee between separate monomorphizations. Field order may differ in principle.#[repr(C)]fixes the layout and makes the conversion valid.♻️ Proposed change
#[derive(Debug)] +#[repr(C)] pub struct SlabCache<T: Slab> { pub buddy_order: usize, pub free: Option<NonNull<SlabDescriptor<T, Free>>>, pub partial: Option<NonNull<SlabDescriptor<T, Partial>>>, pub full: Option<NonNull<SlabDescriptor<T, Full>>>, }The same reasoning applies to
SlabDescriptor<T, S>, because the state transitions also transmute between instantiations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/memory/slab/src/cache.rs` around lines 12 - 18, Add #[repr(C)] to both SlabCache<T> and SlabDescriptor<T, S> so their field layouts remain consistent across generic instantiations used by with::<T>(), as_unit(), and state-transition transmutes.crates/memory/slab/src/local_macros.rs (2)
51-53: 📐 Maintainability & Code Quality | 🔵 TrivialTracked TODO.
The comment describes a generated reverse-lookup enum for slab positions. Do you want me to open an issue to track this task?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/memory/slab/src/local_macros.rs` around lines 51 - 53, Replace the untracked TODO near the reverse-lookup macro logic with an actionable tracking reference, such as an issue link or identifier, for implementing the generated enum-based reverse lookup and validating its compiler output. Preserve the existing intent and book-documentation note.
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify
COUNTand relax theinitreceiver.Line 26 builds a temporary array of string literals only to read its length. A repetition sum states the intent directly and avoids the element type.
initat Line 38 takes&mut self, but the body only readsself.slab_arena, which is a&'static SpinMutex.&selfis sufficient and lets a caller initialize the allocator through a shared static. The loop also locks and unlocksSLAB_ARENAonce per registered type; a single guard covers the whole loop.♻️ Proposed change
- const COUNT: usize = [$(stringify!($t)),*].len(); + const COUNT: usize = 0 $(+ { let _ = ::core::stringify!($t); 1 })*;- pub fn init(&mut self) { + pub fn init(&self) { + let mut arena = self.slab_arena.lock(); $( let index = <$t>::SLAB_POSITION; let slab_cache = SlabCache::<$t>::new(size_of::<$t>().div_ceil(REGULAR_PAGE_SIZE)); - self.slab_arena.lock()[index] = unsafe { slab_cache.as_unit() }; + arena[index] = unsafe { slab_cache.as_unit() }; )* }Also applies to: 38-46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/memory/slab/src/local_macros.rs` at line 26, Update the macro’s COUNT calculation to use a repetition sum instead of constructing a temporary stringify array. In init, change the receiver from &mut self to &self, then acquire the SLAB_ARENA lock once before iterating over all registered types and reuse that guard throughout the loop.crates/memory/slab/Cargo.toml (1)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the
num_enumgit dependency to a revision.The dependency points at a branch head of a personal fork. Builds are not reproducible, and a force push changes the compiled code. Add
revortag.♻️ Proposed change
-num_enum = { git = "https://github.com/sagi21805/num_enum.git", default-features = false, features = [ +num_enum = { git = "https://github.com/sagi21805/num_enum.git", rev = "<commit-sha>", default-features = false, features = [ "complex-expressions", ] }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/memory/slab/Cargo.toml` around lines 11 - 13, Pin the num_enum git dependency in Cargo.toml to a specific immutable revision or tag by adding the appropriate rev or tag field. Keep the existing repository URL, default-features setting, and complex-expressions feature unchanged.Cargo.toml (1)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrack the disabled
testsworkspace member.The
testsmember is commented out. The workspace no longer builds or runs that crate. Restore it before merge, or record a follow-up task so the tests are re-enabled after the slab refactor lands.Do you want me to open an issue to track re-enabling
tests?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` at line 20, Restore the commented-out "tests" workspace member in the Cargo.toml workspace configuration so the crate is included in builds and test runs; if it cannot be re-enabled during this change, create a follow-up task tracking its restoration after the slab refactor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/common/src/address_types.rs`:
- Around line 163-164: Update VirtualAddress::new_unchecked so the bit-47
sign-extension is performed only when target_pointer_width is 64; preserve the
direct address conversion on 32-bit targets, using the existing target
configuration or fixed-width arithmetic to avoid applying 64-bit
canonicalization to usize.
In `@crates/memory/page/src/lib.rs`:
- Around line 39-45: Update the meta_mut and meta methods to borrow self.buddy
directly instead of converting a shared-reference pointer through
NonNull::from_ref. Return a mutable reference from meta_mut and a shared
reference from meta while preserving their existing signatures and behavior.
In `@crates/memory/slab/src/cache.rs`:
- Around line 129-134: Update the Ok(partial) branch around partial.dealloc in
the slab cache deallocation flow to detect when
partial.state.get_total_allocated() reaches zero. Remove the emptied slab
descriptor from the partial list, reinitialize its state as FullFreeMeta, and
push it onto self.free; preserve the existing deallocation behavior for slabs
that still contain live objects.
- Around line 76-98: Normalize descriptor metadata and list heads across all
SlabCache state transitions. In crates/memory/slab/src/cache.rs lines 76-98,
update the partial-to-full path to write a fresh FullFreeMeta, set next to the
target head, update the source and target heads, clear next when the full list
was empty, and handle the normal SlabStateKind::Partial result instead of
asserting unreachable. In lines 100-114, update the free-list head and
unconditionally set partial.next to self.partial during free-to-partial. In
lines 135-147, update self.full when detaching its head, then rewrite the
descriptor state, set next to the partial head, and push it onto self.partial.
In `@crates/memory/slab/src/descriptor.rs`:
- Around line 223-233: Update the unsafe dealloc method to explicitly drop the
ManuallyDrop<T> object in the freed slot before overwriting it with the
free-list link, ensuring owned resources are released while preserving the
existing allocator bookkeeping.
- Around line 148-163: Validate the object count computed before the free-list
loop in the descriptor initialization path, ensuring it is strictly less than
u16::MAX before converting indices to u16. Add an appropriate compile-time or
runtime assertion, then preserve the existing PreAllocated initialization for
valid counts.
- Around line 189-216: Update Descriptor::alloc to cast the
NonNull<ManuallyDrop<T>> derived from preallocated.allocated to NonNull<T>, and
prevent allocation when the slab has no remaining capacity by asserting
total_allocated is below T::OBJECT_PER_SLAB or changing the API to return None
for a full slab; preserve the existing partial/full state updates for valid
allocations.
- Around line 70-82: Update the PartialMeta bitfield definition to total 64
bits, adding the required padding while preserving the existing next_free_idx,
total_allocated, and partial fields. Ensure PartialMeta has the same layout size
as RawMeta and FullFreeMeta so SlabDescriptor<T, S> metadata transmutations
remain valid.
- Around line 275-285: Update Descriptor::detach to copy only the detached
descriptor’s previous-link value into the next descriptor, preserving its other
state fields. After relinking the neighboring descriptors, clear self.next and
self.state’s previous link so repeated detach or later attach operations cannot
follow stale links.
- Around line 116-120: Update SlabDescriptor::from_non_null so failed conversion
of ptr.addr() to NonZeroU64 is not discarded through .ok(). Propagate the
conversion error or otherwise return it to the caller, and adjust the method’s
return type and callers as needed so overflow cannot be interpreted as None or
cause descriptor unlinking.
- Around line 138-146: Update the allocation logic in the descriptor constructor
around `NonNull::new_unchecked` to validate the computed layout before
allocation and handle layout creation failure, including overflow from large
`order` values. Check the pointer returned by `alloc` before converting it to
`NonNull`, returning an error or panicking on either failure instead of invoking
unchecked operations.
In `@crates/memory/slab/src/lib.rs`:
- Around line 80-84: Update kmalloc and kfree to follow a single lock order:
acquire buddy_arena before slab_arena. Ensure kmalloc holds or obtains
buddy_arena before entering the slab_arena allocation path, matching kfree and
preserving the existing allocation behavior while allowing the future
SlabCache::alloc grow path to use the page allocator safely.
- Around line 101-110: Update the index calculation in the kfree path to compute
the offset in PreAllocated<T> slots rather than T-sized elements. Base the
pointer arithmetic on the descriptor’s PreAllocated<T> object storage,
preserving the existing NonMaxU16 validation and unreachable behavior for
objects outside the page.
In `@crates/memory/slab/src/local_macros.rs`:
- Around line 32-35: Add the missing 'static bound to Arena in the SlabAllocator
impl's where clause, alongside BuddyArena<Block>, so it matches the constraint
declared on SlabAllocator and proves the self type is well-formed.
- Around line 22-34: Update the exported macro define_slab_system! to qualify
all referenced types and constants through $crate, including SlabCache,
SpinMutex, SlabAllocator, BuddyBlock, SlabBlock, and BuddyArena, so expansion
does not depend on caller imports. Replace the injected REGULAR_PAGE_SIZE use
with a fully qualified ::common::constants::REGULAR_PAGE_SIZE reference at both
existing locations, avoiding namespace collisions while preserving the macro’s
behavior.
- Line 42: Update the slab_cache initialization in the local macro to convert
the computed page count into the buddy allocation order expected by
SlabCache::new. Use the smallest order whose power-of-two page capacity
accommodates size_of::<$t>().div_ceil(REGULAR_PAGE_SIZE), preserving correct
allocation for single- and multi-page slab types.
In `@crates/memory/slab/src/preallocated.rs`:
- Around line 10-17: The Debug implementation for PreAllocated<T> must not
unconditionally read both union fields; update it to format raw bytes or accept
the known active state, without directly formatting allocated and next_free_idx
together. In crates/memory/slab/src/preallocated.rs lines 10-17, modify Debug
for PreAllocated. In crates/memory/page/src/lib.rs lines 20-26, retain the buddy
read but document it as an assumption in a comment and rename the debug label
from "PageMeta" to "Page".
In `@crates/memory/slab/src/traits.rs`:
- Line 15: Replace the empty `impl Slab for ()` in the `Slab` trait
implementations with an explicit implementation that provides safe non-zero
constants for unit-sized erased storage, or switch the erased cache marker to a
dedicated non-zero-sized type and update the corresponding cache
storage/transmute path. Ensure `OBJECT_PER_SLAB` is never derived by dividing by
`size_of::<()>()`.
- Around line 19-23: Update the `OBJECT_PER_SLAB` constant to calculate capacity
from the slab’s page count multiplied by `REGULAR_PAGE_SIZE`, divided by
`size_of::<PreAllocated<Self>>()`; do not divide `PAGES_PER_SLAB` by
`size_of::<Self>()`. Preserve `PAGES_PER_SLAB` and ensure the resulting count
matches the `PreAllocated<T>` object-array stride used by slab allocation.
- Around line 56-76: Update the SlabBlock trait’s slab_descriptor and
slab_descriptor_mut methods to be unsafe fn declarations, preserving their
existing signatures and # Safety sections. Correct both doc comments by changing
“Retrive” to “Retrieve” and describing the unchecked cast as occurring
“unconditionally, which is unsafe.”
---
Outside diff comments:
In `@crates/common/src/address_types.rs`:
- Around line 166-176: The range check in new must match the canonicalization
performed by new_unchecked: accept valid low-half addresses and
already-canonical higher-half addresses such as KERNEL_OFFSET, while rejecting
non-canonical values. Update new’s validation accordingly, or explicitly
document and enforce that it accepts only low 48-bit inputs; preserve the
existing unsafe construction path.
---
Minor comments:
In `@crates/memory/page/src/lib.rs`:
- Around line 20-26: Update Page’s Debug implementation in fmt to use the
current struct label "Page" and avoid unconditionally reading the inactive buddy
union field. Either mark the buddy read as unchecked or, preferably, add/use a
discriminant so fmt selects and reports only the active buddy or slab descriptor
field.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 20: Restore the commented-out "tests" workspace member in the Cargo.toml
workspace configuration so the crate is included in builds and test runs; if it
cannot be re-enabled during this change, create a follow-up task tracking its
restoration after the slab refactor.
In `@crates/memory/slab/Cargo.toml`:
- Around line 11-13: Pin the num_enum git dependency in Cargo.toml to a specific
immutable revision or tag by adding the appropriate rev or tag field. Keep the
existing repository URL, default-features setting, and complex-expressions
feature unchanged.
In `@crates/memory/slab/src/cache.rs`:
- Around line 12-18: Add #[repr(C)] to both SlabCache<T> and SlabDescriptor<T,
S> so their field layouts remain consistent across generic instantiations used
by with::<T>(), as_unit(), and state-transition transmutes.
In `@crates/memory/slab/src/local_macros.rs`:
- Around line 51-53: Replace the untracked TODO near the reverse-lookup macro
logic with an actionable tracking reference, such as an issue link or
identifier, for implementing the generated enum-based reverse lookup and
validating its compiler output. Preserve the existing intent and
book-documentation note.
- Line 26: Update the macro’s COUNT calculation to use a repetition sum instead
of constructing a temporary stringify array. In init, change the receiver from
&mut self to &self, then acquire the SLAB_ARENA lock once before iterating over
all registered types and reuse that guard throughout the loop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e6546d52-51c0-4bd6-9ffb-88eaef681f8a
📒 Files selected for processing (15)
Cargo.tomlcrates/common/src/address_types.rscrates/memory/page/Cargo.tomlcrates/memory/page/src/arena.rscrates/memory/page/src/lib.rscrates/memory/page/src/meta.rscrates/memory/slab/Cargo.tomlcrates/memory/slab/src/cache.rscrates/memory/slab/src/descriptor.rscrates/memory/slab/src/lib.rscrates/memory/slab/src/local_macros.rscrates/memory/slab/src/macros.rscrates/memory/slab/src/preallocated.rscrates/memory/slab/src/traits.rscrates/memory/slab/src/unassigned.rs
💤 Files with no reviewable changes (3)
- crates/memory/slab/src/unassigned.rs
- crates/memory/page/src/meta.rs
- crates/memory/slab/src/macros.rs
| Self(((address << 16) as isize >> 16) as usize) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find target specifications and per-crate target settings.
fd -e json -e toml --hidden | rg -n 'target|x86' | head -50
fd '^\.cargo$' -t d --hidden --exec cat {}/config.toml
rg -n 'target_arch\s*=\s*"x86"' --type rust -C2
rg -n 'common' -g 'Cargo.toml' -C2 bootloaderRepository: sagi21805/LearnixOS
Length of output: 439
🏁 Script executed:
#!/bin/bash
set -e
# Locate and inspect the relevant Rust source and Cargo metadata around common/arch targets.
printf 'Tracked Rust/config files:\n'
git ls-files | rg '(^crates/|Cargo\.toml$|\.cargo/config|32bit_target|64bit_target|16bit_target)' || true
printf '\naddress_types.rs outline:\n'
ast-grep outline crates/common/src/address_types.rs || true
printf '\naddress_types.rs relevant lines:\n'
cat -n crates/common/src/address_types.rs | sed -n '1,240p'
printf '\nTarget json files:\n'
cat -n bootloader/second_stage/32bit_target.json
printf '\n---\n'
cat -n kernel/64bit_target.json
printf '\nCommon references in Cargo.toml files:\n'
rg -n 'common|crates/common|crate_name|target =' -g 'Cargo.toml' -C2 .
printf '\nArchitecture target checks in Rust:\n'
rg -n 'target_arch\s*=\s*"x86"|target_arch\s*=\s*"x86_64"|target_pointer_width\s*=\s*"|new_unchecked|canonicalize' --type rust -C2 .Repository: sagi21805/LearnixOS
Length of output: 50375
Gate virtual-address canonicalization to 64-bit pointer width.
VirtualAddress::new_unchecked signs-extends bit 47 after shifting by 16, but usize is 32 bits on the target_arch = "x86" build (p:32:32). That canonicalization no longer applies only to the 64-bit path; apply it only when target_pointer_width = "64" or use fixed-width u64/i64 arithmetic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/common/src/address_types.rs` around lines 163 - 164, Update
VirtualAddress::new_unchecked so the bit-47 sign-extension is performed only
when target_pointer_width is 64; preserve the direct address conversion on
32-bit targets, using the existing target configuration or fixed-width
arithmetic to avoid applying 64-bit canonicalization to usize.
| fn meta_mut(&mut self) -> &mut BuddyMeta<Regular> { | ||
| unsafe { NonNull::from_ref(&self.meta.buddy).as_mut() } | ||
| unsafe { NonNull::from_ref(&self.buddy).as_mut() } | ||
| } | ||
|
|
||
| fn meta(&self) -> &BuddyMeta<Regular> { | ||
| unsafe { NonNull::from_ref(&self.meta.buddy).as_ref() } | ||
| unsafe { NonNull::from_ref(&self.buddy).as_ref() } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not create a mutable reference from a shared-reference pointer.
meta_mut calls NonNull::from_ref(&self.buddy) and then as_mut(). from_ref takes a shared reference, so the resulting pointer carries read-only provenance. Writing through it is undefined behavior under Stacked Borrows. Borrow the field directly instead. meta also does not need the pointer round trip.
🐛 Proposed fix
fn meta_mut(&mut self) -> &mut BuddyMeta<Regular> {
- unsafe { NonNull::from_ref(&self.buddy).as_mut() }
+ unsafe { &mut self.buddy }
}
fn meta(&self) -> &BuddyMeta<Regular> {
- unsafe { NonNull::from_ref(&self.buddy).as_ref() }
+ unsafe { &self.buddy }
}📝 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.
| fn meta_mut(&mut self) -> &mut BuddyMeta<Regular> { | |
| unsafe { NonNull::from_ref(&self.meta.buddy).as_mut() } | |
| unsafe { NonNull::from_ref(&self.buddy).as_mut() } | |
| } | |
| fn meta(&self) -> &BuddyMeta<Regular> { | |
| unsafe { NonNull::from_ref(&self.meta.buddy).as_ref() } | |
| unsafe { NonNull::from_ref(&self.buddy).as_ref() } | |
| } | |
| fn meta_mut(&mut self) -> &mut BuddyMeta<Regular> { | |
| unsafe { &mut self.buddy } | |
| } | |
| fn meta(&self) -> &BuddyMeta<Regular> { | |
| unsafe { &self.buddy } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/memory/page/src/lib.rs` around lines 39 - 45, Update the meta_mut and
meta methods to borrow self.buddy directly instead of converting a
shared-reference pointer through NonNull::from_ref. Return a mutable reference
from meta_mut and a shared reference from meta while preserving their existing
signatures and behavior.
| if let Some(partial) = | ||
| self.partial.map(|mut p| unsafe { p.as_mut() }) | ||
| { | ||
| let (allocation, final_state) = partial.alloc(); | ||
| match final_state { | ||
| SlabStateKind::Full => { | ||
| self.partial = partial.next; | ||
| match self.full { | ||
| Some(mut full) => unsafe { | ||
| full.as_mut() | ||
| .attach(core::mem::transmute(partial)) | ||
| }, | ||
| None => { | ||
| self.full = Some(unsafe { | ||
| core::mem::transmute(partial) | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| _ => debug_assert!(false, "unreachable!"), | ||
| } | ||
| return allocation; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Every state transition in SlabCache leaves the list linkage inconsistent. All three transitions transmute a SlabDescriptor from one state to another, but none of them fully reinitializes the state word or the next field, and none of them updates the cache head that the descriptor is leaving. PartialMeta and FullFreeMeta also have different bit layouts, so a transmute without a state rewrite carries stale bits into the new interpretation. Establish one rule: on every transition, write a fresh state, set next to the target list head, and update both the source head and the target head in SlabCache.
crates/memory/slab/src/cache.rs#L76-L98: on the partial-to-full transition, write a freshFullFreeMeta, clearnextin theNonearm, and replace the wildcarddebug_assert!(false)that fires on the normalSlabStateKind::Partialresult.crates/memory/slab/src/cache.rs#L100-L114: on the free-to-partial transition, assignpartial.next = self.partial;unconditionally so the descriptor stops pointing into the free list.crates/memory/slab/src/cache.rs#L135-L147: on the full-to-partial transition, updateself.fullwhen the detached descriptor is the head, then setnextand push the descriptor ontoself.partial.
📍 Affects 1 file
crates/memory/slab/src/cache.rs#L76-L98(this comment)crates/memory/slab/src/cache.rs#L100-L114crates/memory/slab/src/cache.rs#L135-L147
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/memory/slab/src/cache.rs` around lines 76 - 98, Normalize descriptor
metadata and list heads across all SlabCache state transitions. In
crates/memory/slab/src/cache.rs lines 76-98, update the partial-to-full path to
write a fresh FullFreeMeta, set next to the target head, update the source and
target heads, clear next when the full list was empty, and handle the normal
SlabStateKind::Partial result instead of asserting unreachable. In lines
100-114, update the free-list head and unconditionally set partial.next to
self.partial during free-to-partial. In lines 135-147, update self.full when
detaching its head, then rewrite the descriptor state, set next to the partial
head, and push it onto self.partial.
| match slab.is_partial_mut() { | ||
| // TODO: understand how to extract that logic into a function | ||
| // on the slab. | ||
| Ok(partial) => { | ||
| unsafe { partial.dealloc(idx) }; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
An emptied partial slab is never moved back to the free list.
The Ok(partial) arm frees the index and returns. When total_allocated reaches 0, the slab holds no live objects, but it stays on the partial list. The cache therefore never returns whole slabs to the page allocator, and grow() keeps adding new slabs. Memory use grows without bound under an allocate-free cycle.
Check partial.state.get_total_allocated() after the dealloc call. If it is 0, unlink the descriptor from the partial list, reinitialize its state as FullFreeMeta, and push it onto self.free.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/memory/slab/src/cache.rs` around lines 129 - 134, Update the
Ok(partial) branch around partial.dealloc in the slab cache deallocation flow to
detect when partial.state.get_total_allocated() reaches zero. Remove the emptied
slab descriptor from the partial list, reinitialize its state as FullFreeMeta,
and push it onto self.free; preserve the existing deallocation behavior for
slabs that still contain live objects.
| #[bitfields] | ||
| pub struct PartialMeta { | ||
| pub next_free_idx: B16, | ||
| pub total_allocated: B31, | ||
| pub partial: B1, | ||
| } | ||
|
|
||
| #[bitfields] | ||
| pub struct RawMeta { | ||
| #[flag(r)] | ||
| pub reserved: B63, | ||
| pub partial: B1, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the bitfields macro to determine the generated backing width.
fd -e rs . crates/macros/src --exec rg -n -l 'bitfields' {} \; | head
rg -n 'bitfields' -A40 crates/macros/src | head -120Repository: sagi21805/LearnixOS
Length of output: 7780
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate relevant files"
fd -e rs 'descriptor.rs|cache.rs|utils.rs|bitfield.rs|bitfields.rs|lib.rs' crates | sed -n '1,120p'
echo
echo "descriptor.rs outline/size"
wc -l crates/memory/slab/src/descriptor.rs
sed -n '1,140p' crates/memory/slab/src/descriptor.rs
echo "--- transmutations/usages ---"
rg -n "transmute|PartialMeta|RawMeta|FullFreeMeta|SlabDescriptor" crates/memory/slab/src/descriptor.rs
echo
echo "cache.rs relevant transmutations"
wc -l crates/memory/slab/src/cache.rs
sed -n '1,180p' crates/memory/slab/src/cache.rs
echo
echo "bitfield macro implementation"
wc -l crates/macros/src/bitfields/bitfield.rs
sed -n '1,240p' crates/macros/src/bitfields/bitfield.rs
wc -l crates/macros/src/bitfields/utils.rs
sed -n '1,260p' crates/macros/src/bitfields/utils.rs
wc -l crates/macros/src/bitfields.rs
sed -n '1,220p' crates/macros/src/bitfields.rs
echo
echo "derive expansion probe: compile-free AST inspection if macro source is available"
python3 - <<'PY'
from pathlib import Path
p=Path('crates/macros/src/bitfields.rs')
print(p.read_text())
PYRepository: sagi21805/LearnixOS
Length of output: 32159
Make PartialMeta the same size as the other state metadata.
#[bitfields] derives an exact-width wrapper for each struct. PartialMeta sums to 48 bits, but RawMeta and FullFreeMeta are 64 bits, and SlabDescriptor<T, S> is transmuted between these S::Meta variants. Pad PartialMeta to 64 bits and ensure all metadata states share one layout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/memory/slab/src/descriptor.rs` around lines 70 - 82, Update the
PartialMeta bitfield definition to total 64 bits, adding the required padding
while preserving the existing next_free_idx, total_allocated, and partial
fields. Ensure PartialMeta has the same layout size as RawMeta and FullFreeMeta
so SlabDescriptor<T, S> metadata transmutations remain valid.
| $( | ||
| let index = <$t>::SLAB_POSITION; | ||
|
|
||
| let slab_cache = SlabCache::<$t>::new(size_of::<$t>().div_ceil(REGULAR_PAGE_SIZE)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
buddy_order receives a page count, not an order.
size_of::<$t>().div_ceil(REGULAR_PAGE_SIZE) produces a page count. SlabCache::new stores that value in buddy_order (cache.rs Lines 41-48), and SlabDescriptor::new consumes it as an exponent: REGULAR_PAGE_SIZE * (1 << order) (descriptor.rs Lines 134-178).
A slab type that needs 2 pages therefore reports order 2 and allocates 4 pages. The overshoot grows exponentially with the type size.
Convert the page count to an order.
🐛 Proposed fix
- let slab_cache = SlabCache::<$t>::new(size_of::<$t>().div_ceil(REGULAR_PAGE_SIZE));
+ let pages = ::core::mem::size_of::<$t>()
+ .div_ceil(::common::constants::REGULAR_PAGE_SIZE)
+ .next_power_of_two();
+ let slab_cache =
+ SlabCache::<$t>::new(pages.trailing_zeros() as usize);📝 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.
| let slab_cache = SlabCache::<$t>::new(size_of::<$t>().div_ceil(REGULAR_PAGE_SIZE)); | |
| let pages = ::core::mem::size_of::<$t>() | |
| .div_ceil(::common::constants::REGULAR_PAGE_SIZE) | |
| .next_power_of_two(); | |
| let slab_cache = | |
| SlabCache::<$t>::new(pages.trailing_zeros() as usize); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/memory/slab/src/local_macros.rs` at line 42, Update the slab_cache
initialization in the local macro to convert the computed page count into the
buddy allocation order expected by SlabCache::new. Use the smallest order whose
power-of-two page capacity accommodates
size_of::<$t>().div_ceil(REGULAR_PAGE_SIZE), preserving correct allocation for
single- and multi-page slab types.
| impl<T: Debug> Debug for PreAllocated<T> { | ||
| fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { | ||
| f.debug_struct("PreAllocated") | ||
| .field("allocated", unsafe { &self.allocated }) | ||
| .field("next_free_idx", unsafe { &self.next_free_idx }) | ||
| .finish() | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Both new Debug implementations read union fields without knowing the active variant. A union stores one field at a time. Each fmt here reads a field unconditionally, so the output can interpret unrelated bytes. For a generic T with validity invariants, that read is undefined behavior.
crates/memory/slab/src/preallocated.rs#L10-L17: stop formattingallocatedandnext_free_idxin the same call. Print raw bytes, or take the known state as an argument.crates/memory/page/src/lib.rs#L20-L26: mark thebuddyread as an assumption in a comment, and rename the label from"PageMeta"to"Page".
📍 Affects 2 files
crates/memory/slab/src/preallocated.rs#L10-L17(this comment)crates/memory/page/src/lib.rs#L20-L26
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/memory/slab/src/preallocated.rs` around lines 10 - 17, The Debug
implementation for PreAllocated<T> must not unconditionally read both union
fields; update it to format raw bytes or accept the known active state, without
directly formatting allocated and next_free_idx together. In
crates/memory/slab/src/preallocated.rs lines 10-17, modify Debug for
PreAllocated. In crates/memory/page/src/lib.rs lines 20-26, retain the buddy
read but document it as an assumption in a comment and rename the debug label
from "PageMeta" to "Page".
| pub trait Slab: 'static + Sized + SlabPosition + SlabFlags {} | ||
| pub trait Slab: SlabPosition + SlabFlags {} | ||
|
|
||
| impl Slab for () {} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
impl Slab for () makes the derived constants divide by zero.
size_of::<()>() is 0. OBJECT_PER_SLAB divides by size_of::<Self>(), so the constant fails const evaluation if anything instantiates it for (). crates/memory/slab/src/lib.rs stores [SlabCache<()>; COUNT] and transmutes it to typed caches, so the unit implementation is reachable. Give () an explicit implementation with safe constants, or use a dedicated erased marker type with a non-zero size.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/memory/slab/src/traits.rs` at line 15, Replace the empty `impl Slab
for ()` in the `Slab` trait implementations with an explicit implementation that
provides safe non-zero constants for unit-sized erased storage, or switch the
erased cache marker to a dedicated non-zero-sized type and update the
corresponding cache storage/transmute path. Ensure `OBJECT_PER_SLAB` is never
derived by dividing by `size_of::<()>()`.
| const PAGES_PER_SLAB: usize = size_of::<Self>() | ||
| .next_multiple_of(REGULAR_PAGE_SIZE) | ||
| / REGULAR_PAGE_SIZE; | ||
| const OBJECT_PER_SLAB: usize = | ||
| Self::PAGES_PER_SLAB / size_of::<Self>(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
OBJECT_PER_SLAB computes the wrong value.
PAGES_PER_SLAB is a page count, not a byte count. Dividing a page count by size_of::<Self>() gives 0 for every type larger than one byte. Example: for a 64-byte type, PAGES_PER_SLAB is 1 and OBJECT_PER_SLAB is 0.
crates/memory/slab/src/cache.rs at Line 145 uses this constant as the full-slab allocation count: .total_allocated(T::OBJECT_PER_SLAB as u32). The following partial.dealloc(idx) decrements total_allocated, so the count underflows on the first free of a full slab.
Multiply by the page size first. The slab stride is size_of::<PreAllocated<T>>(), not size_of::<Self>(), because crates/memory/slab/src/descriptor.rs at Line 151 sizes the object array with PreAllocated<T>.
🐛 Proposed fix
- const OBJECT_PER_SLAB: usize =
- Self::PAGES_PER_SLAB / size_of::<Self>();
+ const OBJECT_PER_SLAB: usize = (Self::PAGES_PER_SLAB
+ * REGULAR_PAGE_SIZE)
+ / size_of::<PreAllocated<Self>>();📝 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.
| const PAGES_PER_SLAB: usize = size_of::<Self>() | |
| .next_multiple_of(REGULAR_PAGE_SIZE) | |
| / REGULAR_PAGE_SIZE; | |
| const OBJECT_PER_SLAB: usize = | |
| Self::PAGES_PER_SLAB / size_of::<Self>(); | |
| const PAGES_PER_SLAB: usize = size_of::<Self>() | |
| .next_multiple_of(REGULAR_PAGE_SIZE) | |
| / REGULAR_PAGE_SIZE; | |
| const OBJECT_PER_SLAB: usize = (Self::PAGES_PER_SLAB | |
| * REGULAR_PAGE_SIZE) | |
| / size_of::<PreAllocated<Self>>(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/memory/slab/src/traits.rs` around lines 19 - 23, Update the
`OBJECT_PER_SLAB` constant to calculate capacity from the slab’s page count
multiplied by `REGULAR_PAGE_SIZE`, divided by `size_of::<PreAllocated<Self>>()`;
do not divide `PAGES_PER_SLAB` by `size_of::<Self>()`. Preserve `PAGES_PER_SLAB`
and ensure the resulting count matches the `PreAllocated<T>` object-array stride
used by slab allocation.
| pub trait SlabBlock { | ||
| fn from_address(address: VirtualAddress) -> Self; | ||
|
|
||
| /// Retrive a reference to the [`SlabDescriptor`] from the block. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// The `T` that this function gets will cast the [`SlabDescriptor`] | ||
| /// that in this block unconditionality which in unsafe. | ||
| fn slab_descriptor<T: Slab>(&self) -> &SlabDescriptor<T, Used>; | ||
|
|
||
| /// Retrive a mutable reference [`SlabDescriptor`] from the block. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// The `T` that this function gets will cast the [`SlabDescriptor`] | ||
| /// that in this block unconditionality which in unsafe. | ||
| fn slab_descriptor_mut<T: Slab>( | ||
| &mut self, | ||
| ) -> &mut SlabDescriptor<T, Used>; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Mark the descriptor accessors unsafe, and fix the doc text.
The doc comments state that the T cast is unchecked and unsafe, but both functions are safe. A caller can pick any T and obtain a mismatched SlabDescriptor<T, Used> without an unsafe block. Declare both functions unsafe fn, and keep the # Safety sections.
The doc text also contains typos: "Retrive" → "Retrieve", "unconditionality which in unsafe" → "unconditionally, which is unsafe".
♻️ Proposed change
- /// Retrive a reference to the [`SlabDescriptor`] from the block.
+ /// Retrieve a reference to the [`SlabDescriptor`] from the block.
///
/// # Safety
///
- /// The `T` that this function gets will cast the [`SlabDescriptor`]
- /// that in this block unconditionality which in unsafe.
- fn slab_descriptor<T: Slab>(&self) -> &SlabDescriptor<T, Used>;
+ /// The caller must guarantee that this block holds a slab
+ /// descriptor for `T`. The cast is unconditional.
+ unsafe fn slab_descriptor<T: Slab>(&self) -> &SlabDescriptor<T, Used>;
- /// Retrive a mutable reference [`SlabDescriptor`] from the block.
+ /// Retrieve a mutable reference to the [`SlabDescriptor`] from the
+ /// block.
///
/// # Safety
///
- /// The `T` that this function gets will cast the [`SlabDescriptor`]
- /// that in this block unconditionality which in unsafe.
- fn slab_descriptor_mut<T: Slab>(
+ /// The caller must guarantee that this block holds a slab
+ /// descriptor for `T`. The cast is unconditional.
+ unsafe fn slab_descriptor_mut<T: Slab>(
&mut self,
) -> &mut SlabDescriptor<T, Used>;📝 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.
| pub trait SlabBlock { | |
| fn from_address(address: VirtualAddress) -> Self; | |
| /// Retrive a reference to the [`SlabDescriptor`] from the block. | |
| /// | |
| /// # Safety | |
| /// | |
| /// The `T` that this function gets will cast the [`SlabDescriptor`] | |
| /// that in this block unconditionality which in unsafe. | |
| fn slab_descriptor<T: Slab>(&self) -> &SlabDescriptor<T, Used>; | |
| /// Retrive a mutable reference [`SlabDescriptor`] from the block. | |
| /// | |
| /// # Safety | |
| /// | |
| /// The `T` that this function gets will cast the [`SlabDescriptor`] | |
| /// that in this block unconditionality which in unsafe. | |
| fn slab_descriptor_mut<T: Slab>( | |
| &mut self, | |
| ) -> &mut SlabDescriptor<T, Used>; | |
| } | |
| pub trait SlabBlock { | |
| fn from_address(address: VirtualAddress) -> Self; | |
| /// Retrieve a reference to the [`SlabDescriptor`] from the block. | |
| /// | |
| /// # Safety | |
| /// | |
| /// The caller must guarantee that this block holds a slab | |
| /// descriptor for `T`. The cast is unconditional. | |
| unsafe fn slab_descriptor<T: Slab>(&self) -> &SlabDescriptor<T, Used>; | |
| /// Retrieve a mutable reference to the [`SlabDescriptor`] from the | |
| /// block. | |
| /// | |
| /// # Safety | |
| /// | |
| /// The caller must guarantee that this block holds a slab | |
| /// descriptor for `T`. The cast is unconditional. | |
| unsafe fn slab_descriptor_mut<T: Slab>( | |
| &mut self, | |
| ) -> &mut SlabDescriptor<T, Used>; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/memory/slab/src/traits.rs` around lines 56 - 76, Update the SlabBlock
trait’s slab_descriptor and slab_descriptor_mut methods to be unsafe fn
declarations, preserving their existing signatures and # Safety sections.
Correct both doc comments by changing “Retrive” to “Retrieve” and describing the
unchecked cast as occurring “unconditionally, which is unsafe.”
|
Handle case where freeing an object from partial descriptor results in an empty one. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/memory/slab/src/cache.rs`:
- Around line 50-59: Update find_partial so it checks each current descriptor,
including the tail, against partial and returns the matching current descriptor
rather than next. Preserve None when no descriptor matches.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 857f5ce6-5339-47cd-9744-02d32509aa6c
📒 Files selected for processing (2)
crates/memory/slab/src/cache.rscrates/memory/slab/src/descriptor.rs
| pub fn find_partial(&self, partial: &SlabDescriptor<T, Partial>) -> Option<&SlabDescriptor<T, Partial>> { | ||
| let mut current = self.partial?; | ||
| while let Some(next) = unsafe { current.as_ref().next } { | ||
| if NonNull::from_ref(partial) == current { | ||
| return Some(unsafe { next.as_ref() }); | ||
| } | ||
| current = next; | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return the matched partial descriptor.
Line 53 compares current with partial, but Line 54 returns next. The loop also skips the tail descriptor. A caller can therefore reclassify or unlink a different descriptor.
Proposed fix
pub fn find_partial(&self, partial: &SlabDescriptor<T, Partial>) -> Option<&SlabDescriptor<T, Partial>> {
let mut current = self.partial?;
- while let Some(next) = unsafe { current.as_ref().next } {
+ loop {
if NonNull::from_ref(partial) == current {
- return Some(unsafe { next.as_ref() });
+ return Some(unsafe { current.as_ref() });
}
- current = next;
+ current = unsafe { current.as_ref().next? };
}
- None
}📝 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.
| pub fn find_partial(&self, partial: &SlabDescriptor<T, Partial>) -> Option<&SlabDescriptor<T, Partial>> { | |
| let mut current = self.partial?; | |
| while let Some(next) = unsafe { current.as_ref().next } { | |
| if NonNull::from_ref(partial) == current { | |
| return Some(unsafe { next.as_ref() }); | |
| } | |
| current = next; | |
| } | |
| None | |
| } | |
| pub fn find_partial(&self, partial: &SlabDescriptor<T, Partial>) -> Option<&SlabDescriptor<T, Partial>> { | |
| let mut current = self.partial?; | |
| loop { | |
| if NonNull::from_ref(partial) == current { | |
| return Some(unsafe { current.as_ref() }); | |
| } | |
| current = unsafe { current.as_ref().next? }; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/memory/slab/src/cache.rs` around lines 50 - 59, Update find_partial so
it checks each current descriptor, including the tail, against partial and
returns the matching current descriptor rather than next. Preserve None when no
descriptor matches.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/sync/src/spsc.rs (2)
79-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the safety contracts for these public unsafe methods.
buffer,Producer::inner, andConsumer::innerhave no# Safetysections. Document the aliasing and lifetime requirements for callers. Makeinnersafe if it has no caller-side safety precondition.Also applies to: 113-113, 146-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/sync/src/spsc.rs` at line 79, Document the caller safety contracts for the public unsafe methods buffer, Producer::inner, and Consumer::inner, including their aliasing and lifetime requirements. Review whether each inner method has any caller-side safety precondition; make it safe and remove unnecessary unsafe requirements if not, while preserving unsafe only where required.Source: Linters/SAST tools
113-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn the stored reference directly.
self.bufalready has type&SpscRingBuffer<T>. Returning&self.bufcreates an extra reference and relies on dereference coercion. Returnself.bufdirectly in both methods.Also applies to: 146-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/sync/src/spsc.rs` at line 113, Update both `inner` methods in `SpscRingBuffer` to return the stored `self.buf` reference directly instead of borrowing it again as `&self.buf`, preserving their existing return types and unsafe behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/memory/page/src/lib.rs`:
- Around line 13-17: Update the Page union’s slab field to use
SlabDescriptor<(), Used> wrapped in core::mem::ManuallyDrop, and import or
qualify ManuallyDrop and Used as needed. Preserve the existing
BuddyMeta<Regular> field and ensure the descriptor matches the state required by
slab_descriptor and slab_descriptor_mut.
In `@crates/sync/src/spsc.rs`:
- Around line 82-83: Implement Drop for SpscRingBuffer<T> by reclaiming the
backing allocation created by new instead of calling todo!(); ensure the owned
Box<[T]> is retained or reconstructed and released exactly once when the buffer
is dropped, without panicking.
---
Nitpick comments:
In `@crates/sync/src/spsc.rs`:
- Line 79: Document the caller safety contracts for the public unsafe methods
buffer, Producer::inner, and Consumer::inner, including their aliasing and
lifetime requirements. Review whether each inner method has any caller-side
safety precondition; make it safe and remove unnecessary unsafe requirements if
not, while preserving unsafe only where required.
- Line 113: Update both `inner` methods in `SpscRingBuffer` to return the stored
`self.buf` reference directly instead of borrowing it again as `&self.buf`,
preserving their existing return types and unsafe behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d11db19-6001-4871-810d-8629e8c1917a
📒 Files selected for processing (21)
bootloader/first_stage/src/main.rsbootloader/second_stage/32bit_target.jsonbootloader/second_stage/src/main.rscrates/arch/x86/src/memory_map.rscrates/arch/x86/src/structures/global_descriptor_table.rscrates/common/src/address_types.rscrates/common/src/alloc.rscrates/common/src/enums/keyboard.rscrates/common/src/enums/paging.rscrates/common/src/late_init.rscrates/drivers/vga-display/src/color_code.rscrates/drivers/vga-display/src/screen_char.rscrates/drivers/vga-display/src/writer.rscrates/macros/src/bitfields.rscrates/memory/page/src/lib.rscrates/memory/slab/src/cache.rscrates/sync/src/spsc.rskernel/64bit_target.jsonkernel/src/main.rssnippets/src/book/ch02_02/flag_macro_expand.rssnippets/src/book/flag_macro_expand.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/common/src/address_types.rs
- crates/memory/slab/src/cache.rs
| use slab::descriptor::SlabDescriptor; | ||
|
|
||
| pub union Page { | ||
| pub buddy: BuddyMeta<Regular>, | ||
| pub slab: SlabDescriptor<()>, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 \
'pub struct SlabDescriptor|pub slab:|ManuallyDrop|SlabDescriptor<.*(Used|Free|Partial|Full)' \
crates/memory/page/src/lib.rs \
crates/memory/slab/src/descriptor.rs \
crates/memory/slab/src/cache.rs \
crates/memory/slab/src/traits.rsRepository: sagi21805/LearnixOS
Length of output: 8894
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- page lib ---\n'
cat -n crates/memory/page/src/lib.rs
printf '\n--- descriptor state/impls ---\n'
sed -n '10,90p' crates/memory/slab/src/descriptor.rs | cat -n
sed -n '1,50p' crates/memory/slab/src/traits.rs | cat -n
printf '\n--- slab descriptor field declarations/usages ---\n'
rg -n 'pub slab:|\\.slab|slab_descriptor|SlabDescriptor<' crates/memory -g '*.rs'Repository: sagi21805/LearnixOS
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- page lib ---'
cat -n crates/memory/page/src/lib.rs
echo
echo '--- descriptor state/impls ---'
sed -n '1,90p' crates/memory/slab/src/traits.rs | cat -n
sed -n '10,90p' crates/memory/slab/src/descriptor.rs | cat -n
echo
echo '--- slab descriptor field declarations/usages ---'
rg -n 'pub slab:|\.slab|slab_descriptor|SlabDescriptor<' crates/memory -g '*.rs'Repository: sagi21805/LearnixOS
Length of output: 11941
🌐 Web query:
Rust union field types Copy or ManuallyDrop non-copy restrictions
💡 Result:
In Rust, union field types are restricted to prevent the compiler from implicitly running "drop glue" (the code that executes when a variable is dropped) [1][2]. Because unions cannot track which field is currently active, the compiler cannot safely determine which destructor to run, and thus it forbids fields that might require one [2]. To be valid as a union field, a type must satisfy one of the following conditions: 1. It must implement the Copy trait [1][3]. 2. It must be wrapped in std::mem::ManuallyDrop [1][3]. If a type does not implement Copy, it cannot be used directly as a union field [4][5]. Wrapping the type in ManuallyDrop tells the compiler that you are taking manual responsibility for the memory management of that field, effectively opting out of automatic drop behavior for that specific member [2][6]. Common pitfalls and notes: - Generic types: When using #[derive(Copy)] on a struct with a generic parameter T, the compiler implicitly adds a T: Copy bound [4][5]. If your struct's logic does not strictly require T to be Copy (e.g., it only holds a reference), the derive macro may cause a compilation error when you try to use that struct in a union [4]. In such cases, you may need to implement Copy manually to avoid the excessive bound [4][5]. - ManuallyDrop usage: While ManuallyDrop allows non-Copy types, it does not magically handle destructors for you [2][6]. You become responsible for ensuring that the contained value is properly dropped (if necessary) by using unsafe operations like std::mem::ManuallyDrop::drop [6]. - Union Drop: Although individual fields are restricted, you can still implement the Drop trait for the union itself if you need to perform custom cleanup logic [1][3]. This restriction is enforced by the compiler to ensure memory safety, as unions that implicitly dropped their fields would lead to undefined behavior [2][3].
Citations:
- 1: https://doc.rust-lang.org/stable/reference/items/unions.html
- 2: https://rust-lang.github.io/rfcs/2514-union-initialization-and-drop.html
- 3: https://doc.rust-lang.org/1.77.2/reference/items/unions.html
- 4: https://stackoverflow.com/questions/79858170/field-must-implement-copy-in-union-on-field-with-derived-copy
- 5: https://users.rust-lang.org/t/why-cant-i-use-a-copy-type-in-a-union/92364
- 6: https://doc.rust-lang.org/std/mem/struct.ManuallyDrop.html
Use SlabDescriptor<(), Used> inside ManuallyDrop.
SlabDescriptor<T, S> needs two generic arguments, and this field omits the state. The Used state is required for the slab contract in slab_descriptor[T] and slab_descriptor_mut[T]. Rust unions also forbid non-Copy fields, so wrap the descriptor in core::mem::ManuallyDrop.
🐛 Proposed fix
-use slab::descriptor::SlabDescriptor;
+use slab::descriptor::{SlabDescriptor, Used};
pub union Page {
pub buddy: BuddyMeta<Regular>,
- pub slab: SlabDescriptor<()>,
+ pub slab: ManuallyDrop<SlabDescriptor<(), Used>>,
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/memory/page/src/lib.rs` around lines 13 - 17, Update the Page union’s
slab field to use SlabDescriptor<(), Used> wrapped in core::mem::ManuallyDrop,
and import or qualify ManuallyDrop and Used as needed. Preserve the existing
BuddyMeta<Regular> field and ensure the descriptor matches the state required by
slab_descriptor and slab_descriptor_mut.
Source: MCP tools
| impl<T: Clone + Copy> Drop for SpscRingBuffer<T> { | ||
| fn drop(&mut self) { | ||
| todo!() | ||
| } | ||
| fn drop(&mut self) { todo!() } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Implement Drop before exposing this type.
Drop::drop always calls todo!(). Any non-static SpscRingBuffer created by new panics when it goes out of scope. new also leaks the backing Box<[T]>.
Reclaim the allocation in drop, or store the Box<[T]> directly.
Proposed fix
impl<T: Clone + Copy> Drop for SpscRingBuffer<T> {
- fn drop(&mut self) { todo!() }
+ fn drop(&mut self) {
+ unsafe {
+ drop(Box::from_raw(self.buffer.as_ptr()));
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sync/src/spsc.rs` around lines 82 - 83, Implement Drop for
SpscRingBuffer<T> by reclaiming the backing allocation created by new instead of
calling todo!(); ensure the owned Box<[T]> is retained or reconstructed and
released exactly once when the buffer is dropped, without panicking.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes