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 2F241C3DA4A for ; Thu, 1 Aug 2024 12:45:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B58A46B0083; Thu, 1 Aug 2024 08:45:54 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AE12C6B0088; Thu, 1 Aug 2024 08:45:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 981D66B0089; Thu, 1 Aug 2024 08:45:54 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 778CE6B0083 for ; Thu, 1 Aug 2024 08:45:54 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 137C3120D6B for ; Thu, 1 Aug 2024 12:45:54 +0000 (UTC) X-FDA: 82403648628.04.0307E3C Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by imf28.hostedemail.com (Postfix) with ESMTP id 95643C001F for ; Thu, 1 Aug 2024 12:45:51 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b="ReNm/kjV"; spf=pass (imf28.hostedemail.com: domain of dakr@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=dakr@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1722516347; 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=HmLq627pnv0MVsB/S1SfcJPr4WiB1AEbEk4+uymXW3o=; b=e/nk7l1PSVnWbbuBkRKWYaLMBRvSRYHapIn+YOJAbS8o0jQaxnf4OoolugFjqZNdzfwEAk D0rV5f0ex6uug2bjHs0Kd0Xd7lLz2toypVtnaqyxYwenKBsApuQ/97NdSj52jfMn530NmL WPuKNtQ8i5Q6PcU2QT0QdPrn+7LFjzU= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b="ReNm/kjV"; spf=pass (imf28.hostedemail.com: domain of dakr@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=dakr@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722516347; a=rsa-sha256; cv=none; b=ah3BDKtkfNDCKJfBYb+T/+DqasWsN/LH4gTHHKSRc0MhvcvbXs+T1nSNTcEuw1pBQ3nNtj Q2RZNYFzTz5ddywTFTM6lJ1mDZqNAtb8iTL34Fmx8gXA7Zznw8dTh8CB5w0YemtZJTxSge a5ZphUl4eMQFxX2zw1mZMPZ/mcsPFh4= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id A6934CE1692; Thu, 1 Aug 2024 12:45:47 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 13129C32786; Thu, 1 Aug 2024 12:45:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1722516346; bh=uFy9XegXfV2RfEhcBYqHt6jbexjmhCmWnhmt33f0e3U=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ReNm/kjVk52BNkKSLm5Fc+fcd6UmE4GYHHIJAiNCo6ef7gmgAhCN0gHs0w/iAP18/ ZU2EDQnPY8dhP9QHKHFmY7sCO+62u76yNrFWO9PQnttSNThf3g3XybQQxWRqLfzF3z UNe9qo5EzNdgfzYSJ5aFlI3+1AYgiBQAbw8zxPD2wG3J+lZG3kLwWM+DmNqZmwf7VN 3jUZwY0of5Zmpm/wZt1xJRnse3u3pcZ5OSbOtUlxnHOEwuI5KMtkgLuLueCqRqfy5Y 59MXcnzxuDm41p84NBrKSiGSL9R0CbDsy+vPEWKJXRzqsaIFbhCaxZCJnAbGDnCnAR ZSC02tZxY2Tdw== Date: Thu, 1 Aug 2024 14:45:38 +0200 From: Danilo Krummrich To: Alice Ryhl Cc: ojeda@kernel.org, alex.gaynor@gmail.com, wedsonaf@gmail.com, boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com, benno.lossin@proton.me, a.hindborg@samsung.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, acurrid@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 v3 09/25] rust: alloc: implement kernel `Box` Message-ID: References: <20240801000641.1882-1-dakr@kernel.org> <20240801000641.1882-10-dakr@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspam-User: X-Stat-Signature: 38w4wsof71pe8rg7xca5qqf4w65kpm9u X-Rspamd-Queue-Id: 95643C001F X-Rspamd-Server: rspam11 X-HE-Tag: 1722516351-241011 X-HE-Meta: U2FsdGVkX1+Xn0ETqc3wEx1mv10NYkuqfrn1Uwgk0LKlK1rGBzVtOgIMv4CSYav9FZxwc/18VXFkIt1xkFmX7MJSo65m6rfbQO8X6Nkp4Z3Jm9EWCHxWHkP1uUqpPZVPxTZRICf+DKpVtQo2ejPdrinYWEWghRPVqu08eoNp4S+nf/T5f21crjLhqdT2CsuFI11O/ekje5F2jRKjpKOWcDj/RaCqIy/UTbm5sOwOU4+sLphQ3IxU1lkY1nnjQk1TVjCw9zwo7vpmLO0qnoK4LDFa2WPnYNMcAK5U46LCEmmtpIUnkYRIO9/gUWEP0MEujoYz0BnhTODi1sfa5KrzabP6OweIyfNjYT0TucHkOUy8lUjKkFRKk7ebEBfnRawGaEMu6bB9pg0+GqSx5mAtMv55oh+pJTkLLeiPMYCqWgXGp4CaI+pbkndDlQQr6eiY+Gt+lf1nhqXbUojGr5b/5gPPckdS3uczCwco9LzI7UHsRTJdRMkukaQ2SsCfMS2bhD47uFyTyoNgGQLNqrG3pFh+TmErYVdmxNMRQG1bua2OMmGT13vB/ItKQX8KFbGNgHvWYZHCwdgWECD+fcdqb2u6XFdGD6CtHXIz82bDCcmgM5OW96tpK3QzHDF5JsZSu16RKrc1v7y6iyZKm6xS+O072M2MQIlsQg8r+yC1NwEdrbOYYli1AL1vUoAHP3sxJbyumAzuUnTqMnqbosbfnX5wwWNDh8vTSIQNWGPg/2hfo6Q/OSTyH1qQwlyfiZZCqqh24GsASUdxSayM+eVNpxfaY5swttERfLoNCo0PyKvg6A6CaBEb2hsNsAba+RUO9eiZmfL+s5/9yHHqZSWhxe0ypccdyRZ25yLO5wu1iUw0kP24dxtL75Zr5ZAIqihsCz6PmJXwQEPEIvZZsn+rnhgpOvn9r5uGgg/c7BDStDVkIDc3Cn0IaCziwtU5DN7zQmZM/4Z6DdPC4VVTLqJ NIksxeHw F4U3gjU4+v7nkH8grjB/O39bsHq6KhQ+9M/7j1ZKrPlerja2Hq22nPWYHxyzo5zX7RG1bbFNbTjOPuS1wfHRRkIGAWCuxaMvAz4/i4ts4THf/zSaLbVfJhuDgzi2N4qy/vucePw1fE2/yJl7alP3XFNIzrp4k+ojWlM2Szen6WmV7xeBV7PCDy2rxSUydBaW07Xnu5NchhAUFvaqQViPcUHKtEAXUKjcFiFU+rflM3llueN+xyDO3h64Nr/CQgAjb/1Nr00aXEHrs/+hUptL4jcs2UuZULKKQnDn9hJkMXr+sYXIhivI9tTOgVQ1lfA3YpYc5LeLD6e06VfcAUftm9OjuBLMq4AhcuvCT4qp3tGdNufxrNfEQEqOhMN42kW1osHcfECXJYNrCG7Kr6Sw8AxlLjg== 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 Thu, Aug 01, 2024 at 10:55:51AM +0200, Alice Ryhl wrote: > On Thu, Aug 1, 2024 at 2:07 AM Danilo Krummrich wrote: > > > > `Box` provides the simplest way to allocate memory for a generic type > > with one of the kernel's allocators, e.g. `Kmalloc`, `Vmalloc` or > > `KVmalloc`. > > > > In contrast to Rust's `Box` type, the kernel `Box` type considers the > > kernel's GFP flags for all appropriate functions, always reports > > allocation failures through `Result<_, AllocError>` and remains > > independent from unstable features. > > > > Signed-off-by: Danilo Krummrich > > > > [...] > > > > + /// Constructs a `Box` from a raw pointer. > > + /// > > + /// # Safety > > + /// > > + /// `raw` must point to valid memory, previously allocated with `A`, and at least the size of > > + /// type `T`. > > + #[inline] > > + pub const unsafe fn from_raw_alloc(raw: *mut T, alloc: PhantomData) -> Self { > > + // SAFETY: Safe by the requirements of this function. > > + Box(unsafe { Unique::new_unchecked(raw) }, alloc) > > + } > > I don't think it makes sense to take the PhantomData as a parameter. > You can always create a PhantomData value out of thin air. > > Box(unsafe { Unique::new_unchecked(raw) }, PhantomData) > > > + /// Consumes the `Box`, returning a wrapped raw pointer and `PhantomData` of the allocator > > + /// it was allocated with. > > + pub fn into_raw_alloc(b: Self) -> (*mut T, PhantomData) { > > + let b = ManuallyDrop::new(b); > > + let alloc = unsafe { ptr::read(&b.1) }; > > + (b.0.as_ptr(), alloc) > > + } > > I don't think there's any need to have this function. The caller can > always create the PhantomData themselves. I would just keep into_raw > only. Agreed, I actually intended to remove this one and the above. > > > + /// Converts a `Box` into a `Pin>`. > > + #[inline] > > + pub fn into_pin(b: Self) -> Pin > > + where > > + A: 'static, > > + { > > + // SAFETY: It's not possible to move or replace the insides of a `Pin>` when > > + // `T: !Unpin`, so it's safe to pin it directly without any additional requirements. > > + unsafe { Pin::new_unchecked(b) } > > + } > > In the standard library, this functionality is provided using the From > trait rather than an inherent method. I think it makes sense to match > std here. I already provide `impl From> for Pin>` in this patch, which just calls `Box::into_pin`. > > > +impl Drop for Box > > +where > > + T: ?Sized, > > + A: Allocator, > > +{ > > + fn drop(&mut self) { > > + let ptr = self.0.as_ptr(); > > + > > + // SAFETY: We need to drop `self.0` in place, before we free the backing memory. > > + unsafe { core::ptr::drop_in_place(ptr) }; > > + > > + // SAFETY: `ptr` is always properly aligned, dereferenceable and points to an initialized > > + // instance of `T`. > > + if unsafe { core::mem::size_of_val(&*ptr) } != 0 { > > + // SAFETY: `ptr` was previously allocated with `A`. > > + unsafe { A::free(self.0.as_non_null().cast()) }; > > + } > > You just destroyed the value by calling `drop_in_place`, so `ptr` no > longer points at an initialized instance of `T`. Please compute > whether the allocation has non-zero size before you call > `drop_in_place`. Huh! Good catch. No idea how I missed that. > > Also, in normal Rust this code would leak the allocation on panic in > the destructor. We may not care, but it's worth taking into account if > anybody else copies this code to a different project with a different > panic configuration. I can add a corresponding note. > > > +impl ForeignOwnable for crate::alloc::Box > > +where > > + A: Allocator, > > +{ > > + type Borrowed<'a> = &'a T; > > + > > + fn into_foreign(self) -> *const core::ffi::c_void { > > + crate::alloc::Box::into_raw(self) as _ > > + } > > + > > + unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'a T { > > + // SAFETY: The safety requirements for this function ensure that the object is still alive, > > + // so it is safe to dereference the raw pointer. > > + // The safety requirements of `from_foreign` also ensure that the object remains alive for > > + // the lifetime of the returned value. > > + unsafe { &*ptr.cast() } > > + } > > + > > + unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self { > > + // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous > > + // call to `Self::into_foreign`. > > + unsafe { crate::alloc::Box::from_raw(ptr as _) } > > + } > > +} > > You may want to also implement ForeignOwnable for Pin>. See: > https://lore.kernel.org/all/20240730-foreign-ownable-pin-box-v1-1-b1d70cdae541@google.com/ Yeah, I think I've also seen another patch that it about to add a function to convert a `Box` back into uninit state. Depending how fast you need ForeignOwnable for Pin>, do you prefer to contribute a corresponding patch to this series? > > Alice >