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 7958AC3DA7F for ; Thu, 15 Aug 2024 13:25:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0A46C6B00E9; Thu, 15 Aug 2024 09:25:00 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 055996B00EA; Thu, 15 Aug 2024 09:25:00 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E37C66B00EB; Thu, 15 Aug 2024 09:24:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id C1C0C6B00E9 for ; Thu, 15 Aug 2024 09:24:59 -0400 (EDT) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 77C011215CB for ; Thu, 15 Aug 2024 13:24:59 +0000 (UTC) X-FDA: 82454550318.22.496F755 Received: from mail-40131.protonmail.ch (mail-40131.protonmail.ch [185.70.40.131]) by imf30.hostedemail.com (Postfix) with ESMTP id 685A48000B for ; Thu, 15 Aug 2024 13:24:57 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b=nu0Nu4Ec; spf=pass (imf30.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.40.131 as permitted sender) smtp.mailfrom=benno.lossin@proton.me; dmarc=pass (policy=quarantine) header.from=proton.me ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1723728261; a=rsa-sha256; cv=none; b=TnOXa0nb2K3V1/dFZ42zQ9fh7+cKGzTyYDoIFj6Hgj2OGK5zzI4LbDq3N5D8FnpehWLHiJ 1w4BVhZwI/NNnShTp7VTN0hg2UnPnfIhhaMj0OhUzfaEu2+cXhQ8NNA2Y2QqEQ1ny3xnES xrpde6vrUtic/VwsCVKf21cgECgag1g= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b=nu0Nu4Ec; spf=pass (imf30.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.40.131 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=1723728261; 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=FMUGOuajKyHTFH9u5tNpnNSvmkR75tGzNAEVW81ZOIQ=; b=dhnldFnu1JVfFqK1zZD6PtwwGgtutWRyVXSF4rlobE8DFSXPlWlRc33AkMfdI3otmmX+LR yvFdCBToBHd6wlTvQ8M4aslcNc+EzwlNtRQ0AKugattlY9t9jGq64pJy32Rj9rHwd2GLLh nL3BqlvazOGXB3+MEW+kPaCR2rfIAPs= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=protonmail; t=1723728293; x=1723987493; bh=FMUGOuajKyHTFH9u5tNpnNSvmkR75tGzNAEVW81ZOIQ=; 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=nu0Nu4EcX121OuyUu+gqX57ZxpaD89GsTOG4XArgK727k+VNKjJqxG34JjvM/fMox QljUM54jYei8hmVzvt5c5RdSKtCCmMpdeG9CZEO7DKC1J17CZ7sFD3T7X/mhfahPno fsHQJf1qeDfREd1GZJqL7TEFhK9IbH8ODmbtk/+8LNPaCzMFiDQlC8se04Kw77F+B0 fJM2qGIAmZWY4Fcqc1oUtuSjM7cVfv831bRHdpBT/3NV7OqMVuYHtL3Gq1eTD3Uljg z969O7zZIaWwAfYmNV5wvcxdOyDK0rCfka4/fgJsfE2dn61RXtg1yu4Hvoopteu3ii rzPeCzHL7ai/w== Date: Thu, 15 Aug 2024 13:24:47 +0000 To: Danilo Krummrich From: 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 v5 09/26] rust: alloc: implement kernel `Box` Message-ID: <56ebda7b-c570-4dc6-8456-ab768d3a4b77@proton.me> In-Reply-To: References: <20240812182355.11641-1-dakr@kernel.org> <20240812182355.11641-10-dakr@kernel.org> Feedback-ID: 71780778:user:proton X-Pm-Message-ID: a4f9b902f88d07b334aa99c53ec0e419bff8c295 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Stat-Signature: nwa14raph9f7k9cgiufezbhnm7wyxtfe X-Rspamd-Queue-Id: 685A48000B X-Rspam-User: X-Rspamd-Server: rspam10 X-HE-Tag: 1723728297-73717 X-HE-Meta: U2FsdGVkX1+2nbjeEj741dPkNx3xzDZuTUNt3vND4QIO910ieQ2uUoZ67bm9gQsRRdKRSdgJ8cULM0xmNHy5q3/F0TIpTvPsq38ruu776dT6xeQkgQ8IGEA367R3bYMKBUcmVk7o7vzvltn8Fu5hSkqFcOWOOiZQzpbqE49t3kZ9zO1KZoNlwDFDG3NRVvU81J5mIElAs+JrcNdjg5yUn3S0InpL8y0MDfYqYe1HN+s9U1esPy3CdEsOfq/6Z6vkY7SdsZzNChiIUgBFSMqSSIYT5QR276ddMijAQCXYvMokbBecv0xj0KvQ1gsjyMSGy4ibmVhSsd/8aIuTPCcnj5NloqaRadrSOMYDvHJdw5KG0DlSzTR/YMzq9Lev8B6UtiDv0OfoebXLVS2aIMrk6wZum9PxbiY0dgI89E0k3qgiKwI/OQ5zdO05HvPorApHNIsfONaJFeVY9BLx9qRTXXunN+aIjXHD/x68di/NXl/F2CvaO7NAvAZEN48FOtYDmIE+9D+RLM0UXX2oHS/HaTF4Gwk8uhw4mpzWm5f2plwwfhbbCxyGkf8fUMrMyhhciBzgEIrCpglGTVUqSClAKSYNPv1pgD0Xv9PByM9y3pPAK2YTUbHbfnYeuwW0gsARTiE6Y4nOHlIXtwP3O4rKJoHRDSlph3OYesWCrD25DhmbkICwc3ZhGbiRmAaonEpy6w0mte/Xvb4jtbJ+sg7qp39+pRFZJqmZcD/hmfuhycFsxxc1RrWWaQ3fkNHjBonUrzygv+J1R4uk/7mGM/klMf3hm4qL0vB1KABXqaIw+V4WiTbVnKMB5bQvssOh9KuSuRWyt0gJUU7PUQF6HvyyE9FMq3LUZpVD+H7vT/o2BW7yWDQ4a4sBrYlaRGr/RBZJwTLPZ2oPMwaW0Rzj5exOGTew5VHrRA1yTnIOrMtiTnFqXWj+1EPdturOQO8x/XY9LNjxc8TLVpwAL8Alzvr pGQwKRW5 JODTsbAuFcm6C8h0w3cWjFKOu03W8nXXrFoAmHeLwbGohA+11Pc9pXL+lQzXIV97a1kDuQElMuMvSsUoJel4Dh1CsfuRga4vfVVyCPZnlNf5WVgLRc1uuoPSw2VfXC0tQ9hC/5uovro4AxumfoFCVxm/ycpMsCTyoF76nEF5+hRWU2fcij0DAy1RsCt8d9rzgpi1e2VfcuPTOxYb+rlc4gzK9/XwPn4Xs6OHGZEMLPu6qSJo4eNNXWYbSFk+AOgO4zN+AWva3MEbafVSbP3zjVsDRrnPx+wmr816kPHm74SDDeeRZF7h+0IWwVef+ZUAXBabY5nb9mErvib3G+ydjCmDmk2T9GVnRrphjtXgtrKJifRFCzUv1jKayEXiFmzlRfCqmos8PXdVQI060XcSfDnYDnicwdTRFjv1RwJmj5B1FMuynFywMYejR9+QQqR5P2KD/Fr9uId9yl7u7PulPIMqecfVVIBfNBwoaiYl4td1IYUUKZvp9xmDH162X/2sxYSl9 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000002, 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 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 o= f 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"). >=20 > 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. >=20 >> 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. >=20 > Obviously, there are also quite some API differences. For instance, `Box` > generally requires two generics, value type and allocator, we take page f= lags > 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::all= ocator::Kmalloc`], >>> +/// [`super::allocator::Vmalloc`] or [`super::allocator::KVmalloc`]. T= here are aliases for `Box` >>> +/// with these allocators ([`KBox`], [`VBox`], [`KVBox`]). >>> +/// >>> +/// When dropping a [`Box`], the value is also dropped and the heap me= mory 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 = 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(NonNull, PhantomData); >> >> Why no `repr(transparent)`? >=20 > 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. >=20 > I don't want duplicate the existing documentation around kmalloc and frie= nds > [1]. >=20 > Maybe we can refer to the existing documentation somehow. >=20 > [1] https://www.kernel.org/doc/html/latest/core-api/memory-allocation.htm= l 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 =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`). >=20 > Same as above, I'd rather not duplicate that. But I'm happy to link thing= s 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