Re: [PATCH v6 3/3] rust: sync: Add SpinLockIrq

[Boqun Feng <boqun.feng@gmail.com>]

On Tue, Oct 08, 2024 at 05:21:19PM +0200, Thomas Gleixner wrote:
> On Mon, Oct 07 2024 at 14:30, Lyude Paul wrote:
> > On Mon, 2024-10-07 at 14:01 +0200, Thomas Gleixner wrote:
> > So actually the new solution I suggested a little after that original email
> > wouldn't need to call local_irq_save() directly - sorry, I just explained it
> > kind of poorly and it hadn't been in my head for very long. I think you'll
> > like this solution a lot more though, lemme explain:
> >
> > Basically instead of having functions like with_interrupts_disabled, we would
> > instead introduce a new trait that can be implemented by locks with context
> > tokens: BackendWithContext:
> >
> > pub trait BackendWithContext: Backend {
> > type ContextState;
> >
> > unsafe fn lock_first(ptr: *Self::State)
> > -> (Self::Context, Self::ContextState, Self::GuardState);
> >
> > unsafe fn unlock_last(
> > ptr: *Self::State,
> > context_state: Self::ContextState,
> > guard_state: &Self::GuardState
> > );
> > }
> >
> > Where the idea is that a type like SpinlockIrq would define ContextState to be
> > a u64 (holding the flags argument from spin_lock_irqsave). lock_first() would

I would suggest we use a usize for the ContextState. And the name of
these two functions can be lock_with_context_saved()
unlock_with_context_restored(), _first() and _last() may be misleading,
because technically, you can have a nested lock_first(), e.g.

	let (c1, c1state, g) = unsafe { lock_first(...); }
	let (c2, c2state, g) = unsafe { lock_first(...); }

we will just need to make sure `unlock_last()` called in the reverse
ordering as a safety requirement. Then _first() and _last() doesn't make
sense.

Regards,
Boqun

> > use spin_lock_irqsave and create the token, unlock_last() would use
> > spin_unlock_irqrestore with the saved ContextState. Then we could use those
> > unsafe primitives to implement a method on Lock like this:
> >
> > impl<T: ?Sized, B: BackendWithContext> Lock<T, B> {
> > pub fn lock_with_new<'a>(
> > &self,
> > cb: impl FnOnce(Self::Context, &mut Guard<'a, T, B>) -> R
> > ) -> R;
> > }
> >
> > What lock_with_new would do is:
> >
> >  * call B::first_lock() (which would be spin_lock_irqsave)
> >  * call cb() with a LocalInterruptsDisabled token and a &mut to the Guard (so
> >    that the caller can't drop the lock before exiting the noirq context)
> >  * Call B::last_unlock() with the ContextState that was passed to first_lock()
> >    (which would be spin_unlock_irqrestore)
> >
> > So we'd absolutely still be modeling around the locking primitives
> > spin_lock_irqsave() and spin_unlock_irqrestore(). And subsequently we could
> > still nest lock contexts like normal. with_irqs_disabled() wouldn't be needed
> > in this arrangement - but we would still need the Interrupt tokens (which
> > would be fine since they're just for enforcing correctness anyway).
> 
> Makes sense.
> 
[...]