Message-ID: <c5af692e-56e7-427f-81d5-8b887478aff1@proton.me>
Date: Sat, 24 Aug 2024 13:15:52 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Trevor Gross <tmgross@...ch.edu>, Andreas Hindborg <nmi@...aspace.dk>
Cc: Luis Chamberlain <mcgrof@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Andreas Hindborg <a.hindborg@...sung.com>, Adam Bratschi-Kaye <ark.email@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Alice Ryhl <aliceryhl@...gle.com>, Daniel Gomez <da.gomez@...sung.com>, rust-for-linux@...r.kernel.org, linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rust: add `module_params` macro

On 24.08.24 13:27, Trevor Gross wrote:
> On Mon, Aug 19, 2024 at 8:35 AM Andreas Hindborg <nmi@...aspace.dk> wrote:
>> From: Andreas Hindborg <a.hindborg@...sung.com>
>> +                write!(
>> +                    self.param_buffer,
>> +                    "
>> +                static mut __{name}_{param_name}_value: {param_type} = {param_default};
> 
> Ah.. we need to migrate from `static mut` to `UnsafeCell` wrappers at
> some point. Since `module!` already uses `static mut`, this may need

IIRC Alice wanted to do something about that.

> to happen separately - meaning I don't think we need to block on
> making any change here.
> 
> This would mean adding an `UnsafeSyncCell` / `RacyCell` (just a
> wrapper around `UnsafeCell` that always implements `Sync`), using
> `UnsafeSyncCell<{param_type}>` as the type here, and changing from
> `static mut` to just `static`.
> 
> (I can take a look at doing this change for existing `static mut` in
> the near future).
> 
>> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
>> index efacca63c897..a65bd0233843 100644
>> --- a/scripts/Makefile.build
>> +++ b/scripts/Makefile.build
>> @@ -263,7 +263,7 @@ $(obj)/%.lst: $(obj)/%.c FORCE
>>  # Compile Rust sources (.rs)
>>  # ---------------------------------------------------------------------------
>>
>> -rust_allowed_features := new_uninit
>> +rust_allowed_features := new_uninit,const_mut_refs
> 
> We shouldn't enable `const_mut_refs`. It is indeed close to
> stabilization, but it is still kind of churny right now and we don't
> want to enable the sharp edges everywhere.
> 
> If the change from `static mut` to `UnsafeCell` that I mentioned above
> happens, `addr_of_mut!` turns into a `.get().cast::<...>()` takes the
> place of `addr_of_mut!` and doesn't require this feature (and also
> isn't unsafe).

I think this is a good idea. There might only be a problem with not
being `Sync` though... So probably need to use `SyncUnsafeCell` instead.

> If you prefer not to make that change, I think
> `addr_of!(...).cast_mut()` might be the best solution.

Won't that be resulting in the wrong provenance? I.e. the pointer won't
be allowed to write to that location?

I just checked with miri, it doesn't complain (even with
`strict-provenance`), so I guess it's fine? It feels rather wrong to me
to allow writing through a pointer obtained via `addr_of!`.

> ---
> 
> Other thought: would a wrapper type make more sense here? Something like this:
> 
> ```
> /* implementation */
> struct ModParam<T>(UnsafeCell<T>);
> 
> // `Parameter` is the existing `ModParameter` (could be
> // any name). It could be sealed.
> impl<T: Parameter> ModParam<T> {
>     #[doc(hidden)] // used in the macro
>     fn new(value: T) -> Self { ... }
> 
>     fn get(&self) -> T::Value { ... }
>     fn set(&self, v: T::Value) { ... }
> }
> ```
> 
> With usage:
> 
> ```
> module! {
>     // ...
>     // instantiated as:
>     // `static MY_PARAM: ModParam<i64> = ModParam::new(1);`

We used to do this, but it lead to problems: normally the parameter has
a lower case name, since that's the convention in the kernel. But then
pattern matching prioritises the static instead of introducing it as a
local parameter:

    let MY_PARAM = ...;

would fail, since you can't match MY_PARAM.

This is also the reason why they live in their own module.

But you can still do the modification of creating `ModParam` and using
that as the type of the static.

---
Cheers,
Benno

>     MY_PARAM: i64 {
>         default: 1,
>         description: "foo",
>     },
> }
> 
> fn foo() {
>     pr_info!("My param is {}", MY_PARAM.get());
> }
> ```
> 
> Advantages I see:
> - You bring your own name, rather than being scoped and needing to
> remember the module name
> - You can check `ModParam` in the docs to see the API, rather than
> needing to refer to source for the exact signatures of `read` and
> `write`
> - The interface gets a bit more like a mutex or atomic
> 
> - Trevor

Powered by blists - more mailing lists