Received-SPF: pass (google.com: domain of linux-kernel+bounces-122933-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240327032337.188938-2-wedsonaf@gmail.com> <20240327081317.45574-1-kernel@valentinobst.de>
 <CANeycqo9noMJGSCsjCoH29xuPTJ7NEmYnFBE__9D+tvaqsiLdQ@mail.gmail.com> <nNO5IHIkY0ti-c5334QC1qNmxmomUn7FpPoq7gw8rrty90n8s5j0r0a8opXAQL2SBBU5ZI3Bni3W7p0i4jnoPZjxQqPTcmv3qYt1emMZ4kQ=@proton.me>
In-Reply-To: <nNO5IHIkY0ti-c5334QC1qNmxmomUn7FpPoq7gw8rrty90n8s5j0r0a8opXAQL2SBBU5ZI3Bni3W7p0i4jnoPZjxQqPTcmv3qYt1emMZ4kQ=@proton.me>
From: Wedson Almeida Filho <wedsonaf@gmail.com>
Date: Thu, 28 Mar 2024 09:57:52 -0300
Message-ID: <CANeycqpz59mxtbBF_0_XM6hMwGaD+RMPT_ks5XKWpeBd5MZjPQ@mail.gmail.com>
Subject: Re: [PATCH 1/2] rust: introduce `InPlaceModule`
To: Benno Lossin <benno.lossin@proton.me>
Cc: Valentin Obst <kernel@valentinobst.de>, a.hindborg@samsung.com, alex.gaynor@gmail.com, 
	aliceryhl@google.com, 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>
Content-Type: text/plain; charset="UTF-8"

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