Re: [PATCH v4] rust: add global lock support

[Benno Lossin]

On 11.10.24 01:06, Boqun Feng wrote:
> On Thu, Oct 10, 2024 at 10:21:41PM +0000, Benno Lossin wrote:
>> On 10.10.24 18:33, Boqun Feng wrote:
>>> On Thu, Oct 10, 2024 at 07:29:32AM -0700, Boqun Feng wrote:
>>>> On Thu, Oct 10, 2024 at 03:58:07PM +0200, Alice Ryhl wrote:
>>>>> On Thu, Oct 10, 2024 at 3:55 PM Boqun Feng <boqun.feng@xxxxxxxxx> wrote:
>>>>>>
>>>>>> On Thu, Oct 10, 2024 at 12:53:00PM +0200, Alice Ryhl wrote:
>>>>>> [...]
>>>>>>>>> +#[macro_export]
>>>>>>>>> +macro_rules! global_lock {
>>>>>>>>> +    {
>>>>>>>>> +        $(#[$meta:meta])* $pub:vis static $name:ident: $kind:ident<$valuety:ty> = unsafe { uninit };
>>>>>>>>> +        value: $value:expr;
>>>>>>>>
>>>>>>>> I would find it more natural to use `=` instead of `:` here, since then
>>>>>>>> it would read as a normal statement with the semicolon at the end.
>>>>>>>> Another alternative would be to use `,` instead of `;`, but that doesn't
>>>>>>>> work nicely with the static keyword above (although you could make the
>>>>>>>> user write it in another {}, but that also isn't ideal...).
>>>>>>>>
>>>>>>>> Using `=` instead of `:` makes my editor put the correct amount of
>>>>>>>> indentation there, `:` adds a lot of extra spaces.
>>>>>>>
>>>>>>> That seems sensible.
>>>>>>>
>>>>>>
>>>>>> While we are at it, how about we make the syntax:
>>>>>>
>>>>>>         global_lock!{
>>>>>>             static MY_LOCK: Mutex<u32> = unsafe { 0 };
>>>>>>         }
>>>>>>
>>>>>> or
>>>>>>
>>>>>>         global_lock!{
>>>>>>             static MY_LOCK: Mutex<u32> = unsafe { uninit { 0 } };
>>>>>>         }
>>>>>>
>>>>>> ?
>>>>>>
>>>>>> i.e. instead of a "value" field, we put it in the "initialization
>>>>>> expression". To me, this make it more clear that "value" is the
>>>>>> initialized value protected by the lock. Thoughts?
>>>>>
>>>>> `uninit { 0 }` looks pretty terrible IMO. Can we come up with something better?
>>>>>
>>>>
>>>
>>> how about:
>>>
>>>         global_lock!{
>>>             static MY_LOCK: Mutex<u32> = unsafe { data: 0 };
>>
>> I dislike this, since there is no `uninit` anywhere, but the mutex needs
>> to be initialized.
>>
>>>         }
>>>
>>> ?
>>>
>>> "data: " will make it clear that the value is not for the lock state.
>>> "uninit" is dropped because the "unsafe" already requires the global
>>> variable to be initialised first. Or "unsafe { uninit, data: 0 }" if you
>>> want to keep the "uninit" part?
>>
>> That also looks weird to me...
>>
>> But I haven't come up with a good alternative
>>
> 
> How about a "fake" MaybyUninit:
> 
> 	global_lock!{
>             static MY_LOCK: Mutex<u32> = unsafe { MaybeUninit::new(0).assume_init() };
> 	}
> 
> ?

That still suggests to the user, that the contents are initialized.

> I feel like we need to put the data in the initialization expression
> because if we resolve the initialization issues and can skip the extra
> init step, we pretty much want to use the macro like:
> 
> 	global_lock!{
>             static MY_LOCK: Mutex<u32> = { data: 0 };
> 	    // maybe even
>             // static MY_LOCK: Mutex<u32> = { 0 };
> 	}
> 
> instead of
> 
> 	global_lock!{
>             static MY_LOCK: Mutex<u32> = init;
> 	    value = 0;
> 	}
> 
> , right?
> 
> So we need to think about providing a smooth way for users to transfer.
> Not just adjust the changes (which I believe is a good practice for
> coccinelle), but also the conceptual model "oh now I don't need to
> provide a 'value=' field?". 

I think we can just use a multiline regex to find `global_lock!` and
then change the current way. It shouldn't be that difficult to change.

> Hence even though the above proposals may look weird, but I think
> that's still better?

Do you think that we will have 1000s of users by the time we change it?
I don't think so, but I don't know how often drivers use global locks. I
think we should discourage them. People still can have a "global" lock
by putting it inside of their Module struct (they need access to the
module, but maybe we should have a function that gives them the module
for the `ThisModule` pointer?)

---
Cheers,
Benno