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

[Boqun Feng]

On Fri, Oct 11, 2024 at 07:01:09AM +0000, Benno Lossin wrote:
> 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.
> 

This only solves the "not just" part of the problem, it still confuses
the users why "value=" used to be a field but then becomes in the
initialization expression. In fact, after I take a deep look, I think we
should also move the optional "name=" field (the name for lockdep class)
to the static-item-like statement, i.e.

    global_lock!{
        // I use a new format here, which I think it's better than
	// previous proposed formats.
        static MY_LOCK: Mutex<u32> = { unsafe { uninit }, data: 0, name: "my_lock" };
    }

I think it makes more sense than making "value=" and "name=" as fields.
Think about this: we choose to use a statement in global_lock!() macro
that is similar to a static item because we want to let users know what
the macro does is similar as defining a static item, right? If so,
wouldn't it make more sense to have the initialization inputs in the
static-item-like statement instead of a separate field? This will
provide a clear picture to users that which macro parameter participates
the initialization of the static variable.

Besides, for guard/locked_by, the current format will be something like:

    global_lock!{
        static MY_LOCK: Mutex<u32> = unsafe { uninit };
	value=0;
	guard=MyGuard;
	locked_by=MyLockedBy;
    }

"value=" is the value used to initialize MY_LOCK, and "guard=" and
"locked_by=" are the types associated with the static variable, making
these three parallel in the same level doesn't really make logical sense
to me. I cannot find any reason this is better than making "value=" part
of the initialization expression.

Your argument seems to be that will be weird, however, as you mentioned
below, you don't think there will be many users, so being weird but
accurate on what are used to initialize the static variable is fine. ;-)

Plus, there is no implementation difficulty, so let do it right.

Regards,
Boqun

> > 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
>