Received-SPF: pass (google.com: domain of linux-kernel+bounces-120540-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Valentin Obst <kernel@valentinobst.de>
To: wedsonaf@gmail.com
Cc: a.hindborg@samsung.com,
	alex.gaynor@gmail.com,
	aliceryhl@google.com,
	benno.lossin@proton.me,
	bjorn3_gh@protonmail.com,
	boqun.feng@gmail.com,
	gary@garyguo.net,
	linux-kernel@vger.kernel.org,
	ojeda@kernel.org,
	rust-for-linux@vger.kernel.org,
	walmeida@microsoft.com,
	Trevor Gross <tmgross@umich.edu>,
	FUJITA Tomonori <fujita.tomonori@gmail.com>,
	Valentin Obst <kernel@valentinobst.de>
Subject: Re: [PATCH 1/2] rust: introduce `InPlaceModule`
Date: Wed, 27 Mar 2024 09:13:17 +0100
Message-ID: <20240327081317.45574-1-kernel@valentinobst.de>
In-Reply-To: <20240327032337.188938-2-wedsonaf@gmail.com>
References: <20240327032337.188938-2-wedsonaf@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:Yr1f17Cn2OE=;0K2wMy6d/77ATITYacg+zpDik+B
 lOiRzlMMWBQNTHOGKbJg0tFPHYEoKATgAx4E4y7KCIQWQ++WhhJXWrSs2uCoiDYNwtR9M51aq
 3Dv6G/23REBETH/6A8+wFSh3aDu4x6kVmfEPxl9sE19wIHodRjWO5IhpUfB8CrDNvKe4f8fqg
 atIxKKBr8YtnJ43ylrlcHeptwRxcGr3/U7vwssFx552MjcicQbmJdkwXEWU2mSa/OHu29KPbF
 WDi2YcunDoKrk0iT4ILWR+NDib3SJqsnf0KffdjBzfPUI8HS8jymVtoCApuL8kVJv9AaTt0G/
 AsMlFYExmhs0/3LAg5ES/6GwYz21H7xkDdoXWUI6Sb7GC2zvgYMUmxubtrT9R8nZGkfXAw/mD
 7e30qpTqmp21CB3uMaeE4iMQ9aOSSua7TVEL9mQ7eA2tVkMFyGTyNjBAcYDOzR7xLuRcQRZXv
 a/fI7X3ntEL2A4vvU2BDsRSnUwtaYAzsAu8y50GHD7x0UUN03Joq4EuP96OZUuk9KmMAgRcXu
 Gsa7f95ZPJfy04Y8PqzodMcfxcQ5514X84soBGixqQzZ1/P+D3qxKVdb/9x6NyAhfE1V2mHNz
 YJIvWeOlKMpw6WUYOmzzt7J3C2LiqpQkXTOCg2t13CZUPIdp9k2S9b/gxkQpAvCzTOxYqwJqP
 cOW3iyz/Xcj5NsJJlJEomH/BkLBcv9Tmwheb1Yj4m1QyBPf0Bgc9XRDxmUtq5N2DMDFPRYzxX
 HoH9rfSNgKkojE+kjlVtckq7qQTpggcsvOjVCv8tkASGLPnDTBEBmI=

> This allows modules to be initialised in-place in pinned memory, which
> enables the usage of pinned types (e.g., mutexes, spinlocks, driver
> registrations, etc.) in modules without any extra allocations.
>
> Drivers that don't need this may continue to implement `Module` without
> any changes.
>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
>  rust/kernel/lib.rs    | 25 ++++++++++++++++++++++++-
>  rust/macros/module.rs | 18 ++++++------------
>  2 files changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 5c641233e26d..64aee4fbc53b 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -62,7 +62,7 @@
>  /// The top level entrypoint to implementing a kernel module.
>  ///
>  /// For any teardown or cleanup operations, your type may implement [`D=
rop`].
> -pub trait Module: Sized + Sync {
> +pub trait Module: Sized + Sync + Send {

This does not compile with `CONFIG_AX88796B_RUST_PHY=3Dy || m` (or the
phylib abstractions' doctests) since the module `Registration` is not
`Send`.

I remember Trevor raising the question whether we want to require modules
to be `Send`. I am not aware of any examples of `!Send` modules but I gues=
s
it would be possible to write code that is only correct under the
assumption that it is loaded/unloaded in the same context.

@Trevor: Are you aware of any modules with that requirement?

I have been using this patch for quite a while with my TCP CCAs now
(without the `Send` bound) and did not experience any other issues; thus
offering:
	Tested-by: Valentin Obst <kernel@valentinobst.de>

	- Best Valentin

>      /// Called at module initialization time.
>      ///
>      /// Use this method to perform whatever setup or registration your =
module
> @@ -72,6 +72,29 @@ pub trait Module: Sized + Sync {
>      fn init(module: &'static ThisModule) -> error::Result<Self>;
>  }
>
> +/// A module that is pinned and initialised in-place.
> +pub trait InPlaceModule: Sync + Send {
> +    /// Creates an initialiser for the module.
> +    ///
> +    /// It is called when the module is loaded.
> +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, er=
ror::Error>;
> +}
> +
> +impl<T: Module> InPlaceModule for T {
> +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, er=
ror::Error> {
> +        let initer =3D move |slot: *mut Self| {
> +            let m =3D <Self as Module>::init(module)?;
> +
> +            // SAFETY: `slot` is valid for write per the contract with =
`pin_init_from_closure`.
> +            unsafe { slot.write(m) };
> +            Ok(())
> +        };
> +
> +        // SAFETY: On success, `initer` always fully initialises an ins=
tance of `Self`.
> +        unsafe { init::pin_init_from_closure(initer) }
> +    }
> +}
> +
>  /// Equivalent to `THIS_MODULE` in the C API.
>  ///
>  /// C header: [`include/linux/export.h`](srctree/include/linux/export.h=
)
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index 27979e582e4b..0b2bb4ec2fba 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -208,7 +208,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream=
 {
>              #[used]
>              static __IS_RUST_MODULE: () =3D ();
>
> -            static mut __MOD: Option<{type_}> =3D None;
> +            static mut __MOD: core::mem::MaybeUninit<{type_}> =3D core:=
:mem::MaybeUninit::uninit();
>
>              // SAFETY: `__this_module` is constructed by the kernel at =
load time and will not be
>              // freed until the module is unloaded.
> @@ -275,23 +275,17 @@ pub(crate) fn module(ts: TokenStream) -> TokenStre=
am {
>              }}
>
>              fn __init() -> core::ffi::c_int {{
> -                match <{type_} as kernel::Module>::init(&THIS_MODULE) {=
{
> -                    Ok(m) =3D> {{
> -                        unsafe {{
> -                            __MOD =3D Some(m);
> -                        }}
> -                        return 0;
> -                    }}
> -                    Err(e) =3D> {{
> -                        return e.to_errno();
> -                    }}
> +                let initer =3D <{type_} as kernel::InPlaceModule>::init=
(&THIS_MODULE);
> +                match unsafe {{ initer.__pinned_init(__MOD.as_mut_ptr()=
) }} {{
> +                    Ok(m) =3D> 0,
> +                    Err(e) =3D> e.to_errno(),
>                  }}
>              }}
>
>              fn __exit() {{
>                  unsafe {{
>                      // Invokes `drop()` on `__MOD`, which should be use=
d for cleanup.
> -                    __MOD =3D None;
> +                    __MOD.assume_init_drop();
>                  }}
>              }}
>
> --