Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 30 Mar 2023 12:08:32 +0100
From:   Gary Guo <gary@garyguo.net>
To:     y86-dev@protonmail.com
Cc:     Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        Boqun Feng <boqun.feng@gmail.com>,
        =?UTF-8?B?QmrDtnJu?= Roy Baron <bjorn3_gh@protonmail.com>,
        Alice Ryhl <alice@ryhl.io>, rust-for-linux@vger.kernel.org,
        linux-kernel@vger.kernel.org, patches@lists.linux.dev
Subject: Re: [PATCH v3 12/13] rust: sync: reduce stack usage of
 `UniqueArc::try_new_uninit`
Message-ID: <20230330120832.6ced3ecc.gary@garyguo.net>
In-Reply-To: <20230329223239.138757-13-y86-dev@protonmail.com>
References: <20230329223239.138757-13-y86-dev@protonmail.com>
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?pRzLZknrD89mEbqLnlLJyAOEyRF7AN+bLyQKPtU4w1Dg2b+/tizEA8GDl6By?=
 =?us-ascii?Q?XwF6/4u4/8hXmdvWvxOdR7dva4IWycwavI31UWgP0AMbcWZ7XsXBEivPejME?=
 =?us-ascii?Q?vJpzm5qzgi3V0/nHlZ1YXvJYUzQywxlpqZ2Kw7LuG8CORAgTBQ6IOgPTp7ev?=
 =?us-ascii?Q?+Sn4ufMPJDukas0aZROon9WkMzfB1/a+gEuF4WXYZ8HSjGXzLEj53fQ9I5rh?=
 =?us-ascii?Q?rxsuaKvKhO0ljw6PSwdR+Phfnt6aypeyR/hm7PcvMUbjZimXLZz8yTcwPQPe?=
 =?us-ascii?Q?X+Auh+gfsbc0uFTF4q8VKnuGpygXuYYBLuqbV1twB0e1ehz1hCJpS15gLuT0?=
 =?us-ascii?Q?XUp+7eW835qrxTNBZG15loPvwfaY9MrFTjpXfTl+7H4QH3dcYyx36dXS2mH0?=
 =?us-ascii?Q?BXlvCM7iTPtfaK/DtItPiCIHte5o1UAX+ddVQvTi75NCBeaz1L/t4Yxp0jJ6?=
 =?us-ascii?Q?CFFpFyV40hhO2uM3FPgTcKbyOuEDt4Gyjr2FeBMR+/582oxZcmyznGJm0//W?=
 =?us-ascii?Q?Y+sfS2sNKlbY3Ouq1rYyHeAA8TadmQYTS+TD8JoiqLtHAZniyXb2nCjx8a1G?=
 =?us-ascii?Q?Y3/Yp0nvAYDoX3b1Pdfys+l1F7Qyt5b7XgpCbj34jpPg0uZe3KcuNpB+b7Mr?=
 =?us-ascii?Q?d2iPiAtoDMCW5Q20Lj0I+su+eMjidAEKM9mo3KYecMNSSRP4Hx1VrpTpHwy+?=
 =?us-ascii?Q?V7WhuXK7/waJoF26ua0ikQaV5IDO5X5Vd7ncovgIcl5ZPhCZfh+ekz0Zd8Mn?=
 =?us-ascii?Q?9jxAvn7awRASSPD5HMwaWWH4L4S22aDka7dNjWczhqQLsq9y16FUhqtO/u0J?=
 =?us-ascii?Q?BLuTWRfaUkIhkwQJ5CseozID639vYlGZFkizi7N6RlPrXn1xwiWwcFHXtqVz?=
 =?us-ascii?Q?QVbsqAGd4iztC4AsK3zflrUorS/k45GC9CRo7zATnyIw4YZC/LyxrZJXD67Q?=
 =?us-ascii?Q?pbf/0ZlkuV0Uylp4/osxXqnPV5e29ceB3SB/0+j3Dua/7bxOFRSLKUnTBNjQ?=
 =?us-ascii?Q?KnJVStXOhsHD693RyzRnhln3dchA6p2IjfNloQCa6IscTqWWxWxuCR8XCQxC?=
 =?us-ascii?Q?tlX11Re9ImcFYE04Wtm1vaPEjNkPYC1K8vLCVoDd9AgRqUkxjEcD56t1wKQD?=
 =?us-ascii?Q?EdDohBRfRlDAla6+881cBQi4WYoF4oFtdAuM05eeFhP5fTBVS2v5V2oNY2jM?=
 =?us-ascii?Q?KbpVSxlu3zEy/hgBPzxAhHUMqIsJUiH0CW1GWu+9iEmSNq49y74uN2KBYZS6?=
 =?us-ascii?Q?v78/iTh/ZuixFx0965cRnBbDA71Kug54pn28k63G6LSsSAp2LgWdg9AP0yvw?=
 =?us-ascii?Q?OIj0jag4g6aK16eEuc7+iNlHIm7Z9/Hw9v8S791zjNIF5o+LpkACOzVyEP/+?=
 =?us-ascii?Q?Jh0OGLygtQEt1RwSS81tP08r1ZD6/aZcudqyifAF9bY8LuXItAhR4fJEEdAn?=
 =?us-ascii?Q?v4p5/yb3cpIVgY/6EW7o6gbyXS0dzPQqoARGes9eK6BPdsiVSl3PznVCFSMw?=
 =?us-ascii?Q?xJuF0kpFVQmAj7iVIlxlYlzYUpOAO6qJ74EU4sOddyUMH3nF2k4VYQzQeY1k?=
 =?us-ascii?Q?BhTNF9GE8m4YewYpbrTZCG5wet+CTZQj7celrihW?=
