Messages in this thread | | | Date | Thu, 13 Apr 2023 08:46:25 +0000 | From | Benno Lossin <> | Subject | Re: [PATCH v4 02/13] rust: sync: introduce `Lock` and `Guard` |
| |
On 11.04.23 07:45, Wedson Almeida Filho wrote: > From: Wedson Almeida Filho <walmeida@microsoft.com> > > They are generic Rust implementations of a lock and a lock guard that > contain code that is common to all locks. Different backends will be > introduced in subsequent commits. > > Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> > Suggested-by: Gary Guo <gary@garyguo.net> > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com> > --- > v1 -> v2: No changes > v2 -> v3: Use new Opaque::ffi_init from Benno's series > v3 -> v4: Fixed name of parameter in Lock comment > > rust/kernel/sync.rs | 2 +- > rust/kernel/sync/lock.rs | 162 +++++++++++++++++++++++++++++++++++++++ > 2 files changed, 163 insertions(+), 1 deletion(-) > create mode 100644 rust/kernel/sync/lock.rs > > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs > index 541d235ffbeb..81b0998eaa18 100644 > --- a/rust/kernel/sync.rs > +++ b/rust/kernel/sync.rs > @@ -8,6 +8,7 @@ > use crate::types::Opaque; > > mod arc; > +pub mod lock; > > pub use arc::{Arc, ArcBorrow, UniqueArc}; > > @@ -25,7 +26,6 @@ impl LockClassKey { > Self(Opaque::uninit()) > } > > - #[allow(dead_code)] > pub(crate) fn as_ptr(&self) -> *mut bindings::lock_class_key { > self.0.get() > } > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs > new file mode 100644 > index 000000000000..1a8ecccf4f24 > --- /dev/null > +++ b/rust/kernel/sync/lock.rs > @@ -0,0 +1,162 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Generic kernel lock and guard. > +//! > +//! It contains a generic Rust lock and guard that allow for different backends (e.g., mutexes, > +//! spinlocks, raw spinlocks) to be provided with minimal effort. > + > +use super::LockClassKey; > +use crate::{bindings, init::PinInit, pin_init, str::CStr, types::Opaque}; > +use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned}; > +use macros::pin_data; > + > +/// The "backend" of a lock. > +/// > +/// It is the actual implementation of the lock, without the need to repeat patterns used in all > +/// locks. > +/// > +/// # Safety > +/// > +/// - 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`.
I am a bit confused which data is protected. The `Backend` should not be aware of any data it protects. To me the safety requirement is: - Up to one thread may be in the critical section at any one time. - When `lock` returns, the current thread has entered the critical section. - A thread leaves the critical section by calling `unlock`.
> +pub unsafe trait Backend { > + /// The state required by the lock. > + type State; > + > + /// The state required to be kept between lock and unlock. > + type GuardState; > + > + /// Initialises the lock. > + /// > + /// # Safety > + /// > + /// `ptr` must be valid for write for the duration of the call, while `name` and `key` must > + /// remain valid for read indefinitely.
- Only call this function once on uninitialized memory.
Do we require some sort of drop guarantee like pin? This function does not state anything about `State` having to be pinned, but `Lock` is `!Unpin` (and also pins the state). If other people decide to use `Backend` somewhere, these things should be clear.
> + unsafe fn init( > + ptr: *mut Self::State, > + name: *const core::ffi::c_char, > + key: *mut bindings::lock_class_key, > + ); > + > + /// Acquires the lock, making the caller its owner. > + /// > + /// # Safety > + /// > + /// Callers must ensure that [`Backend::init`] has been previously called.
Is it safe to call this function twice? Does `unlock` have to be called, or is one allowed to drop the value before?
> + #[must_use] > + unsafe fn lock(ptr: *mut Self::State) -> Self::GuardState; > + > + /// Releases the lock, giving up its ownership. > + /// > + /// # Safety > + /// > + /// It must only be called by the current owner of the lock. > + unsafe fn unlock(ptr: *mut Self::State, guard_state: &Self::GuardState); > +} > + > +/// A mutual exclusion primitive. > +/// > +/// Exposes one of the kernel locking primitives. Which one is exposed depends on the lock banckend
Typo: 'banckend' -> '[`Backend`]' (I would prefer a link here)
> +/// specified as the generic parameter `B`. > +#[pin_data] > +pub struct Lock<T: ?Sized, B: Backend> { > + /// The kernel lock object. > + #[pin] > + state: Opaque<B::State>, > + > + /// Some locks are known to be self-referential (e.g., mutexes), while others are architecture > + /// or config defined (e.g., spinlocks). So we conservatively require them to be pinned in case > + /// some architecture uses self-references now or in the future. > + #[pin] > + _pin: PhantomPinned, > + > + /// The data protected by the lock. > + data: UnsafeCell<T>, > +} > + > +// SAFETY: `Lock` can be transferred across thread boundaries iff the data it protects can.
Are there backends where `Backend::State: !Send`? If so we should add this as a bound, if not, there should be a comment here, since it is unconditionally implemented. Same for `Sync`
> +unsafe impl<T: ?Sized + Send, B: Backend> Send for Lock<T, B> {} > + > +// SAFETY: `Lock` serialises the interior mutability it provides, so it is `Sync` as long as the > +// data it protects is `Send`. > +unsafe impl<T: ?Sized + Send, B: Backend> Sync for Lock<T, B> {} > + > +impl<T, B: Backend> Lock<T, B> { > + /// Constructs a new lock initialiser. > + #[allow(clippy::new_ret_no_self)] > + pub fn new(t: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> { > + pin_init!(Self { > + data: UnsafeCell::new(t), > + _pin: PhantomPinned, > + // SAFETY: `slot` is valid while the closure is called and both `name` and `key` have > + // static lifetimes so they live indefinitely. > + state <- Opaque::ffi_init(|slot| unsafe { > + B::init(slot, name.as_char_ptr(), key.as_ptr()) > + }), > + }) > + } > +} > + > +impl<T: ?Sized, B: Backend> Lock<T, B> { > + /// Acquires the lock and gives the caller access to the data protected by it. > + pub fn lock(&self) -> Guard<'_, T, B> { > + // SAFETY: The constructor of the type calls `init`, so the existence of the object proves
I would make 'the type' concrete here: 'The constructor of `Self` ...'. And then '... so the existence of `self` ...'.
> + // that `init` was called. > + let state = unsafe { B::lock(self.state.get()) }; > + // SAFETY: The lock was just acquired. > + unsafe { Guard::new(self, state) } > + } > +} > + > +/// A lock guard. > +/// > +/// Allows mutual exclusion primitives that implement the `Backend` trait to automatically unlock
Make `Backend` a link.
> +/// when a guard goes out of scope. It also provides a safe and convenient way to access the data > +/// protected by the lock. > +#[must_use = "the lock unlocks immediately when the guard is unused"] > +pub struct Guard<'a, T: ?Sized, B: Backend> { > + pub(crate) lock: &'a Lock<T, B>, > + pub(crate) state: B::GuardState, > + _not_send: PhantomData<*mut ()>, > +} > + > +// 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> core::ops::Deref for Guard<'_, T, B> { > + type Target = T; > + > + fn deref(&self) -> &Self::Target { > + // SAFETY: The caller owns the lock, so it is safe to deref the protected data. > + unsafe { &*self.lock.data.get() } > + } > +} > + > +impl<T: ?Sized, B: Backend> core::ops::DerefMut for Guard<'_, T, B> { > + fn deref_mut(&mut self) -> &mut Self::Target { > + // SAFETY: The caller owns the lock, so it is safe to deref the protected data. > + unsafe { &mut *self.lock.data.get() } > + } > +} > + > +impl<T: ?Sized, B: Backend> Drop for Guard<'_, T, B> { > + fn drop(&mut self) { > + // SAFETY: The caller owns the lock, so it is safe to unlock it. > + unsafe { B::unlock(self.lock.state.get(), &self.state) }; > + } > +} > + > +impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> { > + /// Constructs a new immutable lock guard. > + /// > + /// # Safety > + /// > + /// The caller must ensure that it owns the lock. > + pub(crate) unsafe fn new(lock: &'a Lock<T, B>, state: B::GuardState) -> Self { > + Self { > + lock, > + state, > + _not_send: PhantomData, > + } > + } > +} > -- > 2.34.1 >
| |