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

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

"Benno Lossin" <lossin@kernel.org> writes:

> On Mon Jun 30, 2025 at 1:18 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Fri Jun 27, 2025 at 9:57 AM CEST, Andreas Hindborg wrote:
>>>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>>> 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.
>>>
>>> Sounds good, though we probably don't want to name it `Once`. Since it
>>> is something that will be populated in the future, but not by some
>>> random accessor, but rather a specific populator.
>>>
>>> So maybe:
>>>
>>>     pub struct Delayed<T> {
>>>         dummy: T,
>>>         real: Opaque<T>,
>>>         populated: Atomic<bool>, // or Atomic<Flag>
>>>         writing: Atomic<bool>, // or Atomic<Flag>
>>>     }
>>>
>>>     impl<T> Delayed<T> {
>>>         pub fn new(dummy: T) -> Self {
>>>             Self {
>>>                 dummy,
>>>                 real: Opaque::uninit(),
>>>                 populated: Atomic::new(false),
>>>                 writing: Atomic::new(false),
>>>             }
>>>         }
>>>
>>>         pub fn get(&self) -> &T {
>>>             if self.populated.load(Acquire) {
>>>                 unsafe { &*self.real.get() }
>>>             } else {
>>>                 // maybe print a warning here?
>>>                 // or maybe let the user configure this in `new()`?
>>>                 &self.dummy
>>>             }
>>>         }
>>>
>>>         pub fn populate(&self, value: T) {
>>>             if self.writing.cmpxchg(false, true, Release) {
>>>                 unsafe { *self.real.get() = value };
>>>                 self.populated.store(true, Release);
>>>             } else {
>>>                 pr_warn!("`Delayed<{}>` written to twice!\n", core::any::type_name::<T>());
>>>             }
>>>         }
>>>     }
>>>
>>> (no idea if the orderings are correct, I always have to think way to
>>> much about that... especially since our atomics seem to only take one
>>> ordering in compare_exchange?)
>>>
>>>> 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.
>>>
>>> Yeah, sorry about that :(
>>
>> Actually, perhaps we could aim at merging this code without this
>> synchronization?
>
> I won't remember this issue in a few weeks and I fear that it will just
> get buried. In fact, I already had to re-read now what the actual issue
> was...
>
>> The lack of synchronization is only a problem if we
>> support custom parsing. This patch set does not allow custom parsing
>> code, so it does not suffer this issue.
>
> ... In doing that, I saw my original example of UB:
>
>     module! {
>         // ...
>         params: {
>             my_param: i64 {
>                 default: 0,
>                 description: "",
>             },
>         },
>     }
>
>     static BAD: &'static i64 = module_parameters::my_param.get();
>
> That can happen without custom parsing, so it's still a problem...

Ah, got it. Thanks.


Best regards,
Andreas Hindborg