The C maple_tree struct contains a *mut c_void, which prevents Rust from auto-deriving Send/Sync. Following is an example error message when using MapleTree in nova-core's Vmm. This propagates up through MapleTreeAlloc to Vmm, BarUser, Gpu, and NovaCore, causing NovaCore to fail the Send bound required by pci::Driver: error[E0277]: `*mut c_void` cannot be sent between threads safely --> drivers/gpu/nova-core/driver.rs:77:22 | 77 | impl pci::Driver for NovaCore { | ^^^^^^^^ `*mut c_void` cannot be sent between threads safely | = help: within `MapleTreeAlloc<()>`, the trait `Send` is not implemented for `*mut c_void` note: required because it appears within the type `kernel::bindings::maple_tree` note: required because it appears within the type `Opaque` note: required because it appears within the type `MapleTree<()>` note: required because it appears within the type `MapleTreeAlloc<()>` = note: required for `Box, Kmalloc>` to implement `Send` note: required because it appears within the type `core::pin::Pin, Kmalloc>>` note: required because it appears within the type `Vmm` note: required because it appears within the type `BarUser` note: required because it appears within the type `Gpu` note: required because it appears within the type `NovaCore` note: required by a bound in `kernel::pci::Driver` --> rust/kernel/pci.rs:294:19 Implement Send and Sync for MapleTree. The tree contains no thread-local state, and all shared access goes through the internal ma_lock spinlock. Reviewed-by: Gary Guo Reviewed-by: Alice Ryhl Signed-off-by: Joel Fernandes --- v2->v3: Added tags. v1->v2: Address Gary's comments about the comments. rust/kernel/maple_tree.rs | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/rust/kernel/maple_tree.rs b/rust/kernel/maple_tree.rs index 265d6396a78a..74cfd5c0fb2a 100644 --- a/rust/kernel/maple_tree.rs +++ b/rust/kernel/maple_tree.rs @@ -16,7 +16,11 @@ alloc::Flags, error::to_result, prelude::*, - types::{ForeignOwnable, Opaque}, + types::{ + ForeignOwnable, + NotThreadSafe, + Opaque, // + }, }; /// A maple tree optimized for storing non-overlapping ranges. @@ -240,7 +244,10 @@ pub fn lock(&self) -> MapleGuard<'_, T> { unsafe { bindings::spin_lock(self.ma_lock()) }; // INVARIANT: We just took the spinlock. - MapleGuard(self) + MapleGuard { + tree: self, + _not_send: NotThreadSafe, + } } #[inline] @@ -302,19 +309,30 @@ fn drop(mut self: Pin<&mut Self>) { } } +// SAFETY: `MapleTree` is `Send` if `T` is `Send` because `MapleTree` owns its elements. +unsafe impl Send for MapleTree {} + +// SAFETY: `&MapleTree` never hands out `&T`; all entry access is serialized +// by `ma_lock` or `&mut Guard`, so `T: Send` suffices (`T: Sync` not required). +unsafe impl Sync for MapleTree {} + /// A reference to a [`MapleTree`] that owns the inner lock. /// /// # Invariants /// /// This guard owns the inner spinlock. #[must_use = "if unused, the lock will be immediately unlocked"] -pub struct MapleGuard<'tree, T: ForeignOwnable>(&'tree MapleTree); +pub struct MapleGuard<'tree, T: ForeignOwnable> { + tree: &'tree MapleTree, + // A held spinlock must be released on the same CPU that acquired it. + _not_send: NotThreadSafe, +} impl<'tree, T: ForeignOwnable> Drop for MapleGuard<'tree, T> { #[inline] fn drop(&mut self) { // SAFETY: By the type invariants, we hold this spinlock. - unsafe { bindings::spin_unlock(self.0.ma_lock()) }; + unsafe { bindings::spin_unlock(self.tree.ma_lock()) }; } } @@ -323,7 +341,7 @@ impl<'tree, T: ForeignOwnable> MapleGuard<'tree, T> { pub fn ma_state(&mut self, first: usize, end: usize) -> MaState<'_, T> { // SAFETY: The `MaState` borrows this `MapleGuard`, so it can also borrow the `MapleGuard`s // read/write permissions to the maple tree. - unsafe { MaState::new_raw(self.0, first, end) } + unsafe { MaState::new_raw(self.tree, first, end) } } /// Load the value at the given index. @@ -375,7 +393,7 @@ pub fn ma_state(&mut self, first: usize, end: usize) -> MaState<'_, T> { #[inline] pub fn load(&mut self, index: usize) -> Option> { // SAFETY: `self.tree` contains a valid maple tree. - let ret = unsafe { bindings::mtree_load(self.0.tree.get(), index) }; + let ret = unsafe { bindings::mtree_load(self.tree.tree.get(), index) }; if ret.is_null() { return None; } -- 2.34.1