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

[Boqun Feng]

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

I'm trying, but in the meanwhile, another idea comes to me, can we just
avoid using the `global_lock!` macro if "locked_by" is not needed? I.e.
as the global counter in your examples, we probably need a new lock type
in these cases but we don't use macros. I feel like macros don't bring
us anything in these usages. As a result, we make `global_lock!`
dedicated for locked_by usages, where values should usually be a `()`.

> > Besides, instead of a "guard" type name, could you make a
> > generic guard type over the "locked_by" type? E.g.
> >
> >         struct GlobalGuard<L: GlobalLockedBy>(Guard<...>, PhantomData<*mut L>);
> >
> > I feel like this could make the relationship between the guard type and
> > the locked_by type more obvious. But maybe there's something I'm
> > missing?
> 
> Sorry, I don't understand this. Why is the LockedBy type relevant to the guard?
> 

well, $locked_by's `as_ref()` and `as_mut()` both need a $guard
reference, instead of asking users to provide an arbitrary $guard name,
I thought it's better to just make $guard generic over $locked_by.
However, I realise that $locked_by is a generic type over T, so my
previous proposal doesn't work. How about another way around: making
$locked_by type generic over $guard:

	pub(crate) struct GlobalLockedBy<T: ?Sized, G>(::core::cell::UnsafeCell<T>, ...);

(G is a $guard type)

and if we do

	global_lock!{
	    static MY_LOCK ...;
	    guard: MyGuard;
	}

we can have a struct:

	struct MyStruct {
	    inner: GlobalLockedBy<u32, MyGuard>
	}

instead of asking users to provide a name of the $locked_by type.

Does this makes sense?

Regards,
Boqun

> Alice