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 64CE4C3DA4A for ; Wed, 14 Aug 2024 17:01:45 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E4C846B0098; Wed, 14 Aug 2024 13:01:44 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id DAE966B009C; Wed, 14 Aug 2024 13:01:44 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C01846B009F; Wed, 14 Aug 2024 13:01:44 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 9C9076B0098 for ; Wed, 14 Aug 2024 13:01:44 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 20B064114C for ; Wed, 14 Aug 2024 17:01:44 +0000 (UTC) X-FDA: 82451467728.23.8F9E725 Received: from mail-40133.protonmail.ch (mail-40133.protonmail.ch [185.70.40.133]) by imf21.hostedemail.com (Postfix) with ESMTP id 46B4F1C0039 for ; Wed, 14 Aug 2024 17:01:40 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b=VXY8DMqR; spf=pass (imf21.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.40.133 as permitted sender) smtp.mailfrom=benno.lossin@proton.me; dmarc=pass (policy=quarantine) header.from=proton.me ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1723654829; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=FYIDwo42GSydB8Cho/K5oGRFpWUC2sxe7phTeXQHbYU=; b=sSxddVwpu2+RlcWBmo92qavt53vp7FW0+SbPc2ogjnMZv+ArjgGLh21KCq9HxaddHakF09 Tsd1miFQdEfJpDA93xD3rtHWeTpbY4per2FUXsKIf2G3quyYDUN9bLrI4yyWhKpxHfJP8G 1AC5k54CZk5n6O3QXEU6+JMwq7nj6eg= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1723654829; a=rsa-sha256; cv=none; b=oG1oRdwI2Gqguvk+Mf11+xYRGUJbSZ3qbFlUfVZYyARAeHJXb/jM+hW5qHkDqMHJ5HBbvS l2KJMmHMbBbUu5kXAF2ao4Y+lVOmIKzNaUjhliIgWi62b4AHQczkTNPr8YuuFSVZYi1v0v 9BWkYmquBjk0pvtCQYe6O/SUi4FeFC4= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b=VXY8DMqR; spf=pass (imf21.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.40.133 as permitted sender) smtp.mailfrom=benno.lossin@proton.me; dmarc=pass (policy=quarantine) header.from=proton.me DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=protonmail; t=1723654898; x=1723914098; bh=FYIDwo42GSydB8Cho/K5oGRFpWUC2sxe7phTeXQHbYU=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector; b=VXY8DMqRrvHDq7+UNN7fv19bOU/xOporU5f339bF+O8eHNQgO3nOEL2NOLK2XqfK6 rhBZfmOUp5i7eO7z630axmqjyNSXnR+zPdvtwVTT5Imfu0LElBOim1JWof7lisiN+y hJDw2+mm9LxOZDTo82L1SStNafXZK9P4DJyVcOMoW/ZEtNNZ0HSUD3a6WA1KX3qbBg eyQGIIviaxbHsdzPCeeQXN1zcRryoiuQ4hQN2nFtBROG4hk4MDgLX5tI4L78OUq2p9 veOqlCeEFsTD9apCkaGrZM2X8ZebE5Ze0QL8D9FJALTalvgqNb8xERZwHqL+fbEeJQ 0nLH5Z8WfOGLw== Date: Wed, 14 Aug 2024 17:01:34 +0000 To: Danilo Krummrich , 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 From: Benno Lossin Cc: 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 v5 09/26] rust: alloc: implement kernel `Box` Message-ID: In-Reply-To: <20240812182355.11641-10-dakr@kernel.org> References: <20240812182355.11641-1-dakr@kernel.org> <20240812182355.11641-10-dakr@kernel.org> Feedback-ID: 71780778:user:proton X-Pm-Message-ID: 7b6c5c29aefcbab38be1671307f4ade33ed6d891 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 46B4F1C0039 X-Stat-Signature: cu4cs478y7kd6peb6sqhc1ynm5y6o5y3 X-HE-Tag: 1723654900-912064 X-HE-Meta: U2FsdGVkX191XkS3CL+HZhSFBillvJ/SF6a8e4X/3d6T1CGszeo8DIaUtjgk11gvJTFoAkj5m0K1y73OtriTUCUlADA+GetKaHtKkV06EvFwKMMUpAX/uy2K98qACOnaOQyW6nwcNwjrSZbY1bMqUrVoD7e6obTpxx2xLcc3Xog/4qhJia1WdKJPcQB9shzYY6i+Ol3ffc4wpiI/ZsaS0vJylvT6L4bQIlgWfmMiES6JRvP0gQV//0ulEnVnu6SwHSerS5gwAEdGpop8mkTvVk0OuTpHLX6SRbccpCWwpZjCmWbuXXL52Iiz2GGE92vWS+OxZkNX9+9DvazVoRpt8vapv57FLllUdzfKiyeb/yQzFppL06HTdHzbzOBz9amm8RHzTt+CjSxrhOqSh4tzAuL8f4wK+fQp6KX218hZoSnmb7ApPB/UtfRFnwLHheSXS2Rr0H5LHFfLUrXpTaYhv4aZ7o5h/MJZ0BAOCtdd8JXiNd95ujMllOuLejX52wNEA89nrsLkKlnn+taJG+SPWVOwc+4nBrAShuO3x3aO77janzHqft8msYl6HyWd038if/l6Ro9fSP9qj4Z1n6r7/hF/A0m0gFDrCIy85SzNOHUqT5TCNu8BfP6EAs9FJEOXImRxpANJAqe2mDqb7/WnA+ZIWrTuat2STZLZOwGQ/xUeZkNWzKjc4j55lc5zYIHWW2Vo2xCv9JWfwflV6TLymUu+vQm+agOZzLmrcjYw+/AkiXf+fDYaXWWHruTV8RMFGmwB2HWz/vtr4JkmMD95RlwNGXFMU6JgoHDCknDK3ID8aunQHPZKSPHAd4WEW5ygLEwi3xGnGGZaZ9nT3To/ZU6tp1cwrJ6enO8SacJQc1pOolUMOPxSQcMH/QIgK9bmH2t0QvFJ122zDVIjp7wQLRdefiQSnpYU5Bw0Lg9BHeOrlVXVhplU2MTgRCUsy/0qw4d0bXxKTWn39k6YRcp W21b4k6k K/3+/HWajSGL37mIP7mRC1mVq0S8wOkjXohWR4DCeQoXnU6RJbXGfapBoPjl9l8vun0ltUVRg7ftSdvaedRzk/Za+3NKcUDQNDcQ6qiXjBr5lQk2/MEwZSRpwdUxitPQpbfMcgadPF4TUsdLlBuNrli7Jur//1fy+DHEHqR6ZyiZznv4BWJ+4R/8wCrX0J3c5T8jaxRHXZJRrZesdzvjdA+Z7wumbqym8Ekg+KYv7sumSOa9l1tWnNMU5f4uaqOgHfwAZsAmq2r5z5fsK0It08B+ovXliWRf2jesGu/YBPqhEkWZCy6FhnagE/NTwjySocD797xYVYijNcCdhOcXnof1cGalbKyFtzjzAtAcYZZ/csCo= 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 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 o= ut 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"). 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. > +/// > +/// `Box` works with any of the kernel's allocators, e.g. [`super::alloc= ator::Kmalloc`], > +/// [`super::allocator::Vmalloc`] or [`super::allocator::KVmalloc`]. The= re are aliases for `Box` > +/// with these allocators ([`KBox`], [`VBox`], [`KVBox`]). > +/// > +/// When dropping a [`Box`], the value is also dropped and the heap memo= ry is automatically freed. > +/// > +/// # Examples > +/// > +/// ``` > +/// let b =3D KBox::::new(24_u64, GFP_KERNEL)?; > +/// > +/// assert_eq!(*b, 24_u64); > +/// > +/// # Ok::<(), Error>(()) > +/// ``` > +/// > +/// ``` > +/// # use kernel::bindings; > +/// > +/// const SIZE: usize =3D bindings::KMALLOC_MAX_SIZE as usize + 1; > +/// struct Huge([u8; SIZE]); > +/// > +/// assert!(KBox::::new_uninit(GFP_KERNEL | __GFP_NOWARN).is_err()= ); > +/// ``` > +/// > +/// ``` > +/// # use kernel::bindings; > +/// > +/// const SIZE: usize =3D bindings::KMALLOC_MAX_SIZE as usize + 1; > +/// struct Huge([u8; SIZE]); > +/// > +/// assert!(KVBox::::new_uninit(GFP_KERNEL).is_ok()); > +/// ``` > +/// > +/// # Invariants > +/// > +/// The [`Box`]' pointer always properly aligned and either points to me= mory 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(NonNull, PhantomData); Why no `repr(transparent)`? > + > +/// Type alias for `Box` with a `Kmalloc` allocator. I think we should add that this is only designed for small values. > +/// > +/// # Examples > +/// > +/// ``` > +/// let b =3D KBox::new(24_u64, GFP_KERNEL)?; > +/// > +/// assert_eq!(*b, 24_u64); > +/// > +/// # Ok::<(), Error>(()) > +/// ``` > +pub type KBox =3D Box; > + > +/// 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`). > +/// > +/// # Examples > +/// > +/// ``` > +/// let b =3D VBox::new(24_u64, GFP_KERNEL)?; > +/// > +/// assert_eq!(*b, 24_u64); > +/// > +/// # Ok::<(), Error>(()) > +/// ``` > +pub type VBox =3D Box; > + > +/// Type alias for `Box` with a `KVmalloc` allocator. Ditto. > +/// > +/// # Examples > +/// > +/// ``` > +/// let b =3D KVBox::new(24_u64, GFP_KERNEL)?; > +/// > +/// assert_eq!(*b, 24_u64); > +/// > +/// # Ok::<(), Error>(()) > +/// ``` > +pub type KVBox =3D Box; > + > +// SAFETY: `Box` is `Send` if `T` is `Send` because the data referenced = by `self.0` is unaliased. > +unsafe impl Send for Box > +where > + T: Send + ?Sized, > + A: Allocator, > +{ > +} > + > +// SAFETY: `Box` is `Sync` if `T` is `Sync` because the data referenced = by `self.0` is unaliased. > +unsafe impl Sync for Box > +where > + T: Send + ?Sized, > + A: Allocator, > +{ > +} > + > +impl Box > +where > + T: ?Sized, > + A: Allocator, > +{ > + /// Creates a new `Box` from a raw pointer. > + /// > + /// # Safety > + /// > + /// `raw` must point to valid memory, previously be allocated with `= A`, and provide at least > + /// the size of type `T`. For ZSTs `raw` must be a dangling pointer. > + #[inline] > + pub const unsafe fn from_raw(raw: *mut T) -> Self { > + // INVARIANT: Validity of `raw` is guaranteed by the safety prec= onditions of this function. > + // SAFETY: By the safety preconditions of this function, `raw` i= s 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 all= ocation will stay alive > + /// indefinitely. Use [`Box::from_raw`] to recover the [`Box`], drop= the value and free the > + /// allocation, if any. > + /// > + /// # Examples > + /// > + /// ``` > + /// let x =3D KBox::new(24, GFP_KERNEL)?; > + /// let ptr =3D KBox::into_raw(x); > + /// let x =3D unsafe { KBox::from_raw(ptr) }; > + /// > + /// assert_eq!(*x, 24); > + /// > + /// # Ok::<(), Error>(()) > + /// ``` > + #[inline] > + pub fn into_raw(b: Self) -> *mut T { > + let b =3D ManuallyDrop::new(b); > + > + b.0.as_ptr() > + } > + > + /// Consumes and leaks the `Box` and returns a mutable referen= ce. > + /// > + /// See [Box::into_raw] for more details. > + #[inline] > + pub fn leak<'a>(b: Self) -> &'a mut T > + where > + T: 'a, This bound shouldn't be needed, it is implicit, since `T` appears in the type `&'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) } > + } > + > + /// Converts a `Box` into a `Pin>`. If `T` does not = implement [`Unpin`], then > + /// `*b` will be pinned in memory and can't be moved. > + /// > + /// This moves `b` into `Pin` without moving `*b` or allocating and = copying any memory. > + #[inline] > + pub fn into_pin(b: Self) -> Pin { I would agree with Alice, that this is not really needed, since you can just as well write `Pin::from(my_box)`. > + // SAFETY: The value wrapped inside a `Pin>` cannot be= moved or replaced as long > + // as `T` does not implement `Unpin`. > + unsafe { Pin::new_unchecked(b) } > + } > +} > + > +impl Box, A> > +where > + A: Allocator, > +{ > + /// Converts a `Box, A>` to a `Box`. > + /// > + /// # Safety > + /// > + /// Callers must ensure that the value inside of `b` is in an initia= lized state. It is > + /// undefined behavior to call this function while the value inside = of `b` is not yet fully > + /// initialized. The second sentence is unnecessary safety documentation. It might still be useful as normal documentation though. > + pub unsafe fn assume_init(b: Self) -> Box { > + let raw =3D 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 initial= ized state. Hence, it is > + // safe to reconstruct the `Box` as `Box`. > + unsafe { Box::from_raw(raw as *mut T) } > + } > + > + /// 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. > + unsafe { Self::assume_init(b) } > + } > +} > + > +impl Box > +where > + A: Allocator, > +{ > + fn is_zst() -> bool { > + core::mem::size_of::() =3D=3D 0 > + } > + > + /// Creates a new `Box` and initializes its contents with `x`. > + /// > + /// New memory is allocated with `a`. The allocation may fail, in wh= ich case an error is Wrong case on "`a`" (can this also be a link?). > + /// returned. For ZSTs no memory is allocated. > + pub fn new(x: T, flags: Flags) -> Result { > + let b =3D Self::new_uninit(flags)?; > + Ok(Box::write(b, x)) > + } > + > + /// Creates a new `Box` with uninitialized contents. > + /// > + /// New memory is allocated with `a`. The allocation may fail, in wh= ich case an error is Ditto. > + /// returned. For ZSTs no memory is allocated. > + /// > + /// # Examples > + /// > + /// ``` > + /// let b =3D KBox::::new_uninit(GFP_KERNEL)?; > + /// let b =3D KBox::write(b, 24); > + /// > + /// assert_eq!(*b, 24_u64); > + /// > + /// # Ok::<(), Error>(()) > + /// ``` > + pub fn new_uninit(flags: Flags) -> Result, A>, Al= locError> { > + let ptr =3D if Self::is_zst() { > + NonNull::dangling() > + } else { > + let layout =3D core::alloc::Layout::new::>(); > + let ptr =3D A::alloc(layout, flags)?; > + > + ptr.cast() > + }; > + > + Ok(Box(ptr, PhantomData::)) Missing INVARIANT comment. --- Cheers, Benno > + }