[v4,11/13] rust: lock: add `Guard::do_unlocked`

  From: Wedson Almeida Filho <walmeida@microsoft.com>

It releases the lock, executes some function provided by the caller,
then reacquires the lock. This is preparation for the implementation of
condvars, which will sleep after between unlocking and relocking.

We need an explicit `relock` method for primitives like `SpinLock` that
have an irqsave variant: we use the guard state to determine if the lock
was originally acquired with the regular `lock` function or
`lock_irqsave`.

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
v1 -> v2: No changes
v2 -> v3: No changes
v3 -> v4: No changes

 rust/kernel/sync/lock.rs          | 25 +++++++++++++++++++++++++
 rust/kernel/sync/lock/spinlock.rs | 17 +++++++++++++++--
 2 files changed, 40 insertions(+), 2 deletions(-)
  

On Tue, 11 Apr 2023 02:45:41 -0300
Wedson Almeida Filho <wedsonaf@gmail.com> wrote:

> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> It releases the lock, executes some function provided by the caller,
> then reacquires the lock. This is preparation for the implementation of
> condvars, which will sleep after between unlocking and relocking.
> 
> We need an explicit `relock` method for primitives like `SpinLock` that
> have an irqsave variant: we use the guard state to determine if the lock
> was originally acquired with the regular `lock` function or
> `lock_irqsave`.
> 
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
> v1 -> v2: No changes
> v2 -> v3: No changes
> v3 -> v4: No changes
> 
>  rust/kernel/sync/lock.rs          | 25 +++++++++++++++++++++++++
>  rust/kernel/sync/lock/spinlock.rs | 17 +++++++++++++++--
>  2 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index 819b8ea5ba2b..cde57756795f 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -22,6 +22,9 @@ pub mod spinlock;
>  ///
>  /// - Implementers must ensure that only one thread/CPU may access the protected data once the lock
>  /// is owned, that is, between calls to `lock` and `unlock`.
> +/// - Implementers must also ensure that `relock` uses the same locking method as the original
> +/// lock operation. For example, it should disable interrupts if [`IrqSaveBackend::lock_irqsave`]
> +/// is used.
>  pub unsafe trait Backend {
>      /// The state required by the lock.
>      type State;
> @@ -55,6 +58,17 @@ pub unsafe trait Backend {
>      ///
>      /// It must only be called by the current owner of the lock.
>      unsafe fn unlock(ptr: *mut Self::State, guard_state: &Self::GuardState);
> +
> +    /// Reacquires the lock, making the caller its owner.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that `state` comes from a previous call to [`Backend::lock`] (or
> +    /// variant) that has been unlocked with [`Backend::unlock`] and will be relocked now.
> +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> +        // SAFETY: The safety requirements ensure that the lock is initialised.
> +        *guard_state = unsafe { Self::lock(ptr) };
> +    }
>  }
>  
>  /// The "backend" of a lock that supports the irq-save variant.
> @@ -164,6 +178,17 @@ pub struct Guard<'a, T: ?Sized, B: Backend> {
>  // SAFETY: `Guard` is sync when the data protected by the lock is also sync.
>  unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
>  
> +impl<T: ?Sized, B: Backend> Guard<'_, T, B> {
> +    #[allow(dead_code)]
> +    pub(crate) fn do_unlocked(&mut self, cb: impl FnOnce()) {
> +        // SAFETY: The caller owns the lock, so it is safe to unlock it.
> +        unsafe { B::unlock(self.lock.state.get(), &self.state) };
> +        cb();
> +        // SAFETY: The lock was just unlocked above and is being relocked now.
> +        unsafe { B::relock(self.lock.state.get(), &mut self.state) };

This should be

	let _guard = ScopeGuard::new(|| unsafe {
	    B::relock(self.lock.state.get(), &mut self.state) }
	});
	cb();

Although we currently use `-Cpanic=abort`, I think as a general rule we
should still try to make code unwind-safe, so it can remain sound if
someone takes the code and use it for userspace (e.g. for testing
purpose, or maybe sharing codebase with tools).

