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

Benno Lossin
2024-10-11T07:01:19Z

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@gmail.com> 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