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