Messages in this thread | | | From | Wedson Almeida Filho <> | Date | Wed, 12 Apr 2023 08:16:34 -0300 | Subject | Re: [PATCH v4 11/13] rust: lock: add `Guard::do_unlocked` |
| |
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. >
| |