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 03B37C3DA64 for ; Thu, 1 Aug 2024 08:56:08 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 85DC66B0082; Thu, 1 Aug 2024 04:56:07 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 80C296B0088; Thu, 1 Aug 2024 04:56:07 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6D41F6B008A; Thu, 1 Aug 2024 04:56:07 -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 4ECF36B0088 for ; Thu, 1 Aug 2024 04:56:07 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id AD58A1A0903 for ; Thu, 1 Aug 2024 08:56:06 +0000 (UTC) X-FDA: 82403069532.07.C3D5219 Received: from mail-wr1-f41.google.com (mail-wr1-f41.google.com [209.85.221.41]) by imf27.hostedemail.com (Postfix) with ESMTP id C725440020 for ; Thu, 1 Aug 2024 08:56:04 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=S6GNRmHQ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf27.hostedemail.com: domain of aliceryhl@google.com designates 209.85.221.41 as permitted sender) smtp.mailfrom=aliceryhl@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722502518; a=rsa-sha256; cv=none; b=m3FGmo+dcwpB9PuvRiNl3JH/ARM9TMBWy9001gHXwKFCB1WHqFv7WtTff/FNnpSsGmQBrG DzMfkGK6dMjVrqfRd2J5JQYDzb9cNItaMyGwbVW7uqpbYpp2X2qdFGB3nTFAOvCNYPP0ZR VftSc/mVhSQbtinCBD+YS0GcHL3A/pA= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=S6GNRmHQ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf27.hostedemail.com: domain of aliceryhl@google.com designates 209.85.221.41 as permitted sender) smtp.mailfrom=aliceryhl@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1722502518; 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=fqO6Iix0wCIf9YemqZbokzS5RPd4qkoavIabrTwRodw=; b=HSQXaWr0nquOccN2WPC0zP50oreOMjao6psk5rD06PKPTJ0Aly5bMKSiYH5qJNQonzvevC pTp8kiH60JQgI5LwbgkQFJLaqFL0qiIpv7X6EjeqYjg1UbFgzmGROxee1BGvo4oGVHtxXl /T8I0zXS0NydsWosQ7h9pwIdMtf2eWM= Received: by mail-wr1-f41.google.com with SMTP id ffacd0b85a97d-3683f56b9bdso3315016f8f.1 for ; Thu, 01 Aug 2024 01:56:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1722502563; x=1723107363; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=fqO6Iix0wCIf9YemqZbokzS5RPd4qkoavIabrTwRodw=; b=S6GNRmHQaXawsdItiIAUkKAt9/lGQGOZvuvprSr69fKKxsAfc6Ix1pw9ZnHFlpvzUX cKmaB2Omux+/nFESDz9BztFqxd0n7mtqKTzDs3oJ1+nwQM4aXywvO38e1DzSb4DNC68R iP1snsaNVwdFF5nHf4I7zN9ABB+VE7dFcZMS7gAUSbE507KPZSn6Q+TX2kTUQHJ0T+cw Vl/YpPnu2aw9aRWkNI/dRWArEJEd+dHHSzRKhj+0Olq+s+dFQf+aw2DXgXN4c1DefNS2 +J0jUZ++GeQMRUih5EO46MZrpRgdDFBATRsnTisfHJ1vWyuov+NzxDqtj7+PKVCiQ7Mj hT1w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722502563; x=1723107363; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=fqO6Iix0wCIf9YemqZbokzS5RPd4qkoavIabrTwRodw=; b=D9vfk4W+vFCg6VXWoGdhwEe1zkrb31gW9n3fSw/HfYYbmV3zf8w2XgIJsJnp4bNfWZ RPTUaKxd4qWocTMvs2tX/DTObnRR8GZXgBuYxsurzj7rioRmtwbj173hijCcv7PWj+rU E6DEThUw5W+se7LdGRMcmZlNeqvYeucw9im1Yq2TTK+t404VK3j2IrDWmAb0GiQPkEMv f6hSxIZaDhYBWkeD2g+UG6Cb1Tb3XnZUfQoRpsrbqUpSjJFGVE871HS3CkbOfP2YU8lS 70vNQDf2y4YjbbyIMV+jDwsiYJ1QwvytBpheuEBaRQYx//ucukfDK+3bqU7Xn2TO/7nh kQNA== X-Forwarded-Encrypted: i=1; AJvYcCXcSaR5oeIc8+nOcwmlfG6oqUq3COYC6xkW08+xBsNu3YwPmmDFs2STyiT2WSwiEjC1O4HGuKlHhtHFrW28tIcax4c= X-Gm-Message-State: AOJu0Yzivxu5u4qATv1/o/CQy90wGFLIwiZxVfF5TXHhqWtoiX64Lzlh PoPxpcgZsVL44VQdntjhRQumXGP+gOnoCP9gn2RfHvg5525qU5hJoUa6YQ8+ZgUrNgKI6Ha5Osw 2DSXNZ/8faTGuTIKhisMTfY1JeY8A8at9HqCL X-Google-Smtp-Source: AGHT+IEJnIJSJM3c7ZxrjSpwo8kICD2epumG5XB51tiQWpb0SXEaiAk6zGgcS2+tg6CW5rhLO+jt6TupjzpSK0lZoh8= X-Received: by 2002:a5d:47af:0:b0:367:8f29:f7ae with SMTP id ffacd0b85a97d-36baaf507cfmr1372748f8f.49.1722502562925; Thu, 01 Aug 2024 01:56:02 -0700 (PDT) MIME-Version: 1.0 References: <20240801000641.1882-1-dakr@kernel.org> <20240801000641.1882-10-dakr@kernel.org> In-Reply-To: <20240801000641.1882-10-dakr@kernel.org> From: Alice Ryhl Date: Thu, 1 Aug 2024 10:55:51 +0200 Message-ID: Subject: Re: [PATCH v3 09/25] rust: alloc: implement kernel `Box` To: Danilo Krummrich 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: C725440020 X-Stat-Signature: qx19xb5k3n6q8rdkgjpyjginprxjpejp X-Rspam-User: X-HE-Tag: 1722502564-566235 X-HE-Meta: U2FsdGVkX196pr76qbN++xkanwybA5uqHvAIRDI7BKQLf9frXNrMoJ5euPBJkmoetpfQ+HSV1jtmsz6GZy53Rk+/IwPZ68gyDQP9oaW4xjPpf73sx4wqG6vjoakx1F6U0bW2ZtCiAmEnBtYyWm1paCEAB+frLrtvdCx9LdwzCNsyyhFDgxKLCDhoQyX6+y2inCUWtKvMZWPYJPEriO/oosGIywA0y1LuOTT0RA/p8lrsP/T0RMD3PAJTF8dJMpmbR4VyD/ulpDsClsTT+g801xBkjN/ef6ujHSTgrWSIqsoLjZkMdbEyxZoQtQcHp85aOa5HLBZv9+bmdEFILoIiOyO8Z0w8iq8gouzVNVyqv2wvjFBP/8Yh2rg5WTxSmB0SWoOadASIUttUry3iUEAahFY82u3Wc+qJ2vrzbKQer0bdpknrGGAN0IsitVGhikD7M/kHDSDNyPa1Xa/wUvAA6gbyJsB0JQVgxhqTDN4ahyiFrYigxTzJphABGgHzkxY4q4tEDGGLinngJn/ovyxERC8J3BhJxks5qj8EjIU+mi0VGrKd+62GgS/MKPwJ1wsZEe945r8hZGW3yEJLRms2YyFnqob6Dwm9ONyWHWpZ4t+/YN4sdhBwyokV5P5uxdwNJxZIzl1kOxRKrsKCI1xd2sB14l/fgEWmSvsLwAbGgyTtFvcTy75A15fi3GUVFni+UwaN8HBGRJIYHBgleg+1lk9O6lcOwSc8fjxXI+Q5vfGMoPAbR+rqdk8k+tSZ0fAJrdjb9ANm//45LMTFsQtuI5WImMXhyfqaI5KaTPa0TPXwaIMKi8c6r5vf3NiijhKDci8id1BaXSrQZPCJ/QNL8fsAf1HdhVNYrtDI9rphE+bsjjAvPWa/YQZOlyduXB7wzMQ9IHf2F5af2m04hIuHCJcDYiDAurYHCCsHGyxANifxF6C4/qcaTvsjutYsRTCGD/suMRbxpSXj0xBCAfB OMnAn+dF tBm0/+mTS+0iNew+hRkPdlhmEq5ofp3Wl9N4SRjgoye33/AnVzGab98qdAUTHwkghctLDldG9fTTyuMhEbWIkLBIbpceayqHSrJemlBwIk8d4+/6FvuwE4ElrldFYMyr7Xrs3AKr/WAsOvr/MdVwpKhzxU9MyMnFc4/KmKzWlg/gtCq1+ubRNhwvq+1odSZ7sSK5NNNw+hGRDncScAq/U6P82c3ALy8Z6nBaPlD+T08BD8JZMLJ5qM8PMzW7Ck75dAn7FovnbWVaW7lB9v+lE/IJDVZ5TO+5tcEuRGWh4ZZ2L67xDCEMijM8KJiVE4orbw8SG50wlFdzbLgk= X-Bogosity: Ham, tests=bogofilter, spamicity=0.006858, 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 1, 2024 at 2:07=E2=80=AFAM Danilo Krummrich w= rote: > > `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 `P= hantomData` of the allocator > + /// it was allocated with. > + pub fn into_raw_alloc(b: Self) -> (*mut T, PhantomData) { > + let b =3D ManuallyDrop::new(b); > + let alloc =3D 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. > + /// 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 addi= tional 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. > +impl Drop for Box > +where > + T: ?Sized, > + A: Allocator, > +{ > + fn drop(&mut self) { > + let ptr =3D 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) } !=3D 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`. 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. > +impl ForeignOwnable for crate::alloc::Box > +where > + A: Allocator, > +{ > + type Borrowed<'a> =3D &'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 th= e 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-b1d70cdae= 541@google.com/ Alice