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

Martin Rodriguez Reboredo
2023-04-09T16:49:42Z

On 4/8/23 04:53, 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`.
>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
> v1 -> v2: No changes
> v2 -> v3: 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 690429561f0e..a2c654ec5bd3 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) };
> + }
> +}
> +
> 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.

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>