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 7F042EE01F1 for ; Tue, 10 Sep 2024 23:25:58 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C2BFB8D00C8; Tue, 10 Sep 2024 19:25:57 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BDC0D8D0002; Tue, 10 Sep 2024 19:25:57 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id ACAB28D00C8; Tue, 10 Sep 2024 19:25:57 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 8F8EF8D0002 for ; Tue, 10 Sep 2024 19:25:57 -0400 (EDT) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id AE145140CDE for ; Tue, 10 Sep 2024 23:25:56 +0000 (UTC) X-FDA: 82550413512.24.C2AC967 Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf23.hostedemail.com (Postfix) with ESMTP id 08982140016 for ; Tue, 10 Sep 2024 23:25:54 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=c56a4Lah; spf=pass (imf23.hostedemail.com: domain of dakr@kernel.org designates 147.75.193.91 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=1726010703; 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=Styl0qIc39UDomSIQtjNNme1oInkGhMKEPpl5EaCqx4=; b=IJaEe88SOaXWsNE03vdCTcLGyyTUR0Zzpw8wB+BR4sGSKpDVKcXBIcDGe9uT0GHmLTNd97 T02uARI2HhcsUKwdPLNNEemrptoWPXvMpMah5BZLYEVlBpSitG70zgmAxWDDjrOszbNIhl 0mbBuGXvSXIIIFM8PdzLRXOYO6cigHE= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=c56a4Lah; spf=pass (imf23.hostedemail.com: domain of dakr@kernel.org designates 147.75.193.91 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=1726010703; a=rsa-sha256; cv=none; b=fmxsdNhnFlrIRHS3rA/1/lh5AfwMVT9CmX6GyVrUDIyxFvbk7A0mFUnwf7yoAV6QnP4VxP TT8DQMsDFjQjtugiQR7Ix2XBA8WvUccPmkMfq5FUl9qrL3nrgb70o6TU0d9LmzCKwc9AzA gZhJMGWRnhEFUFJoi2lNogavEal62C0= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 720FEA43FDB; Tue, 10 Sep 2024 23:25:46 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DC5FDC4CEC3; Tue, 10 Sep 2024 23:25:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1726010753; bh=jWbTVGJQQJ9or0giI5ucycy3BO0lPAlGG7IxUJVCoG0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=c56a4LahYMixLImso+PUxaJI0z2WGqSqY+LG6P6j9Fk0X/bMfxBHugfQbq6LLPri9 Vjn0TzBdxtHVoqgZsWEcND1v4XDSYZb0G2+VB0T0Sqxs9dxBbQ1rq1qZUdXu1I+q3m mBqlsWOAuteFOW/Dff3mK3meyhnAuh0y1oygcsQkv3cQgDIJ4R8Ck8JQ83PdDZsVon leFPMLjiiFYPAdtErLaNSrrYOBq2mwh1k01N+czjsCiK1cGxVYt3aRGPPw923OdbJx Uee5SdbZHGtyz+Dz4PVFdFQqud8Y/KGgWzddDKva4Nj0MUbS0qELl4qNXQfHez4jDd kHmI4tMUkrsTA== Date: Wed, 11 Sep 2024 01:25:46 +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> <19d81a27-9600-4990-867c-624104e3ad83@proton.me> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <19d81a27-9600-4990-867c-624104e3ad83@proton.me> X-Rspamd-Server: rspam03 X-Rspam-User: X-Rspamd-Queue-Id: 08982140016 X-Stat-Signature: pgic6fg87awr357acgtqdgxnb99d9bwi X-HE-Tag: 1726010754-780620 X-HE-Meta: U2FsdGVkX19iZKrR+rLwJJtAd7bJtGC4eCaFsyVOOtMb0brmNg4wLLRmlDupH6U/LDns1qTYieuK7E9PKtu9blUle1rtH8sTNqpIgfbrlprLksHUxYbEoUMtRBtdqftsKImvd2Ii1C9mDwhyw/nlDFT9gm0lQQn+cddokFuucjIPFxOD760+ork1T9M718tYD/28v9dHrMnv/9SSuJTYU0STu3V9hqr7nmmAgikJr+p/FV08LNHCTUSSOdtziFjXyFKPunWUVIgPAuw8c9hto+Zax1prCTHo/ZvkL8v7bFsZNuh8JiL+sWtw5bRPk25WKi9UndkW941zlo1bmtzgvR/E64wTnFT6pYcVLHJ4MPP6EVoRXpA3ULrsQCFGreU7tgWRNip+RBxNWmMaA1/nRczrDRIJupEkqfG8djPkl+9qu9qQZp3eK2C8J6iQpZVUje3NXd+/okRaoafuWP3VaNHt2EkPduDcJxiGGr7hlqudFA7h6v8wCMLSORuuq0fD4jEhqLb3Wn86iPTFCzTCTCyisvUsrB2h3BYAwBEWwGPhXfh4kuAHje24bDt2QP34/349nip46SUpHkVuQCe4ZzmZxtK1vYpq8vKjWOfmKnc9zWpSnlaLevHBxxQvnBUNF4qJ1BDrWrdBRQHWCsXTFlOwu/ubflucn64AuabSAOGJnBrVlCGArQNDty6CNfXN7it6MOoEHnjg9ZGsvcJbhn8ynJO5RNZUKA9b+3Sg1zA/rfhuAn+ViGEmiPt1zSpVNebMMu/eLyujx7D9OmRSte/HtY8l9IaS68LNrxLAr3YgFr0xq6zHjG2aHGSLddK9SoGNjl7cphAjOZDL5VFAvlyMp9IUN48+NADpSRnsRNNde+OOKmb/zW57SpgF5ohd5/PKgCINy2iMIlRJtIlXV4d7FEJN1gTGhezdIiC6YGIvVWTz/xmcerW5ARnaYWMmlAG9+A61hmtcCytrYrl NSq7uhA8 IH1eRyoZzgaE+jLwqekD3jKNx7EMlMIM2buU4CyY0pkWnBPpTaBDFPKT9UTJPAmYXPBYslGmr9/gnSfbg4s7MD73bvB4CMlkQMzSS4937rrrzhvbU780uniJekV/uZBQ6/wGMIcEGuDPslwmkI0CcN6v+MmWKI+LMDY5s5iwy8ItvYpxmkS8FXy8mO9oyduiTmnkO63+FZHUDBvxlPVj1L2y2LYkBT7fRjtTPqunFlq03ZiWhtpHXpPZ31uY8bpDwGRqMRrsn/x02ZNtteVF0smgF85tYYxAp7bvOVZL162t92RB9kw2pbCHxwBOf1a3g6/mmCGAA85iYBckDMzq3x8Uj9P6c2Tc4TBPT9i8BZP2sJBkyEHjifLu8+NTEiwZrjWihskpLC00Su9VoQXTBBdeOBMPI+v29MzAF 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 Tue, Sep 10, 2024 at 07:49:42PM +0000, Benno Lossin wrote: > On 10.09.24 19:40, Danilo Krummrich wrote: > > On Sat, Aug 31, 2024 at 05:39:07AM +0000, Benno Lossin wrote: > >> On 16.08.24 02:10, Danilo Krummrich wrote: > >>> +/// # 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. > > Fair point, nevertheless I find examples a bit more useful, when the > intention behind them is not only given as code. > > >>> +/// > >>> +/// ``` > >>> +/// # 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? > > ZSTs can have alignment and then unaligned pointers do exist for them > (and dereferencing them is UB!): Where is this documented? The documentation says: "For operations of size zero, *every* pointer is valid, including the null pointer. The following points are only concerned with non-zero-sized accesses." [1] [1] https://doc.rust-lang.org/std/ptr/index.html > > #[repr(align(64))] > struct Token; > > fn main() { > let t = 64 as *mut Token; > let t = unsafe { t.read() }; // this is fine. > let t = 4 as *mut Token; > let t = unsafe { t.read() }; // this is UB, see below for miri's output > } > > Miri complains: > > error: Undefined Behavior: accessing memory based on pointer with alignment 4, but alignment 64 is required > --> src/main.rs:8:22 > | > 8 | let t = unsafe { t.read() }; // this is UB, see below for miri's output > | ^^^^^^^^ accessing memory based on pointer with alignment 4, but alignment 64 is required > | > = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior > = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information > = note: BACKTRACE: > = note: inside `main` at src/main.rs:8:22: 8:30 `read` explicitly asks for non-null and properly aligned even if `T` has size zero. > > >>> +#[repr(transparent)] > >>> +pub struct Box(NonNull, PhantomData); > > > >>> +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? > > We require a non-null, well aligned pointer (see above). We indeed do, gonna add it. > > --- > Cheers, > Benno > > >>> + #[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::) > >>> + } >