> +    }
> +}
> +
>  impl<T: ?Sized, B: Backend> core::ops::Deref for Guard<'_, T, B> {
>      type Target = T;
>  
> diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs
> index 34dec09a97c0..e2a2f68e6d93 100644
> --- a/rust/kernel/sync/lock/spinlock.rs
> +++ b/rust/kernel/sync/lock/spinlock.rs
> @@ -4,6 +4,7 @@
>  //!
>  //! This module allows Rust code to use the kernel's `spinlock_t`.
>  
> +use super::IrqSaveBackend;
>  use crate::bindings;
>  
>  /// Creates a [`SpinLock`] initialiser with the given name and a newly-created lock class.
> @@ -95,7 +96,8 @@ pub type SpinLock<T> = super::Lock<T, SpinLockBackend>;
>  /// A kernel `spinlock_t` lock backend.
>  pub struct SpinLockBackend;
>  
> -// SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion.
> +// SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. `relock` uses the
> +// same scheme as `unlock` to figure out which locking method was used originally.
>  unsafe impl super::Backend for SpinLockBackend {
>      type State = bindings::spinlock_t;
>      type GuardState = Option<core::ffi::c_ulong>;
> @@ -127,13 +129,24 @@ unsafe impl super::Backend for SpinLockBackend {
>              None => unsafe { bindings::spin_unlock(ptr) },
>          }
>      }
> +
> +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> +        let _ = match guard_state {
> +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> +            // initialised.
> +            None => unsafe { Self::lock(ptr) },
> +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> +            // initialised.
> +            Some(_) => unsafe { Self::lock_irqsave(ptr) },
> +        };
> +    }
>  }
>  
>  // SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. We use the `irqsave`
>  // variant of the C lock acquisition functions to disable interrupts and retrieve the original
>  // interrupt state, and the `irqrestore` variant of the lock release functions to restore the state
>  // in `unlock` -- we use the guard context to determine which method was used to acquire the lock.
> -unsafe impl super::IrqSaveBackend for SpinLockBackend {
> +unsafe impl IrqSaveBackend for SpinLockBackend {
>      unsafe fn lock_irqsave(ptr: *mut Self::State) -> Self::GuardState {
>          // SAFETY: The safety requirements of this function ensure that `ptr` points to valid
>          // memory, and that it has been initialised before.
  

On Tue, Apr 11, 2023 at 02:45:41AM -0300, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> It releases the lock, executes some function provided by the caller,
> then reacquires the lock. This is preparation for the implementation of
> condvars, which will sleep after between unlocking and relocking.
> 
> We need an explicit `relock` method for primitives like `SpinLock` that
> have an irqsave variant: we use the guard state to determine if the lock
> was originally acquired with the regular `lock` function or
> `lock_irqsave`.
> 
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
> v1 -> v2: No changes
> v2 -> v3: No changes
> v3 -> v4: No changes
> 
>  rust/kernel/sync/lock.rs          | 25 +++++++++++++++++++++++++
>  rust/kernel/sync/lock/spinlock.rs | 17 +++++++++++++++--
>  2 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index 819b8ea5ba2b..cde57756795f 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -22,6 +22,9 @@ pub mod spinlock;
>  ///
>  /// - Implementers must ensure that only one thread/CPU may access the protected data once the lock
>  /// is owned, that is, between calls to `lock` and `unlock`.
> +/// - Implementers must also ensure that `relock` uses the same locking method as the original
> +/// lock operation. For example, it should disable interrupts if [`IrqSaveBackend::lock_irqsave`]
> +/// is used.
>  pub unsafe trait Backend {
>      /// The state required by the lock.
>      type State;
> @@ -55,6 +58,17 @@ pub unsafe trait Backend {
>      ///
>      /// It must only be called by the current owner of the lock.
>      unsafe fn unlock(ptr: *mut Self::State, guard_state: &Self::GuardState);
> +
> +    /// Reacquires the lock, making the caller its owner.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that `state` comes from a previous call to [`Backend::lock`] (or

I think you mean

"Callers must ensure that `guard_state` comes ..."

, right?

Regards,
Boqun

> +    /// variant) that has been unlocked with [`Backend::unlock`] and will be relocked now.
> +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> +        // SAFETY: The safety requirements ensure that the lock is initialised.
> +        *guard_state = unsafe { Self::lock(ptr) };
> +    }
>  }
>  
>  /// The "backend" of a lock that supports the irq-save variant.
> @@ -164,6 +178,17 @@ pub struct Guard<'a, T: ?Sized, B: Backend> {
>  // SAFETY: `Guard` is sync when the data protected by the lock is also sync.
>  unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
>  
> +impl<T: ?Sized, B: Backend> Guard<'_, T, B> {
> +    #[allow(dead_code)]
> +    pub(crate) fn do_unlocked(&mut self, cb: impl FnOnce()) {
> +        // SAFETY: The caller owns the lock, so it is safe to unlock it.
> +        unsafe { B::unlock(self.lock.state.get(), &self.state) };
> +        cb();
> +        // SAFETY: The lock was just unlocked above and is being relocked now.
> +        unsafe { B::relock(self.lock.state.get(), &mut self.state) };
> +    }
> +}
> +
>  impl<T: ?Sized, B: Backend> core::ops::Deref for Guard<'_, T, B> {
>      type Target = T;
>  
> diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs
> index 34dec09a97c0..e2a2f68e6d93 100644
> --- a/rust/kernel/sync/lock/spinlock.rs
> +++ b/rust/kernel/sync/lock/spinlock.rs
> @@ -4,6 +4,7 @@
>  //!
>  //! This module allows Rust code to use the kernel's `spinlock_t`.
>  
> +use super::IrqSaveBackend;
>  use crate::bindings;
>  
>  /// Creates a [`SpinLock`] initialiser with the given name and a newly-created lock class.
> @@ -95,7 +96,8 @@ pub type SpinLock<T> = super::Lock<T, SpinLockBackend>;
>  /// A kernel `spinlock_t` lock backend.
>  pub struct SpinLockBackend;
>  
> -// SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion.
> +// SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. `relock` uses the
> +// same scheme as `unlock` to figure out which locking method was used originally.
>  unsafe impl super::Backend for SpinLockBackend {
>      type State = bindings::spinlock_t;
>      type GuardState = Option<core::ffi::c_ulong>;
> @@ -127,13 +129,24 @@ unsafe impl super::Backend for SpinLockBackend {
>              None => unsafe { bindings::spin_unlock(ptr) },
>          }
>      }
> +
> +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> +        let _ = match guard_state {
> +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> +            // initialised.
> +            None => unsafe { Self::lock(ptr) },
> +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> +            // initialised.
> +            Some(_) => unsafe { Self::lock_irqsave(ptr) },
> +        };
> +    }
>  }
>  
>  // SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. We use the `irqsave`
>  // variant of the C lock acquisition functions to disable interrupts and retrieve the original
>  // interrupt state, and the `irqrestore` variant of the lock release functions to restore the state
>  // in `unlock` -- we use the guard context to determine which method was used to acquire the lock.
> -unsafe impl super::IrqSaveBackend for SpinLockBackend {
> +unsafe impl IrqSaveBackend for SpinLockBackend {
>      unsafe fn lock_irqsave(ptr: *mut Self::State) -> Self::GuardState {
>          // SAFETY: The safety requirements of this function ensure that `ptr` points to valid
>          // memory, and that it has been initialised before.
> -- 
> 2.34.1
>
  

On Tue, Apr 11, 2023 at 02:45:41AM -0300, Wedson Almeida Filho wrote:
[...]
> +
> +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> +        let _ = match guard_state {
> +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> +            // initialised.
> +            None => unsafe { Self::lock(ptr) },
> +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> +            // initialised.
> +            Some(_) => unsafe { Self::lock_irqsave(ptr) },
> +        };
> +    }
>  }
>  

One thing I'm little worried about the above is that we don't store back
the new GuardState into `guard_state`, the particular case I'm worried
about is as follow:

	// IRQ is enabled.
	// Disabling IRQ
	unsafe { bindings::local_irq_disable(); }

	let mut g = unsafe { SpinLockBackend::lock(&mut lock as *mut _) };
	// `g` records irq state is "irq disabled"

	unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
	// restore into "irq disabled" mode.
	// IRQ is disabled.

	// Enabling IRQ
	unsafe { bindings::local_irq_enable(); }
	// IRQ is enabled.

	unsafe { SpinLockBackend::relock(&mut lock as *mut _, &mut g) }
	// `g` still records irq state is "irq disabled"

	unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
	// restore into "irq disabled" mode.
	// IRQ is disabled.


This looks pretty scary to me, I would expect `relock()` updates the
latest GuardState to the guard. Any reason it's implemented this way?

Regards,
Boqun

>  // SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. We use the `irqsave`
>  // variant of the C lock acquisition functions to disable interrupts and retrieve the original
>  // interrupt state, and the `irqrestore` variant of the lock release functions to restore the state
>  // in `unlock` -- we use the guard context to determine which method was used to acquire the lock.
> -unsafe impl super::IrqSaveBackend for SpinLockBackend {
> +unsafe impl IrqSaveBackend for SpinLockBackend {
>      unsafe fn lock_irqsave(ptr: *mut Self::State) -> Self::GuardState {
>          // SAFETY: The safety requirements of this function ensure that `ptr` points to valid
>          // memory, and that it has been initialised before.
> -- 
> 2.34.1
>
  

On Wed, 12 Apr 2023 at 03:25, Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Tue, Apr 11, 2023 at 02:45:41AM -0300, Wedson Almeida Filho wrote:
> [...]
> > +
> > +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> > +        let _ = match guard_state {
> > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > +            // initialised.
> > +            None => unsafe { Self::lock(ptr) },
> > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > +            // initialised.
> > +            Some(_) => unsafe { Self::lock_irqsave(ptr) },
> > +        };
> > +    }
> >  }
> >
>
> One thing I'm little worried about the above is that we don't store back
> the new GuardState into `guard_state`, the particular case I'm worried
> about is as follow:
>
>         // IRQ is enabled.
>         // Disabling IRQ
>         unsafe { bindings::local_irq_disable(); }
>
>         let mut g = unsafe { SpinLockBackend::lock(&mut lock as *mut _) };
>         // `g` records irq state is "irq disabled"
>
>         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
>         // restore into "irq disabled" mode.
>         // IRQ is disabled.
>
>         // Enabling IRQ
>         unsafe { bindings::local_irq_enable(); }
>         // IRQ is enabled.
>
>         unsafe { SpinLockBackend::relock(&mut lock as *mut _, &mut g) }
>         // `g` still records irq state is "irq disabled"

Yes, that's by design. If you want it to record the new "irq enabled"
state, then you should call `lock()`, not `relock()`.

>         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
>         // restore into "irq disabled" mode.
>         // IRQ is disabled.
>
>
> This looks pretty scary to me, I would expect `relock()` updates the
> latest GuardState to the guard. Any reason it's implemented this way?

A `relock()` followed by an `unlock()` takes the state back to how it
was when `lock()` was originally called: this is precisely why
`relock()` exists.

Consider the following case:

```
local_disable_irq();
let mut guard = spinlock.lock();

guard.do_unlocked(|| {
    local_irq_enable();
    schedule();
});

drop(guard);
```

What would you expect the state to be? It's meant to be the state
right before `spinlock.lock()` was called, that's what the guard
represents.

If you want to preserve a new state, then you don't want `relock()`,
you just want a new `lock()` call.

> Regards,
> Boqun
>
> >  // SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. We use the `irqsave`
> >  // variant of the C lock acquisition functions to disable interrupts and retrieve the original
> >  // interrupt state, and the `irqrestore` variant of the lock release functions to restore the state
> >  // in `unlock` -- we use the guard context to determine which method was used to acquire the lock.
> > -unsafe impl super::IrqSaveBackend for SpinLockBackend {
> > +unsafe impl IrqSaveBackend for SpinLockBackend {
> >      unsafe fn lock_irqsave(ptr: *mut Self::State) -> Self::GuardState {
> >          // SAFETY: The safety requirements of this function ensure that `ptr` points to valid
> >          // memory, and that it has been initialised before.
> > --
> > 2.34.1
> >
  

On Tue, 11 Apr 2023 at 18:18, Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Tue, Apr 11, 2023 at 02:45:41AM -0300, Wedson Almeida Filho wrote:
> > From: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > It releases the lock, executes some function provided by the caller,
> > then reacquires the lock. This is preparation for the implementation of
> > condvars, which will sleep after between unlocking and relocking.
> >
> > We need an explicit `relock` method for primitives like `SpinLock` that
> > have an irqsave variant: we use the guard state to determine if the lock
> > was originally acquired with the regular `lock` function or
> > `lock_irqsave`.
> >
> > Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> > ---
> > v1 -> v2: No changes
> > v2 -> v3: No changes
> > v3 -> v4: No changes
> >
> >  rust/kernel/sync/lock.rs          | 25 +++++++++++++++++++++++++
> >  rust/kernel/sync/lock/spinlock.rs | 17 +++++++++++++++--
> >  2 files changed, 40 insertions(+), 2 deletions(-)
> >
> > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> > index 819b8ea5ba2b..cde57756795f 100644
> > --- a/rust/kernel/sync/lock.rs
> > +++ b/rust/kernel/sync/lock.rs
> > @@ -22,6 +22,9 @@ pub mod spinlock;
> >  ///
> >  /// - Implementers must ensure that only one thread/CPU may access the protected data once the lock
> >  /// is owned, that is, between calls to `lock` and `unlock`.
> > +/// - Implementers must also ensure that `relock` uses the same locking method as the original
> > +/// lock operation. For example, it should disable interrupts if [`IrqSaveBackend::lock_irqsave`]
> > +/// is used.
> >  pub unsafe trait Backend {
> >      /// The state required by the lock.
> >      type State;
> > @@ -55,6 +58,17 @@ pub unsafe trait Backend {
> >      ///
> >      /// It must only be called by the current owner of the lock.
> >      unsafe fn unlock(ptr: *mut Self::State, guard_state: &Self::GuardState);
> > +
> > +    /// Reacquires the lock, making the caller its owner.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that `state` comes from a previous call to [`Backend::lock`] (or
>
> I think you mean
>
> "Callers must ensure that `guard_state` comes ..."
>
> , right?

That's right, thanks for spotting this!

I renamed the parameter names (based on your feedback :)) but didn't
update the comment accordingly. Will do.

> Regards,
> Boqun
>
> > +    /// variant) that has been unlocked with [`Backend::unlock`] and will be relocked now.
> > +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> > +        // SAFETY: The safety requirements ensure that the lock is initialised.
> > +        *guard_state = unsafe { Self::lock(ptr) };
> > +    }
> >  }
> >
> >  /// The "backend" of a lock that supports the irq-save variant.
> > @@ -164,6 +178,17 @@ pub struct Guard<'a, T: ?Sized, B: Backend> {
> >  // SAFETY: `Guard` is sync when the data protected by the lock is also sync.
> >  unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
> >
> > +impl<T: ?Sized, B: Backend> Guard<'_, T, B> {
> > +    #[allow(dead_code)]
> > +    pub(crate) fn do_unlocked(&mut self, cb: impl FnOnce()) {
> > +        // SAFETY: The caller owns the lock, so it is safe to unlock it.
> > +        unsafe { B::unlock(self.lock.state.get(), &self.state) };
> > +        cb();
> > +        // SAFETY: The lock was just unlocked above and is being relocked now.
> > +        unsafe { B::relock(self.lock.state.get(), &mut self.state) };
> > +    }
> > +}
> > +
> >  impl<T: ?Sized, B: Backend> core::ops::Deref for Guard<'_, T, B> {
> >      type Target = T;
> >
> > diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs
> > index 34dec09a97c0..e2a2f68e6d93 100644
> > --- a/rust/kernel/sync/lock/spinlock.rs
> > +++ b/rust/kernel/sync/lock/spinlock.rs
> > @@ -4,6 +4,7 @@
> >  //!
> >  //! This module allows Rust code to use the kernel's `spinlock_t`.
> >
> > +use super::IrqSaveBackend;
> >  use crate::bindings;
> >
> >  /// Creates a [`SpinLock`] initialiser with the given name and a newly-created lock class.
> > @@ -95,7 +96,8 @@ pub type SpinLock<T> = super::Lock<T, SpinLockBackend>;
> >  /// A kernel `spinlock_t` lock backend.
> >  pub struct SpinLockBackend;
> >
> > -// SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion.
> > +// SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. `relock` uses the
> > +// same scheme as `unlock` to figure out which locking method was used originally.
> >  unsafe impl super::Backend for SpinLockBackend {
> >      type State = bindings::spinlock_t;
> >      type GuardState = Option<core::ffi::c_ulong>;
> > @@ -127,13 +129,24 @@ unsafe impl super::Backend for SpinLockBackend {
> >              None => unsafe { bindings::spin_unlock(ptr) },
> >          }
> >      }
> > +
> > +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> > +        let _ = match guard_state {
> > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > +            // initialised.
> > +            None => unsafe { Self::lock(ptr) },
> > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > +            // initialised.
> > +            Some(_) => unsafe { Self::lock_irqsave(ptr) },
> > +        };
> > +    }
> >  }
> >
> >  // SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. We use the `irqsave`
> >  // variant of the C lock acquisition functions to disable interrupts and retrieve the original
> >  // interrupt state, and the `irqrestore` variant of the lock release functions to restore the state
> >  // in `unlock` -- we use the guard context to determine which method was used to acquire the lock.
> > -unsafe impl super::IrqSaveBackend for SpinLockBackend {
> > +unsafe impl IrqSaveBackend for SpinLockBackend {
> >      unsafe fn lock_irqsave(ptr: *mut Self::State) -> Self::GuardState {
> >          // SAFETY: The safety requirements of this function ensure that `ptr` points to valid
> >          // memory, and that it has been initialised before.
> > --
> > 2.34.1
> >
  

On Tue, 11 Apr 2023 at 17:54, Gary Guo <gary@garyguo.net> wrote:
>
> On Tue, 11 Apr 2023 02:45:41 -0300
> Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> > From: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > It releases the lock, executes some function provided by the caller,
> > then reacquires the lock. This is preparation for the implementation of
> > condvars, which will sleep after between unlocking and relocking.
> >
> > We need an explicit `relock` method for primitives like `SpinLock` that
> > have an irqsave variant: we use the guard state to determine if the lock
> > was originally acquired with the regular `lock` function or
> > `lock_irqsave`.
> >
> > Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> > ---
> > v1 -> v2: No changes
> > v2 -> v3: No changes
> > v3 -> v4: No changes
> >
> >  rust/kernel/sync/lock.rs          | 25 +++++++++++++++++++++++++
> >  rust/kernel/sync/lock/spinlock.rs | 17 +++++++++++++++--
> >  2 files changed, 40 insertions(+), 2 deletions(-)
> >
> > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> > index 819b8ea5ba2b..cde57756795f 100644
> > --- a/rust/kernel/sync/lock.rs
> > +++ b/rust/kernel/sync/lock.rs
> > @@ -22,6 +22,9 @@ pub mod spinlock;
> >  ///
> >  /// - Implementers must ensure that only one thread/CPU may access the protected data once the lock
> >  /// is owned, that is, between calls to `lock` and `unlock`.
> > +/// - Implementers must also ensure that `relock` uses the same locking method as the original
> > +/// lock operation. For example, it should disable interrupts if [`IrqSaveBackend::lock_irqsave`]
> > +/// is used.
> >  pub unsafe trait Backend {
> >      /// The state required by the lock.
> >      type State;
> > @@ -55,6 +58,17 @@ pub unsafe trait Backend {
> >      ///
> >      /// It must only be called by the current owner of the lock.
> >      unsafe fn unlock(ptr: *mut Self::State, guard_state: &Self::GuardState);
> > +
> > +    /// Reacquires the lock, making the caller its owner.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that `state` comes from a previous call to [`Backend::lock`] (or
> > +    /// variant) that has been unlocked with [`Backend::unlock`] and will be relocked now.
> > +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> > +        // SAFETY: The safety requirements ensure that the lock is initialised.
> > +        *guard_state = unsafe { Self::lock(ptr) };
> > +    }
> >  }
> >
> >  /// The "backend" of a lock that supports the irq-save variant.
> > @@ -164,6 +178,17 @@ pub struct Guard<'a, T: ?Sized, B: Backend> {
> >  // SAFETY: `Guard` is sync when the data protected by the lock is also sync.
> >  unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
> >
> > +impl<T: ?Sized, B: Backend> Guard<'_, T, B> {
> > +    #[allow(dead_code)]
> > +    pub(crate) fn do_unlocked(&mut self, cb: impl FnOnce()) {
> > +        // SAFETY: The caller owns the lock, so it is safe to unlock it.
> > +        unsafe { B::unlock(self.lock.state.get(), &self.state) };
> > +        cb();
> > +        // SAFETY: The lock was just unlocked above and is being relocked now.
> > +        unsafe { B::relock(self.lock.state.get(), &mut self.state) };
>
> This should be
>
>         let _guard = ScopeGuard::new(|| unsafe {
>             B::relock(self.lock.state.get(), &mut self.state) }
>         });
>         cb();
>
> Although we currently use `-Cpanic=abort`, I think as a general rule we
> should still try to make code unwind-safe, so it can remain sound if
> someone takes the code and use it for userspace (e.g. for testing
> purpose, or maybe sharing codebase with tools).

Good point. Although this has not been something we cared about in the
last couple of years because we abort, I think we should carefully
review code for this as we upstream.

It is also important for async scenarios: we need to go back to a
consistent state when we tear down `Future` instances.

> > +    }
> > +}
> > +
> >  impl<T: ?Sized, B: Backend> core::ops::Deref for Guard<'_, T, B> {
> >      type Target = T;
> >
> > diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs
> > index 34dec09a97c0..e2a2f68e6d93 100644
> > --- a/rust/kernel/sync/lock/spinlock.rs
> > +++ b/rust/kernel/sync/lock/spinlock.rs
> > @@ -4,6 +4,7 @@
> >  //!
> >  //! This module allows Rust code to use the kernel's `spinlock_t`.
> >
> > +use super::IrqSaveBackend;
> >  use crate::bindings;
> >
> >  /// Creates a [`SpinLock`] initialiser with the given name and a newly-created lock class.
> > @@ -95,7 +96,8 @@ pub type SpinLock<T> = super::Lock<T, SpinLockBackend>;
> >  /// A kernel `spinlock_t` lock backend.
> >  pub struct SpinLockBackend;
> >
> > -// SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion.
> > +// SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. `relock` uses the
> > +// same scheme as `unlock` to figure out which locking method was used originally.
> >  unsafe impl super::Backend for SpinLockBackend {
> >      type State = bindings::spinlock_t;
> >      type GuardState = Option<core::ffi::c_ulong>;
> > @@ -127,13 +129,24 @@ unsafe impl super::Backend for SpinLockBackend {
> >              None => unsafe { bindings::spin_unlock(ptr) },
> >          }
> >      }
> > +
> > +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> > +        let _ = match guard_state {
> > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > +            // initialised.
> > +            None => unsafe { Self::lock(ptr) },
> > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > +            // initialised.
> > +            Some(_) => unsafe { Self::lock_irqsave(ptr) },
> > +        };
> > +    }
> >  }
> >
> >  // SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. We use the `irqsave`
> >  // variant of the C lock acquisition functions to disable interrupts and retrieve the original
> >  // interrupt state, and the `irqrestore` variant of the lock release functions to restore the state
> >  // in `unlock` -- we use the guard context to determine which method was used to acquire the lock.
> > -unsafe impl super::IrqSaveBackend for SpinLockBackend {
> > +unsafe impl IrqSaveBackend for SpinLockBackend {
> >      unsafe fn lock_irqsave(ptr: *mut Self::State) -> Self::GuardState {
> >          // SAFETY: The safety requirements of this function ensure that `ptr` points to valid
> >          // memory, and that it has been initialised before.
>
  

On Wed, Apr 12, 2023 at 08:07:40AM -0300, Wedson Almeida Filho wrote:
> On Wed, 12 Apr 2023 at 03:25, Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Tue, Apr 11, 2023 at 02:45:41AM -0300, Wedson Almeida Filho wrote:
> > [...]
> > > +
> > > +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> > > +        let _ = match guard_state {
> > > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > > +            // initialised.
> > > +            None => unsafe { Self::lock(ptr) },
> > > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > > +            // initialised.
> > > +            Some(_) => unsafe { Self::lock_irqsave(ptr) },
> > > +        };
> > > +    }
> > >  }
> > >
> >
> > One thing I'm little worried about the above is that we don't store back
> > the new GuardState into `guard_state`, the particular case I'm worried
> > about is as follow:
> >
> >         // IRQ is enabled.
> >         // Disabling IRQ
> >         unsafe { bindings::local_irq_disable(); }
> >
> >         let mut g = unsafe { SpinLockBackend::lock(&mut lock as *mut _) };
> >         // `g` records irq state is "irq disabled"
> >
> >         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
> >         // restore into "irq disabled" mode.
> >         // IRQ is disabled.
> >
> >         // Enabling IRQ
> >         unsafe { bindings::local_irq_enable(); }
> >         // IRQ is enabled.
> >
> >         unsafe { SpinLockBackend::relock(&mut lock as *mut _, &mut g) }
> >         // `g` still records irq state is "irq disabled"
> 
> Yes, that's by design. If you want it to record the new "irq enabled"
> state, then you should call `lock()`, not `relock()`.
> 
> >         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
> >         // restore into "irq disabled" mode.
> >         // IRQ is disabled.
> >
> >
> > This looks pretty scary to me, I would expect `relock()` updates the
> > latest GuardState to the guard. Any reason it's implemented this way?
> 
> A `relock()` followed by an `unlock()` takes the state back to how it
> was when `lock()` was originally called: this is precisely why
> `relock()` exists.
> 
> Consider the following case:
> 
> ```
> local_disable_irq();
> let mut guard = spinlock.lock();

I think you meant `spinlock.lock_irqsave()` here, right?

> 
> guard.do_unlocked(|| {
>     local_irq_enable();
>     schedule();
> });
> 
> drop(guard);
> ```
> 
> What would you expect the state to be? It's meant to be the state

I understand your point but I would expect people to code like:

```
local_disable_irq();
let mut guard = spinlock.lock(); // or lock_irqsave(), doesn't matter

guard.do_unlocked(|| {
    local_irq_enable();
    schedule();
    local_irq_disable();
});

drop(guard);
```

And the closure in do_unlocked() can also be something like:
```
	guard.do_unlocked(|| {
	    local_irq_enabled();
	    let _g = ScopeGuard::new(|| {
	        local_irq_disabled();
	    });

	    schedule();

	    if (some_cond) {
	    	return; // return early
	    }

	    if (other_cond) {
	    	return;
	    }
	})

```

One benefit (other that code looks symmetric) is we can use the same
closure in other place. Also it helps klint since we keep the irq
enablement state change as local as possible: we can go ahead and
require irq enabled state should not be changed between the closure in
do_unlock().

Maybe I'm missing something, but the current `relock` semantics is
really tricky to get ;-)

