"Benno Lossin" <lossin@xxxxxxxxxx> writes:
> On Mon Jun 30, 2025 at 1:18 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@xxxxxxxxxx> writes:
>>> On Fri Jun 27, 2025 at 9:57 AM CEST, Andreas Hindborg wrote:
>>>> Andreas Hindborg <a.hindborg@xxxxxxxxxx> writes:
>>>>> "Benno Lossin" <lossin@xxxxxxxxxx> 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
