"Benno Lossin" <los...@kernel.org> writes:
> On Mon Jun 23, 2025 at 4:31 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <los...@kernel.org> writes:
>>
>>> On Mon Jun 23, 2025 at 11:44 AM CEST, Andreas Hindborg wrote:
>>>> "Benno Lossin" <los...@kernel.org> writes:
>>>>
>>>>> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>>>>>> "Benno Lossin" <los...@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.
Best regards,
Andreas Hindborg