Regards,
Boqun

> right before `spinlock.lock()` was called, that's what the guard
> represents.
> 
> If you want to preserve a new state, then you don't want `relock()`,
> you just want a new `lock()` call.
> 
> > Regards,
> > Boqun
> >
> > >  // SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. We use the `irqsave`
> > >  // variant of the C lock acquisition functions to disable interrupts and retrieve the original
> > >  // interrupt state, and the `irqrestore` variant of the lock release functions to restore the state
> > >  // in `unlock` -- we use the guard context to determine which method was used to acquire the lock.
> > > -unsafe impl super::IrqSaveBackend for SpinLockBackend {
> > > +unsafe impl IrqSaveBackend for SpinLockBackend {
> > >      unsafe fn lock_irqsave(ptr: *mut Self::State) -> Self::GuardState {
> > >          // SAFETY: The safety requirements of this function ensure that `ptr` points to valid
> > >          // memory, and that it has been initialised before.
> > > --
> > > 2.34.1
> > >
  

On Wed, 12 Apr 2023 at 11:35, Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Wed, Apr 12, 2023 at 08:07:40AM -0300, Wedson Almeida Filho wrote:
> > On Wed, 12 Apr 2023 at 03:25, Boqun Feng <boqun.feng@gmail.com> wrote:
> > >
> > > On Tue, Apr 11, 2023 at 02:45:41AM -0300, Wedson Almeida Filho wrote:
> > > [...]
> > > > +
> > > > +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> > > > +        let _ = match guard_state {
> > > > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > > > +            // initialised.
> > > > +            None => unsafe { Self::lock(ptr) },
> > > > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > > > +            // initialised.
> > > > +            Some(_) => unsafe { Self::lock_irqsave(ptr) },
> > > > +        };
> > > > +    }
> > > >  }
> > > >
> > >
> > > One thing I'm little worried about the above is that we don't store back
> > > the new GuardState into `guard_state`, the particular case I'm worried
> > > about is as follow:
> > >
> > >         // IRQ is enabled.
> > >         // Disabling IRQ
> > >         unsafe { bindings::local_irq_disable(); }
> > >
> > >         let mut g = unsafe { SpinLockBackend::lock(&mut lock as *mut _) };
> > >         // `g` records irq state is "irq disabled"
> > >
> > >         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
> > >         // restore into "irq disabled" mode.
> > >         // IRQ is disabled.
> > >
> > >         // Enabling IRQ
> > >         unsafe { bindings::local_irq_enable(); }
> > >         // IRQ is enabled.
> > >
> > >         unsafe { SpinLockBackend::relock(&mut lock as *mut _, &mut g) }
> > >         // `g` still records irq state is "irq disabled"
> >
> > Yes, that's by design. If you want it to record the new "irq enabled"
> > state, then you should call `lock()`, not `relock()`.
> >
> > >         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
> > >         // restore into "irq disabled" mode.
> > >         // IRQ is disabled.
> > >
> > >
> > > This looks pretty scary to me, I would expect `relock()` updates the
> > > latest GuardState to the guard. Any reason it's implemented this way?
> >
> > A `relock()` followed by an `unlock()` takes the state back to how it
> > was when `lock()` was originally called: this is precisely why
> > `relock()` exists.
> >
> > Consider the following case:
> >
> > ```
> > local_disable_irq();
> > let mut guard = spinlock.lock();
>
> I think you meant `spinlock.lock_irqsave()` here, right?

Yes, sorry, I meant `lock_irqsave()`.

> >
> > guard.do_unlocked(|| {
> >     local_irq_enable();
> >     schedule();
> > });
> >
> > drop(guard);
> > ```
> >
> > What would you expect the state to be? It's meant to be the state
>
> I understand your point but I would expect people to code like:
>
> ```
> local_disable_irq();
> let mut guard = spinlock.lock(); // or lock_irqsave(), doesn't matter
>
> guard.do_unlocked(|| {
>     local_irq_enable();
>     schedule();
>     local_irq_disable();
> });
>
> drop(guard);
> ```

Well, `relock` works with the code above as well.

> And the closure in do_unlocked() can also be something like:
> ```
>         guard.do_unlocked(|| {
>             local_irq_enabled();
>             let _g = ScopeGuard::new(|| {
>                 local_irq_disabled();
>             });
>
>             schedule();
>
>             if (some_cond) {
>                 return; // return early
>             }
>
>             if (other_cond) {
>                 return;
>             }
>         })
>
> ```
>
> One benefit (other that code looks symmetric) is we can use the same
> closure in other place. Also it helps klint since we keep the irq
> enablement state change as local as possible: we can go ahead and
> require irq enabled state should not be changed between the closure in
> do_unlock().

Note that the only user of `do_unlocked` at the moment works for any
type of lock, including mutexes, so we can't really have this kind of
code there. All irq handling needs to happen on the backends.

> Maybe I'm missing something, but the current `relock` semantics is
> really tricky to get ;-)

