Message-ID: <878quytyc0.ffs@tglx>
Date: Tue, 08 Oct 2024 17:21:19 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Lyude Paul <lyude@...hat.com>, rust-for-linux@...r.kernel.org
Cc: Danilo Krummrich <dakr@...hat.com>, airlied@...hat.com, Ingo Molnar
 <mingo@...hat.com>, Will Deacon <will@...nel.org>, Waiman Long
 <longman@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
 linux-kernel@...r.kernel.org, Benno Lossin <benno.lossin@...ton.me>,
 Daniel
 Almeida <daniel.almeida@...labora.com>, Gary Guo <gary@...yguo.net>, Miguel
 Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Wedson
 Almeida Filho <wedsonaf@...il.com>, Boqun Feng <boqun.feng@...il.com>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas
 Hindborg <a.hindborg@...sung.com>, Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>, Martin Rodriguez Reboredo
 <yakoyoku@...il.com>, Valentin Obst <kernel@...entinobst.de>
Subject: Re: [PATCH v6 3/3] rust: sync: Add SpinLockIrq

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

>> The above example really should not end up in 3 guard contexts, but in
>> two by combining #1 and #2 into one. In C this looks like:
>> 
>>     scoped_guard(spinlock_irqsave)(&A) {
>>         // Allows to operate on resources which are exclusively
>>         // protected by A (DataA)
>>         
>>  scoped_guard(spinlock)(&B) {
>>            // Allows to operate on resources which are exclusively
>>            // protected by B (DataB)
>>         }
>>     }
>> 
>> Nesting B into lock A is required to keep some aspects of DataA and
>> DataB consistent. But the other parts of DataB require only B to be
>> held.
>> 
>> For extended fun lock B is not necessarily required to be acquired with
>> interrupts disabled. The fact that it nests into lock A does not make it
>> mandatory.
>> 
>> A lock is only required to be acquired with interrupts disabled if it
>> can be taken in interrupt context. That's a per lock property.
>
> I think you misunderstood something somewhere - this has always been the case
> with the bindings I submitted that you don't need a context for all locks,
> only locks that define one. That is why we reimplement lock() to look like
> this (where T is the data protected by the lock and B is the backend):
>
> pub fn lock<'a>(&'a self) -> Guard<'a, T, B>
> where
> B::Context<'a>: Default
> {
> self.lock_with(Default::default())
> }
>
> So SpinLock's B::Context is (), which implements Default - meaning you can
> acquire it simply like this:
>
> some_lock.lock();
>
> But that wouldn't work for SpinLockIrq with a context of IrqDisabled<'a>,
> since IrqDisabled doesn't implement Default.

Thanks for clarification. It's clear now.

Thanks,

        tglx

Powered by blists - more mailing lists