[PATCH 0/2] In-place module initialisation

[PATCH 0/2] In-place module initialisation

[Wedson Almeida Filho]

From: Wedson Almeida Filho <walmeida@microsoft.com>

Introduce `InPlaceModule`, which allows modules to be initialised
inplace, as opposed to the current state where modules must return an
instance which is moved to its final memory location.

This allows us to have modules whose state hold objects that must be
initialised inplace like locks. It also allows us to implement
registrations (e.g., driver registration) inplace and have them similar
to their C counterparts where no new allocations are needed.

This is built on top of the allocation APIs because the sample module is
a modified version of rust_minimal, which would be incompatible with the
allocation API series because it uses vectors.

Wedson Almeida Filho (2):
  rust: introduce `InPlaceModule`
  samples: rust: add in-place initialisation sample

 rust/kernel/lib.rs           | 25 ++++++++++++++++++++-
 rust/macros/module.rs        | 18 ++++++----------
 samples/rust/Kconfig         | 11 ++++++++++
 samples/rust/Makefile        |  1 +
 samples/rust/rust_inplace.rs | 42 ++++++++++++++++++++++++++++++++++++
 5 files changed, 84 insertions(+), 13 deletions(-)
 create mode 100644 samples/rust/rust_inplace.rs


base-commit: e0ff891dbadea6226042574a0cbfc24df0318b13
-- 
2.34.1

[PATCH 1/2] rust: introduce `InPlaceModule`

[Wedson Almeida Filho]

From: Wedson Almeida Filho <walmeida@microsoft.com>

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 [`Drop`].
-pub trait Module: Sized + Sync {
+pub trait Module: Sized + Sync + Send {
     /// 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, error::Error>;
+}
+
+impl<T: Module> InPlaceModule for T {
+    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error> {
+        let initer = move |slot: *mut Self| {
+            let m = <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 instance 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: () = ();
 
-            static mut __MOD: Option<{type_}> = None;
+            static mut __MOD: core::mem::MaybeUninit<{type_}> = 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) -> TokenStream {
             }}
 
             fn __init() -> core::ffi::c_int {{
-                match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
-                    Ok(m) => {{
-                        unsafe {{
-                            __MOD = Some(m);
-                        }}
-                        return 0;
-                    }}
-                    Err(e) => {{
-                        return e.to_errno();
-                    }}
+                let initer = <{type_} as kernel::InPlaceModule>::init(&THIS_MODULE);
+                match unsafe {{ initer.__pinned_init(__MOD.as_mut_ptr()) }} {{
+                    Ok(m) => 0,
+                    Err(e) => e.to_errno(),
                 }}
             }}
 
             fn __exit() {{
                 unsafe {{
                     // Invokes `drop()` on `__MOD`, which should be used for cleanup.
-                    __MOD = None;
+                    __MOD.assume_init_drop();
                 }}
             }}
 
-- 
2.34.1

[PATCH 2/2] samples: rust: add in-place initialisation sample

[Wedson Almeida Filho]

From: Wedson Almeida Filho <walmeida@microsoft.com>

This is a modified version of rust_minimal that is initialised in-place.

Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
 samples/rust/Kconfig         | 11 ++++++++++
 samples/rust/Makefile        |  1 +
 samples/rust/rust_inplace.rs | 42 ++++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+)
 create mode 100644 samples/rust/rust_inplace.rs

diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index b0f74a81c8f9..59f44a8b6958 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -20,6 +20,17 @@ config SAMPLE_RUST_MINIMAL
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_INPLACE
+	tristate "Minimal in-place"
+	help
+	  This option builds the Rust minimal module with in-place
+	  initialisation.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_inplace.
+
+	  If unsure, say N.
+
 config SAMPLE_RUST_PRINT
 	tristate "Printing macros"
 	help
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index 03086dabbea4..791fc18180e9 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
+obj-$(CONFIG_SAMPLE_RUST_INPLACE)		+= rust_inplace.o
 obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
 
 subdir-$(CONFIG_SAMPLE_RUST_HOSTPROGS)		+= hostprogs
diff --git a/samples/rust/rust_inplace.rs b/samples/rust/rust_inplace.rs
new file mode 100644
index 000000000000..ba8d051cac56
--- /dev/null
+++ b/samples/rust/rust_inplace.rs
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust minimal in-place sample.
+
+use kernel::prelude::*;
+
+module! {
+    type: RustInPlace,
+    name: "rust_inplace",
+    author: "Rust for Linux Contributors",
+    description: "Rust minimal in-place sample",
+    license: "GPL",
+}
+
+#[pin_data(PinnedDrop)]
+struct RustInPlace {
+    numbers: Vec<i32>,
+}
+
+impl kernel::InPlaceModule for RustInPlace {
+    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
+        pr_info!("Rust minimal sample (init)\n");
+        pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
+        try_pin_init!(Self {
+            numbers: {
+                let mut numbers = Vec::new();
+                numbers.push(72, GFP_KERNEL)?;
+                numbers.push(108, GFP_KERNEL)?;
+                numbers.push(200, GFP_KERNEL)?;
+                numbers
+            },
+        })
+    }
+}
+
+#[pinned_drop]
+impl PinnedDrop for RustInPlace {
+    fn drop(self: Pin<&mut Self>) {
+        pr_info!("My numbers are {:?}\n", self.numbers);
+        pr_info!("Rust minimal inplace sample (exit)\n");
+    }
+}
-- 
2.34.1

