Message-ID: <9eea4df8-09aa-4067-b408-2aa86c7f4084@proton.me>
Date: Sun, 25 Aug 2024 07:09:45 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Trevor Gross <tmgross@...ch.edu>
Cc: Andreas Hindborg <nmi@...aspace.dk>, 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 23:23, Trevor Gross wrote:
> On Sat, Aug 24, 2024 at 8:16 AM Benno Lossin <benno.lossin@...ton.me> wrote:
>>> 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.
> 
> Ah whoops, yeah that is what I meant.
> 
>>> 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!`.
> 
> I think that `static mut` gets the interior mutability rules that
> `UnsafeCell` has, that *const and *mut become interchangeable. Quick
> demo for the `UnsafeCell` at [1]. We would probably have to ask opsem
> to clarify.

I see. I suggested to Andreas to change it to the current version, seems
like I was wrong on this... I feel like the `SyncUnsafeCell` version is
still better, since we can avoid a `static mut`.

> Coincidentally I had been talking to Ralf about this very pattern
> before seeing this, at [2].
> 
>>> Other thought: would a wrapper type make more sense here? Something like this:
>>>
>>> ```
>>> /* implementation */
>>> struct ModParam<T>(UnsafeCell<T>);
>>> [...]
>>> 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. [...]
> 
> To me it seemed logical to keep the uppercase names here since it
> matches the convention for what they are (statics), and the macro
> could lowercase it for the bits exposed to the kernel. But I
> absolutely get the consistency argument here.

That would also be an option, but it might be confusing for people, they
enter a SCREAMING_CASE_SNAKE name, but when they start the kernel it
doesn't work.

>> [...] 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.
> 
> I'm not sure I follow the example here. It looks like it is shadowing
> a static's name as a local, why would you want that? Or if you meant
> the other way around `let SomePat(...) = MY_PARAM`, wouldn't it just
> be `let SomePat(...) = MY_PARAM.get()`? (Sorry if I missed some
> context here).

Yeah I expressed this poorly, the problem before was that you would
write:

    module! {
        /* ... */
        params: {
            foo: i64 {
                default: 1,
                description: "foo",
            }
        }
    }

    pub struct MyDriver {
        foo: i64,
    }

    impl Module for MyDriver {
        fn init(_: &'static ThisModule) -> Result<Self> {
            let foo = foo.read();
            //  ^^^ cannot be named the same as a static
            Ok(Self { foo })
        }
    }

>> But you can still do the modification of creating `ModParam` and using
>> that as the type of the static.
> 
> Do you mean an expansion like this?
> 
>     // module_parameters is kind of a long name
>     mod mod_params {
>         #[allow(non_upper_case_globals)]
>        static my_param: ModParam<i32> = ModParam::new(...);
>     }

Yes that's what I meant, although `my_param` should be `pub(crate)`.

> I don't mind that, even if the name is a bit weird by rust conventions.

Yeah, but I think since they are in their own module, it's fine.

One thing that we need to decide would be if we want

    mod_params::my_param::read()

or

    mod_params::my_param.read()

I slightly prefer the first one, which would mean that the expansion
would have to be:

    mod mod_params {
        pub(crate) enum my_param {}
        static my_param_value: ModParam<i32> = ModParam::new(...);
        impl my_param {
            pub(crate) fn read() {
                my_param_value.read()
            }
            /* ... */
        }
    }

> (For what it's worth, I used this wrapper type pattern for a plugin
> project that does shared variables in a similar way. I have been quite
> happy with it.)

Good to know!

---
Cheers,
Benno

> 
> - Trevor
> 
> [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=43664620f50384b7a3d5bf74ce7c3e39
> [2]: https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/More.20accidental.20stabilization

Powered by blists - more mailing lists