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

[Gary Guo]

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

> From: Wedson Almeida Filho <walmeida@xxxxxxxxxxxxx>
> 
> 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@xxxxxxxxx>
> Signed-off-by: Wedson Almeida Filho <walmeida@xxxxxxxxxxxxx>
> ---
> 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.