Re: [PATCH 1/2] rust: introduce `InPlaceModule`

[Valentin Obst]

> 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 [`Drop`].
> -pub trait Module: Sized + Sync {
> +pub trait Module: Sized + Sync + Send {

This does not compile with `CONFIG_AX88796B_RUST_PHY=y || 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 guess
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, error::Error>;
> +}
> +
> +impl<T: Module> InPlaceModule for T {
> +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error> {
> +        let initer = move |slot: *mut Self| {
> +            let m = <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 instance 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: () = ();
>
> -            static mut __MOD: Option<{type_}> = None;
> +            static mut __MOD: core::mem::MaybeUninit<{type_}> = 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) -> TokenStream {
>              }}
>
>              fn __init() -> core::ffi::c_int {{
> -                match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
> -                    Ok(m) => {{
> -                        unsafe {{
> -                            __MOD = Some(m);
> -                        }}
> -                        return 0;
> -                    }}
> -                    Err(e) => {{
> -                        return e.to_errno();
> -                    }}
> +                let initer = <{type_} as kernel::InPlaceModule>::init(&THIS_MODULE);
> +                match unsafe {{ initer.__pinned_init(__MOD.as_mut_ptr()) }} {{
> +                    Ok(m) => 0,
> +                    Err(e) => e.to_errno(),
>                  }}
>              }}
>
>              fn __exit() {{
>                  unsafe {{
>                      // Invokes `drop()` on `__MOD`, which should be used for cleanup.
> -                    __MOD = None;
> +                    __MOD.assume_init_drop();
>                  }}
>              }}
>
> --

Re: [PATCH 2/2] samples: rust: add in-place initialisation sample

[Benno Lossin]

On 27.03.24 04:23, Wedson Almeida Filho wrote:
> diff --git a/samples/rust/rust_inplace.rs b/samples/rust/rust_inplace.rs
> new file mode 100644
> index 000000000000..ba8d051cac56
> --- /dev/null
> +++ b/samples/rust/rust_inplace.rs
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Rust minimal in-place sample.
> +
> +use kernel::prelude::*;
> +
> +module! {
> +    type: RustInPlace,
> +    name: "rust_inplace",
> +    author: "Rust for Linux Contributors",
> +    description: "Rust minimal in-place sample",
> +    license: "GPL",
> +}
> +
> +#[pin_data(PinnedDrop)]
> +struct RustInPlace {
> +    numbers: Vec<i32>,
> +}
> +
> +impl kernel::InPlaceModule for RustInPlace {
> +    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> +        pr_info!("Rust minimal sample (init)\n");

This text needs updating.

> +        pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
> +        try_pin_init!(Self {
> +            numbers: {
> +                let mut numbers = Vec::new();
> +                numbers.push(72, GFP_KERNEL)?;
> +                numbers.push(108, GFP_KERNEL)?;
> +                numbers.push(200, GFP_KERNEL)?;
> +                numbers
> +            },
> +        })

I think it might be useful to also have a field that needs pin-init, eg
a `Mutex` or similar. What about placing the `Vec` inside of a mutex?

-- 
Cheers,
Benno

> +    }
> +}
> +
> +#[pinned_drop]
> +impl PinnedDrop for RustInPlace {
> +    fn drop(self: Pin<&mut Self>) {
> +        pr_info!("My numbers are {:?}\n", self.numbers);
> +        pr_info!("Rust minimal inplace sample (exit)\n");
> +    }
> +}
> --
> 2.34.1
> 

Re: [PATCH 1/2] rust: introduce `InPlaceModule`

[Wedson Almeida Filho]

On Wed, 27 Mar 2024 at 05:13, Valentin Obst <kernel@valentinobst.de> wrote:
>
> > 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 [`Drop`].
> > -pub trait Module: Sized + Sync {
> > +pub trait Module: Sized + Sync + Send {
>
> This does not compile with `CONFIG_AX88796B_RUST_PHY=y || m` (or the
> phylib abstractions' doctests) since the module `Registration` is not
> `Send`.

Thanks for the heads up. I thought I had enabled all rust code but
indeed I was missing this. I will fix it in v2.

> 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 guess
> it would be possible to write code that is only correct under the
> assumption that it is loaded/unloaded in the same context.

It might be possible in the future, but I don't believe it is now
because all rust modules support unloading. And there is no guarantee
that the thread unloading (and therefore calling module_exit) is the
same that loaded (and called module_init), so a module must be Send to
properly handle drop being called from a different thread.

Not requiring Send on the original Module trait was an oversight that
I don't want to repeat in InPlaceModule.

>
> @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>

Thanks!

>
>         - 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, error::Error>;
> > +}
> > +
> > +impl<T: Module> InPlaceModule for T {
> > +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error> {
> > +        let initer = move |slot: *mut Self| {
> > +            let m = <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 instance 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: () = ();
> >
> > -            static mut __MOD: Option<{type_}> = None;
> > +            static mut __MOD: core::mem::MaybeUninit<{type_}> = 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) -> TokenStream {
> >              }}
> >
> >              fn __init() -> core::ffi::c_int {{
> > -                match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
> > -                    Ok(m) => {{
> > -                        unsafe {{
> > -                            __MOD = Some(m);
> > -                        }}
> > -                        return 0;
> > -                    }}
> > -                    Err(e) => {{
> > -                        return e.to_errno();
> > -                    }}
> > +                let initer = <{type_} as kernel::InPlaceModule>::init(&THIS_MODULE);
> > +                match unsafe {{ initer.__pinned_init(__MOD.as_mut_ptr()) }} {{
> > +                    Ok(m) => 0,
> > +                    Err(e) => e.to_errno(),
> >                  }}
> >              }}
> >
> >              fn __exit() {{
> >                  unsafe {{
> >                      // Invokes `drop()` on `__MOD`, which should be used for cleanup.
> > -                    __MOD = None;
> > +                    __MOD.assume_init_drop();
> >                  }}
> >              }}
> >
> > --

Re: [PATCH 1/2] rust: introduce `InPlaceModule`

[Benno Lossin]

On 27.03.24 15:23, Wedson Almeida Filho wrote:
> On Wed, 27 Mar 2024 at 05:13, Valentin Obst <kernel@valentinobst.de> wrote:
>>
>>> 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 [`Drop`].
>>> -pub trait Module: Sized + Sync {
>>> +pub trait Module: Sized + Sync + Send {
>>
>> This does not compile with `CONFIG_AX88796B_RUST_PHY=y || m` (or the
>> phylib abstractions' doctests) since the module `Registration` is not
>> `Send`.
> 
> Thanks for the heads up. I thought I had enabled all rust code but
> indeed I was missing this. I will fix it in v2.
> 
>> 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 guess
>> it would be possible to write code that is only correct under the
>> assumption that it is loaded/unloaded in the same context.
> 
> It might be possible in the future, but I don't believe it is now
> because all rust modules support unloading. And there is no guarantee
> that the thread unloading (and therefore calling module_exit) is the
> same that loaded (and called module_init), so a module must be Send to
> properly handle drop being called from a different thread.
> 
> Not requiring Send on the original Module trait was an oversight that
> I don't want to repeat in InPlaceModule.

I think that this change should go to the stable tree, can you split it
into its own patch?

-- 
Cheers,
Benno

> 
>>
>> @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>
> 
> Thanks!
> 
>>
>>          - Best Valentin
>>

Re: [PATCH 1/2] rust: introduce `InPlaceModule`

[Benno Lossin]

On 27.03.24 04:23, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> 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>

I have some suggestions below, with those fixed,

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

> ---
>  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 [`Drop`].
> -pub trait Module: Sized + Sync {
> +pub trait Module: Sized + Sync + Send {
>      /// 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, error::Error>;
> +}
> +
> +impl<T: Module> InPlaceModule for T {
> +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error> {
> +        let initer = move |slot: *mut Self| {
> +            let m = <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 instance 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: () = ();
> 
> -            static mut __MOD: Option<{type_}> = None;
> +            static mut __MOD: core::mem::MaybeUninit<{type_}> = core::mem::MaybeUninit::uninit();

I would prefer `::core::mem::MaybeUninit`, since that prevents
accidentally referring to a module named `core`.

> 
>              // 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) -> TokenStream {
>              }}
> 
>              fn __init() -> core::ffi::c_int {{
> -                match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
> -                    Ok(m) => {{
> -                        unsafe {{
> -                            __MOD = Some(m);
> -                        }}
> -                        return 0;
> -                    }}
> -                    Err(e) => {{
> -                        return e.to_errno();
> -                    }}
> +                let initer = <{type_} as kernel::InPlaceModule>::init(&THIS_MODULE);

Ditto with `::kernel::InPlaceModule`.

> +                match unsafe {{ initer.__pinned_init(__MOD.as_mut_ptr()) }} {{

This requires that the `PinInit` trait is in scope, I would use:

     match unsafe {{ ::kernel::init::PinInit::__pinned_init(initer, __MOD.as_mut_ptr()) }} {{

-- 
Cheers,
Benno

> +                    Ok(m) => 0,
> +                    Err(e) => e.to_errno(),
>                  }}
>              }}
> 
>              fn __exit() {{
>                  unsafe {{
>                      // Invokes `drop()` on `__MOD`, which should be used for cleanup.
> -                    __MOD = None;
> +                    __MOD.assume_init_drop();
>                  }}
>              }}
> 
> --
> 2.34.1
> 

Re: [PATCH 1/2] rust: introduce `InPlaceModule`

[Wedson Almeida Filho]

On Wed, 27 Mar 2024 at 13:16, Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 27.03.24 04:23, Wedson Almeida Filho wrote:
> > From: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > 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>
>
> I have some suggestions below, with those fixed,
>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
>
> > ---
> >  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 [`Drop`].
> > -pub trait Module: Sized + Sync {
> > +pub trait Module: Sized + Sync + Send {
> >      /// 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, error::Error>;
> > +}
> > +
> > +impl<T: Module> InPlaceModule for T {
> > +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error> {
> > +        let initer = move |slot: *mut Self| {
> > +            let m = <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 instance 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: () = ();
> >
> > -            static mut __MOD: Option<{type_}> = None;
> > +            static mut __MOD: core::mem::MaybeUninit<{type_}> = core::mem::MaybeUninit::uninit();
>
> I would prefer `::core::mem::MaybeUninit`, since that prevents
> accidentally referring to a module named `core`.

Makes sense, changed in v3.

I also added a patch to v3 that adds `::` to all cases in the code
generated by the macro.

>
> >
> >              // 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) -> TokenStream {
> >              }}
> >
> >              fn __init() -> core::ffi::c_int {{
> > -                match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
> > -                    Ok(m) => {{
> > -                        unsafe {{
> > -                            __MOD = Some(m);
> > -                        }}
> > -                        return 0;
> > -                    }}
> > -                    Err(e) => {{
> > -                        return e.to_errno();
> > -                    }}
> > +                let initer = <{type_} as kernel::InPlaceModule>::init(&THIS_MODULE);
>
> Ditto with `::kernel::InPlaceModule`.
>
> > +                match unsafe {{ initer.__pinned_init(__MOD.as_mut_ptr()) }} {{
>
> This requires that the `PinInit` trait is in scope, I would use:
>
>      match unsafe {{ ::kernel::init::PinInit::__pinned_init(initer, __MOD.as_mut_ptr()) }} {{

Also changed in v3.

> --
> Cheers,
> Benno
>
> > +                    Ok(m) => 0,
> > +                    Err(e) => e.to_errno(),
> >                  }}
> >              }}
> >
> >              fn __exit() {{
> >                  unsafe {{
> >                      // Invokes `drop()` on `__MOD`, which should be used for cleanup.
> > -                    __MOD = None;
> > +                    __MOD.assume_init_drop();
> >                  }}
> >              }}
> >
> > --
> > 2.34.1
> >
>

Re: [PATCH 1/2] rust: introduce `InPlaceModule`

[Wedson Almeida Filho]

On Wed, 27 Mar 2024 at 12:56, Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 27.03.24 15:23, Wedson Almeida Filho wrote:
> > On Wed, 27 Mar 2024 at 05:13, Valentin Obst <kernel@valentinobst.de> wrote:
> >>
> >>> 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 [`Drop`].
> >>> -pub trait Module: Sized + Sync {
> >>> +pub trait Module: Sized + Sync + Send {
> >>
> >> This does not compile with `CONFIG_AX88796B_RUST_PHY=y || m` (or the
> >> phylib abstractions' doctests) since the module `Registration` is not
> >> `Send`.
> >
> > Thanks for the heads up. I thought I had enabled all rust code but
> > indeed I was missing this. I will fix it in v2.
> >
> >> 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 guess
> >> it would be possible to write code that is only correct under the
> >> assumption that it is loaded/unloaded in the same context.
> >
> > It might be possible in the future, but I don't believe it is now
> > because all rust modules support unloading. And there is no guarantee
> > that the thread unloading (and therefore calling module_exit) is the
> > same that loaded (and called module_init), so a module must be Send to
> > properly handle drop being called from a different thread.
> >
> > Not requiring Send on the original Module trait was an oversight that
> > I don't want to repeat in InPlaceModule.
>
> I think that this change should go to the stable tree, can you split it
> into its own patch?

Sure, I split it off in v2.

Note that you'll also need the [new] patch to `rust/kernel/net/phy.rs`.

> --
> Cheers,
> Benno
>
> >
> >>
> >> @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>
> >
> > Thanks!
> >
> >>
> >>          - Best Valentin
> >>
>

Re: [PATCH 2/2] samples: rust: add in-place initialisation sample

[Wedson Almeida Filho]

On Wed, 27 Mar 2024 at 10:53, Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 27.03.24 04:23, Wedson Almeida Filho wrote:
> > diff --git a/samples/rust/rust_inplace.rs b/samples/rust/rust_inplace.rs
> > new file mode 100644
> > index 000000000000..ba8d051cac56
> > --- /dev/null
> > +++ b/samples/rust/rust_inplace.rs
> > @@ -0,0 +1,42 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Rust minimal in-place sample.
> > +
> > +use kernel::prelude::*;
> > +
> > +module! {
> > +    type: RustInPlace,
> > +    name: "rust_inplace",
> > +    author: "Rust for Linux Contributors",
> > +    description: "Rust minimal in-place sample",
> > +    license: "GPL",
> > +}
> > +
> > +#[pin_data(PinnedDrop)]
> > +struct RustInPlace {
> > +    numbers: Vec<i32>,
> > +}
> > +
> > +impl kernel::InPlaceModule for RustInPlace {
> > +    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> > +        pr_info!("Rust minimal sample (init)\n");
>
> This text needs updating.

Fixed in v2.

>
> > +        pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
> > +        try_pin_init!(Self {
> > +            numbers: {
> > +                let mut numbers = Vec::new();
> > +                numbers.push(72, GFP_KERNEL)?;
> > +                numbers.push(108, GFP_KERNEL)?;
> > +                numbers.push(200, GFP_KERNEL)?;
> > +                numbers
> > +            },
> > +        })
>
> I think it might be useful to also have a field that needs pin-init, eg
> a `Mutex` or similar. What about placing the `Vec` inside of a mutex?

I'm not sure this belongs in a "minimal" example.

But I added it in v2 because we're already violating minimality with
vectors anyway. Perhaps we should later have minimal samples and
rename these to something else.

> --
> Cheers,
> Benno
>
> > +    }
> > +}
> > +
> > +#[pinned_drop]
> > +impl PinnedDrop for RustInPlace {
> > +    fn drop(self: Pin<&mut Self>) {
> > +        pr_info!("My numbers are {:?}\n", self.numbers);
> > +        pr_info!("Rust minimal inplace sample (exit)\n");
> > +    }
> > +}
> > --
> > 2.34.1
> >
>

Re: [PATCH 2/2] samples: rust: add in-place initialisation sample

[Benno Lossin]

On 28.03.24 14:00, Wedson Almeida Filho wrote:
> On Wed, 27 Mar 2024 at 10:53, Benno Lossin <benno.lossin@proton.me> wrote:
>> On 27.03.24 04:23, Wedson Almeida Filho wrote:
>>> +        pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
>>> +        try_pin_init!(Self {
>>> +            numbers: {
>>> +                let mut numbers = Vec::new();
>>> +                numbers.push(72, GFP_KERNEL)?;
>>> +                numbers.push(108, GFP_KERNEL)?;
>>> +                numbers.push(200, GFP_KERNEL)?;
>>> +                numbers
>>> +            },
>>> +        })
>>
>> I think it might be useful to also have a field that needs pin-init, eg
>> a `Mutex` or similar. What about placing the `Vec` inside of a mutex?
> 
> I'm not sure this belongs in a "minimal" example.
> 
> But I added it in v2 because we're already violating minimality with
> vectors anyway. Perhaps we should later have minimal samples and
> rename these to something else.

I think a fully minimal example would be less valuable as a learning
resource. We can of course have both, but I think having real usage of
pin-init in this example can help people get familiar with it.

-- 
Cheers,
Benno