It seems straightforward to me: reacquire the lock in the same mode as
the original lock() call (e.g., lock() vs lock_irqsave()) such that
the `unlock()` will restore any state it manages to what it was right
before the original locking call.

But callers are not going to deal with these unsafe backend functions
directly, they'll deal with guards, so the high-level requirement that
`relock()` enforces is the following, given something like:

```
// 1
let guard = spinlock.lock_irqsave();
// Some code, which may include calls to a condvar, and may change the
irq state.
drop(guard);
// 2
```

At 2, the irq state must be the same as in 1, that's why one would use
the `lock_irqsave` variant.

This is a common pattern (seen in a bunch of unrelated places): save
the current state, make changes to the state, and eventually restore
the saved state (independently of what changes were made between
saving and restoring).

> Regards,
> Boqun
>
> > right before `spinlock.lock()` was called, that's what the guard
> > represents.
> >
> > If you want to preserve a new state, then you don't want `relock()`,
> > you just want a new `lock()` call.
> >
> > > Regards,
> > > Boqun
> > >
> > > >  // SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. We use the `irqsave`
> > > >  // variant of the C lock acquisition functions to disable interrupts and retrieve the original
> > > >  // interrupt state, and the `irqrestore` variant of the lock release functions to restore the state
> > > >  // in `unlock` -- we use the guard context to determine which method was used to acquire the lock.
> > > > -unsafe impl super::IrqSaveBackend for SpinLockBackend {
> > > > +unsafe impl IrqSaveBackend for SpinLockBackend {
> > > >      unsafe fn lock_irqsave(ptr: *mut Self::State) -> Self::GuardState {
> > > >          // SAFETY: The safety requirements of this function ensure that `ptr` points to valid
> > > >          // memory, and that it has been initialised before.
> > > > --
> > > > 2.34.1
> > > >