On 14.08.24 23:58, Danilo Krummrich wrote: > On Wed, Aug 14, 2024 at 05:01:34PM +0000, Benno Lossin wrote: >> On 12.08.24 20:22, Danilo Krummrich wrote: >>> +/// The kernel's [`Box`] type - a heap allocation for a single value of type `T`. >>> +/// >>> +/// This is the kernel's version of the Rust stdlib's `Box`. There are a couple of differences, >>> +/// for example no `noalias` attribute is emitted and partially moving out of a `Box` is not >>> +/// supported. >> >> I would add "But otherwise it works the same." (I don't know if there is >> a comma needed after the "otherwise"). > > There are more differences we don't list here, and probably don't need to. > Hence, saying that it otherwise works the same isn't correct. > >> Also I remember that there was one more difference with a custom box >> compared to the stdlib, but I forgot what that was, does someone else >> remember? We should also put that here. > > Obviously, there are also quite some API differences. For instance, `Box` > generally requires two generics, value type and allocator, we take page flags > and return a `Result`, where std just panics on failure. Oh yeah that's true. The things listed above don't really refer to API stuff, so I didn't consider that. How about changing "couple differences" to "several differences"? Also adding that the APIs are different would not hurt. >>> +/// >>> +/// `Box` works with any of the kernel's allocators, e.g. [`super::allocator::Kmalloc`], >>> +/// [`super::allocator::Vmalloc`] or [`super::allocator::KVmalloc`]. There are aliases for `Box` >>> +/// with these allocators ([`KBox`], [`VBox`], [`KVBox`]). >>> +/// >>> +/// When dropping a [`Box`], the value is also dropped and the heap memory is automatically freed. >>> +/// >>> +/// # Examples >>> +/// >>> +/// ``` >>> +/// let b = KBox::<u64>::new(24_u64, GFP_KERNEL)?; >>> +/// >>> +/// assert_eq!(*b, 24_u64); >>> +/// >>> +/// # Ok::<(), Error>(()) >>> +/// ``` >>> +/// >>> +/// ``` >>> +/// # use kernel::bindings; >>> +/// >>> +/// const SIZE: usize = bindings::KMALLOC_MAX_SIZE as usize + 1; >>> +/// struct Huge([u8; SIZE]); >>> +/// >>> +/// assert!(KBox::<Huge>::new_uninit(GFP_KERNEL | __GFP_NOWARN).is_err()); >>> +/// ``` >>> +/// >>> +/// ``` >>> +/// # use kernel::bindings; >>> +/// >>> +/// const SIZE: usize = bindings::KMALLOC_MAX_SIZE as usize + 1; >>> +/// struct Huge([u8; SIZE]); >>> +/// >>> +/// assert!(KVBox::<Huge>::new_uninit(GFP_KERNEL).is_ok()); >>> +/// ``` >>> +/// >>> +/// # Invariants >>> +/// >>> +/// The [`Box`]' pointer always properly aligned and either points to memory allocated with `A` or, >> >> "pointer always properly" -> "pointer is properly" >> >>> +/// for zero-sized types, is a dangling pointer. >> >> I think this section would look nicer, if it were formatted using bullet >> points (that way the bracketing of the "or" is also unambiguous). >> >> Additionally, this is missing that the pointer is valid for reads and >> writes. >> >>> +pub struct Box<T: ?Sized, A: Allocator>(NonNull<T>, PhantomData<A>); >> >> Why no `repr(transparent)`? > > I wasn't entirely sure whether that's OK with the additional `PhantomData`, but > I think it is, gonna add it. Yes it is fine, `repr(transparent)` requires that at most one field is non-ZST, but the type can have as many ZST fields as it wants. Otherwise the compiler will complain (there is no `unsafe` here, so just adding it is completely fine). >>> + >>> +/// Type alias for `Box` with a `Kmalloc` allocator. >> >> I think we should add that this is only designed for small values. > > I don't want duplicate the existing documentation around kmalloc and friends > [1]. > > Maybe we can refer to the existing documentation somehow. > > [1] https://www.kernel.org/doc/html/latest/core-api/memory-allocation.html Oh great! With the C docs, I never know where to find them (is it in the code and do they exist?). Yeah let's just link it. >>> +/// >>> +/// # Examples >>> +/// >>> +/// ``` >>> +/// let b = KBox::new(24_u64, GFP_KERNEL)?; >>> +/// >>> +/// assert_eq!(*b, 24_u64); >>> +/// >>> +/// # Ok::<(), Error>(()) >>> +/// ``` >>> +pub type KBox<T> = Box<T, super::allocator::Kmalloc>; >>> + >>> +/// Type alias for `Box` with a `Vmalloc` allocator. >> >> Same here, add that this is supposed to be used for big values (or is >> this also a general-purpose allocator, just not guaranteeing that the >> memory is physically contiguous? in that case I would document it >> here and also on `Vmalloc`). > > Same as above, I'd rather not duplicate that. But I'm happy to link things in, > just not sure what's the best way doing it. I took a look at the link and there is the "Selecting memory allocator" section, but there isn't really just a vmalloc or kmalloc section, it is rather stuff that we would put in the module documentation. What I would write on these types would be what to use these boxes for. eg large allocations, general purpose etc. I don't think that that is easily accessible from the docs that you linked above. --- Cheers, Benno
