Re: [PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg <a.hindborg@kernel.org>]

Andreas Hindborg <a.hindborg@kernel.org> writes:

> "Benno Lossin" <lossin@kernel.org> writes:
>
>> On Mon Jun 23, 2025 at 4:31 PM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>
>>>> On Mon Jun 23, 2025 at 11:44 AM CEST, Andreas Hindborg wrote:
>>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>>
>>>>>> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>>>>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>>>>> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>>>>>>>>> +/// A wrapper for kernel parameters.
>>>>>>>>> +///
>>>>>>>>> +/// This type is instantiated by the [`module!`] macro when module parameters are
>>>>>>>>> +/// defined. You should never need to instantiate this type directly.
>>>>>>>>> +///
>>>>>>>>> +/// Note: This type is `pub` because it is used by module crates to access
>>>>>>>>> +/// parameter values.
>>>>>>>>> +#[repr(transparent)]
>>>>>>>>> +pub struct ModuleParamAccess<T> {
>>>>>>>>> +    data: core::cell::UnsafeCell<T>,
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +// SAFETY: We only create shared references to the contents of this container,
>>>>>>>>> +// so if `T` is `Sync`, so is `ModuleParamAccess`.
>>>>>>>>> +unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
>>>>>>>>> +
>>>>>>>>> +impl<T> ModuleParamAccess<T> {
>>>>>>>>> +    #[doc(hidden)]
>>>>>>>>> +    pub const fn new(value: T) -> Self {
>>>>>>>>> +        Self {
>>>>>>>>> +            data: core::cell::UnsafeCell::new(value),
>>>>>>>>> +        }
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    /// Get a shared reference to the parameter value.
>>>>>>>>> +    // Note: When sysfs access to parameters are enabled, we have to pass in a
>>>>>>>>> +    // held lock guard here.
>>>>>>>>> +    pub fn get(&self) -> &T {
>>>>>>>>> +        // SAFETY: As we only support read only parameters with no sysfs
>>>>>>>>> +        // exposure, the kernel will not touch the parameter data after module
>>>>>>>>> +        // initialization.
>>>>>>>>
>>>>>>>> This should be a type invariant. But I'm having difficulty defining one
>>>>>>>> that's actually correct: after parsing the parameter, this is written
>>>>>>>> to, but when is that actually?
>>>>>>>
>>>>>>> For built-in modules it is during kernel initialization. For loadable
>>>>>>> modules, it during module load. No code from the module will execute
>>>>>>> before parameters are set.
>>>>>>
>>>>>> Gotcha and there never ever will be custom code that is executed
>>>>>> before/during parameter setting (so code aside from code in `kernel`)?
>>>>>>
>>>>>>>> Would we eventually execute other Rust
>>>>>>>> code during that time? (for example when we allow custom parameter
>>>>>>>> parsing)
>>>>>>>
>>>>>>> I don't think we will need to synchronize because of custom parameter
>>>>>>> parsing. Parameters are initialized sequentially. It is not a problem if
>>>>>>> the custom parameter parsing code name other parameters, because they
>>>>>>> are all initialized to valid values (as they are statics).
>>>>>>
>>>>>> If you have `&'static i64`, then the value at that reference is never
>>>>>> allowed to change.
>>>>>>
>>>>>>>> This function also must never be `const` because of the following:
>>>>>>>>
>>>>>>>>     module! {
>>>>>>>>         // ...
>>>>>>>>         params: {
>>>>>>>>             my_param: i64 {
>>>>>>>>                 default: 0,
>>>>>>>>                 description: "",
>>>>>>>>             },
>>>>>>>>         },
>>>>>>>>     }
>>>>>>>>
>>>>>>>>     static BAD: &'static i64 = module_parameters::my_param.get();
>>>>>>>>
>>>>>>>> AFAIK, this static will be executed before loading module parameters and
>>>>>>>> thus it makes writing to the parameter UB.
>>>>>>>
>>>>>>> As I understand, the static will be initialized by a constant expression
>>>>>>> evaluated at compile time. I am not sure what happens when this is
>>>>>>> evaluated in const context:
>>>>>>>
>>>>>>>     pub fn get(&self) -> &T {
>>>>>>>         // SAFETY: As we only support read only parameters with no sysfs
>>>>>>>         // exposure, the kernel will not touch the parameter data after module
>>>>>>>         // initialization.
>>>>>>>         unsafe { &*self.data.get() }
>>>>>>>     }
>>>>>>>
>>>>>>> Why would that not be OK? I would assume the compiler builds a dependency graph
>>>>>>> when initializing statics?
>>>>>>
>>>>>> Yes it builds a dependency graph, but that is irrelevant? The problem is
>>>>>> that I can create a `'static` reference to the inner value *before* the
>>>>>> parameter is written-to (as the static is initialized before the
>>>>>> parameters).
>>>>>
>>>>> I see, I did not consider this situation. Thanks for pointing this out.
>>>>>
>>>>> Could we get around this without a lock maybe? If we change
>>>>> `ModuleParamAccess::get` to take a closure instead:
>>>>>
>>>>>     /// Call `func` with a reference to the parameter value stored in `Self`.
>>>>>     pub fn read(&self, func: impl FnOnce(&T)) {
>>>>>         // SAFETY: As we only support read only parameters with no sysfs
>>>>>         // exposure, the kernel will not touch the parameter data after module
>>>>>         // initialization.
>>>>>         let data = unsafe { &*self.data.get() };
>>>>>
>>>>>         func(data)
>>>>>     }
>>>>>
>>>>> I think this would bound the lifetime of the reference passed to the
>>>>> closure to the duration of the call, right?
>>>>
>>>> Yes that is correct. Now you can't assign the reference to a static.
>>>> However, this API is probably very clunky to use, since you always have
>>>> to create a closure etc.
>>>>
>>>> Since you mentioned in the other reply that one could spin up a thread
>>>> and do something simultaneously, I don't think this is enough. You could
>>>> have a loop spin over the new `read` function and read the value and
>>>> then the write happens.
>>>
>>> Yes you are right, we have to treat it as if it could be written at any
>>> point in time.
>>>
>>>> One way to fix this issue would be to use atomics to read the value and
>>>> to not create a reference to it. So essentially have
>>>>
>>>>     pub fn read(&self) -> T {
>>>>         unsafe { atomic_read_unsafe_cell(&self.data) }
>>>>     }
>>>
>>> That could work.
>>>
>>>> Another way would be to use a `Once`-like type (does that exist on the C
>>>> side?) so a type that can be initialized once and then never changes.
>>>> While it doesn't have a value set, we return some default value for the
>>>> param and print a warning, when it's set, we just return the value. But
>>>> this probably also requires atomics...
>>>
>>> I think atomic bool is not that far away. Either that, or we can lock.
>>>
>>>> Is parameter accessing used that often in hot paths? Can't you just copy
>>>> the value into your `Module` struct?
>>>
>>> I don't imagine this being read in a hot path. If so, the user could
>>> make a copy.
>>
>> That's good to know, then let's try to go for something simple.
>>
>> I don't think that we can just use a `Mutex<T>`, because we don't have a
>> way to create it at const time... I guess we could have
>>
>>     impl<T> Mutex<T>
>>         /// # Safety
>>         ///
>>         /// The returned value needs to be pinned and then `init` needs
>>         /// to be called before any other methods are called on this.
>>         pub unsafe const fn const_new() -> Self;
>>
>>         pub unsafe fn init(&self);
>>     }
>>
>> But that seems like a bad idea, because where would we call the `init`
>> function? That also needs to be synchronized...
>
> Ah, that is unfortunate. The init function will not run before this, so
> we would need a `Once` or an atomic anyway to initialize the lock.
>
> I am not sure if we are allowed to sleep during this, I would have to
> check. But then we could use a spin lock.
>
> We will need the locking anyway, when we want to enable sysfs write
> access to the parameters.
>
>>
>> Maybe we can just like you said use an atomic bool?
>
> Sigh, I will have to check how far that series has come.
>

I think I am going to build some kind of `Once` feature on top of
Boqun's atomic series [1], so that we can initialize a lock in these
statics. We can't use `global_lock!`, because that depends on module
init to initialize the lock before first use.

As far as I can tell, atomics may not land in v6.17, so this series
will probably not be ready for merge until v6.18 at the earliest.

Thanks for the input, Benno!


Best regards,
Andreas Hindborg


[1] https://lore.kernel.org/all/20250618164934.19817-1-boqun.feng@gmail.com