From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id CA3C3EDE9AC for ; Tue, 10 Sep 2024 17:40:57 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 401FA8D009B; Tue, 10 Sep 2024 13:40:57 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3B2408D0056; Tue, 10 Sep 2024 13:40:57 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 27A5A8D009B; Tue, 10 Sep 2024 13:40:57 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 0AD298D0056 for ; Tue, 10 Sep 2024 13:40:57 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 7927A1A0665 for ; Tue, 10 Sep 2024 17:40:56 +0000 (UTC) X-FDA: 82549544112.27.3646338 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf22.hostedemail.com (Postfix) with ESMTP id C9F56C0003 for ; Tue, 10 Sep 2024 17:40:54 +0000 (UTC) Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=UJpvi9On; spf=pass (imf22.hostedemail.com: domain of dakr@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=dakr@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1725989918; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=okvcluecyXtrpY0qcJbfs29uPVqGjDNe38CwAoKxM8k=; b=5GIzKypsz2mEgJhpYLUJBs4XjiAClBmb/zQwUHXoqXwIDcQj4ImdLtug5vdfkZT2SEvfq0 VlU2dVdqRTPREof4oMuJ7mWTesZdgOXH+JrGD8iy/UysqCwulSPPS4PYbGyJAh5UUm5TPu vQlJjsIpzMsYfwS09PsuGr/c0kttZvo= ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=UJpvi9On; spf=pass (imf22.hostedemail.com: domain of dakr@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=dakr@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1725989918; a=rsa-sha256; cv=none; b=qOjMsZK9vy36Z6NkXXPyp3qprxBAB7xMeh81IQZRnjTjI3U89eH+MwlzXW8KZn9yWBVy2b PmH8CGNobDpktHe6KFcNs+98cbHFtKxIOE81tfTHjftAzT71QjAnWN6L8SVCAqvXO0QKof mcrzCk/FTRMJWYJ0EtKkvJzGsdX9JDg= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 3FC985C0207; Tue, 10 Sep 2024 17:40:50 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8338BC4CECC; Tue, 10 Sep 2024 17:40:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1725990053; bh=sMaXYdfbLEdc5LCVMNz0SSbctxSq28mn5Ng5vutS7JY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=UJpvi9OnRBgXfwxDXjYXMxDeXhGTfNaQYfGeoQHfQZj5gPQa20Yxoo04D9gQDMSym B1AJavkFZDFmYMZLzEQhOiJMnjAYqvlCQBqjZ91mydn84SJ9OY/YNsPdzamo7YY6cE YCH7HHcDXrcr9AW40s3EPCS421gjZEM2Vbt93BGfnqUz164miq0zv46UI0ESjkqY1f ilPucW/cuVTKFHqB3VYJ4wSTelHOH3Brt+WXJXSsYDEGJd8uLKtZvtVHLxHc6g+dm9 5ZS+bP9UIbOHptxEJ3iuO0Fi7KlpcvkclBXqYlQYerYpZLZnE+7DLgxx4BE0vWSkQm plGjsUXByXDrA== Date: Tue, 10 Sep 2024 19:40:45 +0200 From: Danilo Krummrich To: Benno Lossin Cc: ojeda@kernel.org, alex.gaynor@gmail.com, wedsonaf@gmail.com, boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com, a.hindborg@samsung.com, aliceryhl@google.com, akpm@linux-foundation.org, daniel.almeida@collabora.com, faith.ekstrand@collabora.com, boris.brezillon@collabora.com, lina@asahilina.net, mcanal@igalia.com, zhiw@nvidia.com, cjia@nvidia.com, jhubbard@nvidia.com, airlied@redhat.com, ajanulgu@redhat.com, lyude@redhat.com, linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH v6 09/26] rust: alloc: implement kernel `Box` Message-ID: References: <20240816001216.26575-1-dakr@kernel.org> <20240816001216.26575-10-dakr@kernel.org> <0146cb78-5a35-4d6b-bc75-fed1fc0c270c@proton.me> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0146cb78-5a35-4d6b-bc75-fed1fc0c270c@proton.me> X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: C9F56C0003 X-Stat-Signature: in85dhqh8rqtpgyxempeic61kxps15kj X-Rspam-User: X-HE-Tag: 1725990054-217956 X-HE-Meta: U2FsdGVkX195THbn/arqf/ZMOtlPHIhEHcs63wEjMIYh9yQwaCosoR8uPJ7qiFyhdv2A7E+DWORR4tevwyLRZkKS4wJAAwvgohDhbwxq3GfV+3CrQcE1OU5/OB4ILmNCYe4+tapyj2KJDWONwa91CNJa7bsWJJxWVQPBQSkUm8mYix0qsuUok/Uyg5muyhnIv9bo+GO9YoaAuBFriVHXq9Y+AgzpFU1nMrG5xD7H1EtgqWNB27RJOi2lA+6R6VPY0x0NZxptiTFsL3iRQo5pes9AJPKmHFSga3jkizPItnNh2ASRX0/n7X2VCfG5TEO1ZQGcZPVVfQDFmRealACyHBdBM94fX2Fm9EWqJvt6WvFJOiXXmnpav8vdOC9uR2aBlPui8uLomIJirHPvhAK/W4NhdjX1l/0HIwLbbap05ljZ616QjSaWNXJfZYRz590cbN4FZHGw1JKuNUxsCQqyf4dCrLqjMc+mRcjqAchrcJeGn6q4q2fvOv60UBCZIt41Uh9Vdx+Fxbsye5mi3X3Tx9gxZ0qjqCthoU66fkR9kt+38q/PK5kw/hRYZChGimgS6u2RL0n7n1D6N/IUz5YsGUwfCldo3ARaMuVy5gKWQCgQ6cJL1zobkv10SO9xTD3FLt0ghQrEKOHOdZcnzUNjxcCXTBmk/nW3YBQw22ESLm6aL1ObBxt53IT6Ye64LRs1epV8/453icCbSM98gAuyu1ebCrF+ExdKZDzVhrF6/AloQ6bRhL6m93GWpYj6s1TA5M5Ic7qLLJY9vHbhz1HpXjzo8KBXZ0oPfsgVIdEwM8vtLIbrqCoE0/OW5FRkkGTa9sYk0F9xFOg2V5SmxmzfQS7948aooA7E9wvwBVeasMz+gKjdwkTFCh8RiYQRtyqEB/JyRujtz4SXkR2IykVUvXTWLrODG6eIUhcfuj2PSW32QYjFoNTMWz5BjZDDUtRFdBr06mgWIVRYWGnhJaw DEdmMlou dnKx0gM02BP9Zey3qTXgLcVlaV+jVQ8FERFcCt5d7vb8FdJmFn2FJJZR0cL3NBBB6OhRHtzzbno2PbrL6zBz02EP7hPTpdgzY5JBiOB/xJVSVKctwLTnPFl2/yKrJkCriMb3y42Sb2EA9NcJzYxy4BiOFiOPm3fCXDV68u4amndECSp2jtRfZmSMKpSRNYFqbhY826ljiPFWhrzDGgqhbR26565jo74S8ve3C2SxjjIRkNp8ZUzpWI7vLZ3PsNZxSL3pZfedf73qQD2Jh+xHKfesq1+OHqGAiYTf1xE+izR6atlzZ6VXC1dNyvXfHarFT1otCA/pIIio3RYI= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Sat, Aug 31, 2024 at 05:39:07AM +0000, Benno Lossin wrote: > On 16.08.24 02:10, Danilo Krummrich wrote: > > diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs > > new file mode 100644 > > index 000000000000..93b1ab9de6e8 > > --- /dev/null > > +++ b/rust/kernel/alloc/kbox.rs > > @@ -0,0 +1,480 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +//! Implementation of [`Box`]. > > + > > +#[allow(unused_imports)] // Used in doc comments. > > +use super::allocator::{KVmalloc, Kmalloc, Vmalloc}; > > +use super::{AllocError, Allocator, Flags}; > > +use core::fmt; > > +use core::marker::PhantomData; > > +use core::mem::ManuallyDrop; > > +use core::mem::MaybeUninit; > > +use core::ops::{Deref, DerefMut}; > > +use core::pin::Pin; > > +use core::ptr::NonNull; > > +use core::result::Result; > > + > > +use crate::init::{InPlaceInit, InPlaceWrite, Init, PinInit}; > > +use crate::types::ForeignOwnable; > > + > > +/// The kernel's [`Box`] type - a heap allocation for a single value of type `T`. > > A single `-` doesn't really render nicely in markdown, instead use a > double or triple dash (`--` or `---`). > > > +/// > > +/// This is the kernel's version of the Rust stdlib's `Box`. There are several of differences, > > +/// for example no `noalias` attribute is emitted and partially moving out of a `Box` is not > > +/// supported. There are also several API differences, e.g. `Box` always requires an [`Allocator`] > > +/// implementation to be passed as generic, page [`Flags`] when allocating memory and all functions > > +/// that may allocate memory are failable. > > Do you mean fallible? > > > +/// > > +/// `Box` works with any of the kernel's allocators, e.g. [`Kmalloc`], [`Vmalloc`] or [`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::::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::::new_uninit(GFP_KERNEL | __GFP_NOWARN).is_err()); > > +/// ``` > > It would be nice if you could add something like "KBox can't handle big > allocations:" above this example, so that people aren't confused why > this example expects an error. I don't think that's needed, it's implied by `SIZE == bindings::KMALLOC_MAX_SIZE + 1`. Surely, we could add it nevertheless, but it's not very precise to just say "big allocations". And I think this isn't the place for lengthy explanations of `Kmalloc` behavior. > > > +/// > > +/// ``` > > +/// # use kernel::bindings; > > +/// const SIZE: usize = bindings::KMALLOC_MAX_SIZE as usize + 1; > > +/// struct Huge([u8; SIZE]); > > +/// > > +/// assert!(KVBox::::new_uninit(GFP_KERNEL).is_ok()); > > +/// ``` > > Similarly, you could then say above this one "Instead use either `VBox` > or `KVBox`:" > > > +/// > > +/// # Invariants > > +/// > > +/// The [`Box`]' pointer is always properly aligned and either points to memory allocated with `A` > > Please use `self.0` instead of "[`Box`]'". > > > +/// or, for zero-sized types, is a dangling pointer. > > Probably "dangling, well aligned pointer.". Does this add any value? For ZSTs everything is "well aligned", isn't it? > > > +#[repr(transparent)] > > +pub struct Box(NonNull, PhantomData); > > + > > +/// Type alias for `Box` with a [`Kmalloc`] allocator. > > Can you make these `Box` references links? > > > +/// > > +/// # Examples > > +/// > > +/// ``` > > +/// let b = KBox::new(24_u64, GFP_KERNEL)?; > > +/// > > +/// assert_eq!(*b, 24_u64); > > +/// # Ok::<(), Error>(()) > > +/// ``` > > +pub type KBox = Box; > > + > > +/// Type alias for `Box` with a [`Vmalloc`] allocator. > > +/// > > +/// # Examples > > +/// > > +/// ``` > > +/// let b = VBox::new(24_u64, GFP_KERNEL)?; > > +/// > > +/// assert_eq!(*b, 24_u64); > > +/// # Ok::<(), Error>(()) > > +/// ``` > > +pub type VBox = Box; > > + > > +/// Type alias for `Box` with a [`KVmalloc`] allocator. > > +/// > > +/// # Examples > > +/// > > +/// ``` > > +/// let b = KVBox::new(24_u64, GFP_KERNEL)?; > > +/// > > +/// assert_eq!(*b, 24_u64); > > +/// # Ok::<(), Error>(()) > > +/// ``` > > +pub type KVBox = Box; > > + > > +// SAFETY: `Box` is `Send` if `T` is `Send` because the `Box` owns a `T`. > > +unsafe impl Send for Box > > +where > > + T: Send + ?Sized, > > + A: Allocator, > > +{ > > +} > > + > > +// SAFETY: `Box` is `Sync` if `T` is `Sync` because the `Box` owns a `T`. > > +unsafe impl Sync for Box > > +where > > + T: Sync + ?Sized, > > + A: Allocator, > > +{ > > +} > > + > > +impl Box > > +where > > + T: ?Sized, > > + A: Allocator, > > +{ > > + /// Creates a new `Box` from a raw pointer. > > + /// > > + /// # Safety > > + /// > > + /// For non-ZSTs, `raw` must point at an allocation allocated with `A`that is sufficiently > > + /// aligned for and holds a valid `T`. The caller passes ownership of the allocation to the > > + /// `Box`. > > You don't say what must happen for ZSTs. Because we don't require anything for a ZST, do we? > > > + #[inline] > > + pub const unsafe fn from_raw(raw: *mut T) -> Self { > > + // INVARIANT: Validity of `raw` is guaranteed by the safety preconditions of this function. > > + // SAFETY: By the safety preconditions of this function, `raw` is not a NULL pointer. > > + Self(unsafe { NonNull::new_unchecked(raw) }, PhantomData::) > > + } > > + > > + /// Consumes the `Box` and returns a raw pointer. > > + /// > > + /// This will not run the destructor of `T` and for non-ZSTs the allocation will stay alive > > + /// indefinitely. Use [`Box::from_raw`] to recover the [`Box`], drop the value and free the > > + /// allocation, if any. > > + /// > > + /// # Examples > > + /// > > + /// ``` > > + /// let x = KBox::new(24, GFP_KERNEL)?; > > + /// let ptr = KBox::into_raw(x); > > + /// let x = unsafe { KBox::from_raw(ptr) }; > > + /// > > + /// assert_eq!(*x, 24); > > + /// # Ok::<(), Error>(()) > > + /// ``` > > + #[inline] > > + pub fn into_raw(b: Self) -> *mut T { > > + let b = ManuallyDrop::new(b); > > + > > + b.0.as_ptr() > > + } > > + > > + /// Consumes and leaks the `Box` and returns a mutable reference. > > + /// > > + /// See [Box::into_raw] for more details. > > + #[inline] > > + pub fn leak<'a>(b: Self) -> &'a mut T { > > + // SAFETY: `Box::into_raw` always returns a properly aligned and dereferenceable pointer > > + // which points to an initialized instance of `T`. > > + unsafe { &mut *Box::into_raw(b) } > > + } > > +} > > + > > +impl Box, A> > > +where > > + A: Allocator, > > +{ > > + /// Converts a `Box, A>` to a `Box`. > > + /// > > + /// It is undefined behavior to call this function while the value inside of `b` is not yet > > + /// fully initialized. > > + /// > > + /// # Safety > > + /// > > + /// Callers must ensure that the value inside of `b` is in an initialized state. > > + pub unsafe fn assume_init(b: Self) -> Box { > > + let raw = Self::into_raw(b); > > + > > + // SAFETY: `raw` comes from a previous call to `Box::into_raw`. By the safety requirements > > + // of this function, the value inside the `Box` is in an initialized state. Hence, it is > > + // safe to reconstruct the `Box` as `Box`. > > + unsafe { Box::from_raw(raw as *mut T) } > > You should be able to use `.cast()` instead. > > > + } > > + > > + /// Writes the value and converts to `Box`. > > + pub fn write(mut b: Self, value: T) -> Box { > > + (*b).write(value); > > + // SAFETY: We've just initialized `boxed`'s value. > > The variable is called `b`. > > > + unsafe { Self::assume_init(b) } > > + } > > +} > > [...] > > > +impl Drop for Box > > +where > > + T: ?Sized, > > + A: Allocator, > > +{ > > + fn drop(&mut self) { > > + let size = core::mem::size_of_val::(self); > > + > > + // SAFETY: We need to drop `self.0` in place, before we free the backing memory. > > This is the reason you are calling this function, not the justification > why it is OK to do so. (the pointer is valid) > > > + unsafe { core::ptr::drop_in_place(self.0.as_ptr()) }; > > Instead of using the raw pointer directly, you can also just use > `deref_mut`. > > > + > > + if size != 0 { > > + // SAFETY: `ptr` was previously allocated with `A`. > > There is no variable `ptr`, this is guaranteed by the type invariant of > `Self`. > > --- > Cheers, > Benno > > > + unsafe { A::free(self.0.cast()) }; > > + } > > + } > > +} > > diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs > > index 4571daec0961..a9210634a8c3 100644 > > --- a/rust/kernel/prelude.rs > > +++ b/rust/kernel/prelude.rs > > @@ -14,7 +14,7 @@ > > #[doc(no_inline)] > > pub use core::pin::Pin; > > > > -pub use crate::alloc::{box_ext::BoxExt, flags::*, vec_ext::VecExt}; > > +pub use crate::alloc::{box_ext::BoxExt, flags::*, vec_ext::VecExt, KBox, KVBox, VBox}; > > > > #[doc(no_inline)] > > pub use alloc::{boxed::Box, vec::Vec}; > > -- > > 2.46.0 > > >