Messages in this thread | | | Date | Sun, 9 Apr 2023 13:49:03 -0300 | From | Martin Rodriguez Reboredo <> | Subject | Re: [PATCH v3 11/13] rust: lock: add `Guard::do_unlocked` |
| |
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>
| |