X-OriginatorOrg: garyguo.net
X-MS-Exchange-CrossTenant-Network-Message-Id: 1e54b1bf-7bce-47b0-336d-08db310f1a24
X-MS-Exchange-CrossTenant-AuthSource: LO2P265MB5183.GBRP265.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Mar 2023 11:08:33.7101
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bbc898ad-b10f-4e10-8552-d9377b823d45
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 5iXiyP9GHbXtdsu6HS+cAkfQhZdKAsASXU1CY5GjBWFtRisA4IZek3c9XbXqjsxuHdNGAbAU6JivBTQKsZ/vjQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LO6P265MB6332
X-Spam-Status: No, score=-0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
        DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,
        SPF_HELO_PASS,SPF_PASS autolearn=unavailable autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, 29 Mar 2023 22:33:49 +0000
y86-dev@protonmail.com wrote:

> From: Benno Lossin <y86-dev@protonmail.com>
> 
> `UniqueArc::try_new_uninit` calls `Arc::try_new(MaybeUninit::uninit())`.
> This results in the uninitialized memory being placed on the stack,
> which may be arbitrarily large due to the generic `T` and thus could
> cause a stack overflow for large types.
> 
> Change the implementation to use the pin-init API which enables in-place
> initialization. In particular it avoids having to first construct and
> then move the uninitialized memory from the stack into the final location.
> 
> Signed-off-by: Benno Lossin <y86-dev@protonmail.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/lib.rs      |  1 -
>  rust/kernel/sync/arc.rs | 14 ++++++++++++--
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 3e2777d26ff5..d9df77132fa2 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -27,7 +27,6 @@
>  #[cfg(not(CONFIG_RUST))]
>  compile_error!("Missing kernel configuration for conditional compilation");
> 
> -#[allow(unused_extern_crates)]
>  // Allow proc-macros to refer to `::kernel` inside the `kernel` crate (this crate).
>  extern crate self as kernel;
> 
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 77a3833cc265..4ed6329a5e5f 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -18,6 +18,7 @@
>  use crate::{
>      bindings,
>      error::{Error, Result},
> +    init,
>      init::{InPlaceInit, Init, PinInit},
>      types::{ForeignOwnable, Opaque},
>  };
> @@ -29,6 +30,7 @@ use core::{
>      pin::Pin,
>      ptr::NonNull,
>  };
> +use macros::pin_data;
> 
>  /// A reference-counted pointer to an instance of `T`.
>  ///
> @@ -121,6 +123,7 @@ pub struct Arc<T: ?Sized> {
>      _p: PhantomData<ArcInner<T>>,
>  }
> 
> +#[pin_data]
>  #[repr(C)]
>  struct ArcInner<T: ?Sized> {
>      refcount: Opaque<bindings::refcount_t>,
> @@ -501,9 +504,16 @@ impl<T> UniqueArc<T> {
> 
>      /// Tries to allocate a new [`UniqueArc`] instance whose contents are not initialised yet.
>      pub fn try_new_uninit() -> Result<UniqueArc<MaybeUninit<T>>> {
> -        Ok(UniqueArc::<MaybeUninit<T>> {
> +        // INVARIANT: The refcount is initialised to a non-zero value.
> +        let inner = Box::init(init!(ArcInner {
> +            // SAFETY: There are no safety requirements for this FFI call.
> +            refcount: Opaque::new(unsafe { bindings::REFCOUNT_INIT(1) }),
> +            data <- init::uninit(),
> +        }))?;
> +        Ok(UniqueArc {
>              // INVARIANT: The newly-created object has a ref-count of 1.
> -            inner: Arc::try_new(MaybeUninit::uninit())?,
> +            // SAFETY: The pointer from the `Box` is valid.
> +            inner: unsafe { Arc::from_inner(Box::leak(inner).into()) },
>          })
>      }
>  }
> --
> 2.39.2
> 
>