In Rust, the `Send` and `Sync` traits are used to mark in what ways a
specific type is thread safe. In this patch series, I add some missing
thread safety markers and improve the documentation related to them.
This change will let you compile some code that would currently fail to
compile even though it doesn't actually violate any thread safety rules.
You can find a definition of what these marker traits mean at [1].
Changes since v2:
* Removed "for example" as suggested in [2].
* Added Reviewed-by annotations from v2 review.
[1]: https://stackoverflow.com/a/68708557/1704411
[2]: https://lore.kernel.org/all/[email protected]/
Alice Ryhl (4):
rust: sync: reword the `Arc` safety comment for `Send`
rust: sync: reword the `Arc` safety comment for `Sync`
rust: specify when `ARef` is thread safe
rust: task: add `Send` marker to `Task`
rust/kernel/sync/arc.rs | 12 +++++++-----
rust/kernel/task.rs | 10 ++++++++--
rust/kernel/types.rs | 13 +++++++++++++
3 files changed, 28 insertions(+), 7 deletions(-)
base-commit: ac9a78681b921877518763ba0e89202254349d1b
--
2.41.0.rc0.172.g3f132b7071-goog
The safety comment on `impl Sync for Arc` references the Send safety
comment. This commit avoids that in case the two comments drift apart in
the future.
Suggested-by: Andreas Hindborg <[email protected]>
Signed-off-by: Alice Ryhl <[email protected]>
Reviewed-by: Andreas Hindborg <[email protected]>
Reviewed-by: Boqun Feng <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Reviewed-by: Benno Lossin <[email protected]>
---
rust/kernel/sync/arc.rs | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 3ad831603cf4..3da84187ecf4 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -150,9 +150,11 @@ impl<T: ?Sized + Unsize<U>, U: ?Sized> core::ops::DispatchFromDyn<Arc<U>> for Ar
// mutable reference when the reference count reaches zero and `T` is dropped.
unsafe impl<T: ?Sized + Sync + Send> Send for Arc<T> {}
-// SAFETY: It is safe to send `&Arc<T>` to another thread when the underlying `T` is `Sync` for the
-// same reason as above. `T` needs to be `Send` as well because a thread can clone an `&Arc<T>`
-// into an `Arc<T>`, which may lead to `T` being accessed by the same reasoning as above.
+// SAFETY: It is safe to send `&Arc<T>` to another thread when the underlying `T` is `Sync`
+// because it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally,
+// it needs `T` to be `Send` because any thread that has a `&Arc<T>` may clone it and get an
+// `Arc<T>` on that thread, so the thread may ultimately access `T` using a mutable reference when
+// the reference count reaches zero and `T` is dropped.
unsafe impl<T: ?Sized + Sync + Send> Sync for Arc<T> {}
impl<T> Arc<T> {
--
2.41.0.rc0.172.g3f132b7071-goog
When a type also implements `Sync`, the meaning of `Send` is just "this
type may be accessed mutably from threads other than the one it is
created on". That's ok for this type.
Signed-off-by: Alice Ryhl <[email protected]>
Reviewed-by: Andreas Hindborg <[email protected]>
Reviewed-by: Gary Guo <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Reviewed-by: Benno Lossin <[email protected]>
---
rust/kernel/task.rs | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 526d29a0ae27..7eda15e5f1b3 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -64,8 +64,14 @@ macro_rules! current {
#[repr(transparent)]
pub struct Task(pub(crate) Opaque<bindings::task_struct>);
-// SAFETY: It's OK to access `Task` through references from other threads because we're either
-// accessing properties that don't change (e.g., `pid`, `group_leader`) or that are properly
+// SAFETY: By design, the only way to access a `Task` is via the `current` function or via an
+// `ARef<Task>` obtained through the `AlwaysRefCounted` impl. This means that the only situation in
+// which a `Task` can be accessed mutably is when the refcount drops to zero and the destructor
+// runs. It is safe for that to happen on any thread, so it is ok for this type to be `Send`.
+unsafe impl Send for Task {}
+
+// SAFETY: It's OK to access `Task` through shared references from other threads because we're
+// either accessing properties that don't change (e.g., `pid`, `group_leader`) or that are properly
// synchronised by C code (e.g., `signal_pending`).
unsafe impl Sync for Task {}
--
2.41.0.rc0.172.g3f132b7071-goog
The safety comment on `impl Send for Arc` talks about "directly"
accessing the value, when it really means "accessing the value with a
mutable reference". This commit clarifies that.
Suggested-by: Boqun Feng <[email protected]>
Signed-off-by: Alice Ryhl <[email protected]>
Reviewed-by: Andreas Hindborg <[email protected]>
Reviewed-by: Boqun Feng <[email protected]>
Reviewed-by: Gary Guo <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Reviewed-by: Benno Lossin <[email protected]>
---
rust/kernel/sync/arc.rs | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index e6d206242465..3ad831603cf4 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -146,8 +146,8 @@ impl<T: ?Sized + Unsize<U>, U: ?Sized> core::ops::DispatchFromDyn<Arc<U>> for Ar
// SAFETY: It is safe to send `Arc<T>` to another thread when the underlying `T` is `Sync` because
// it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs
-// `T` to be `Send` because any thread that has an `Arc<T>` may ultimately access `T` directly, for
-// example, when the reference count reaches zero and `T` is dropped.
+// `T` to be `Send` because any thread that has an `Arc<T>` may ultimately access `T` using a
+// mutable reference when the reference count reaches zero and `T` is dropped.
unsafe impl<T: ?Sized + Sync + Send> Send for Arc<T> {}
// SAFETY: It is safe to send `&Arc<T>` to another thread when the underlying `T` is `Sync` for the
--
2.41.0.rc0.172.g3f132b7071-goog
An `ARef` behaves just like the `Arc` when it comes to thread safety, so
we can reuse the thread safety comments from `Arc` here.
This is necessary because without this change, the Rust compiler will
assume that things are not thread safe even though they are.
Signed-off-by: Alice Ryhl <[email protected]>
Reviewed-by: Andreas Hindborg <[email protected]>
Reviewed-by: Boqun Feng <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Reviewed-by: Benno Lossin <[email protected]>
---
rust/kernel/types.rs | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 29db59d6119a..1e5380b16ed5 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -321,6 +321,19 @@ pub struct ARef<T: AlwaysRefCounted> {
_p: PhantomData<T>,
}
+// SAFETY: It is safe to send `ARef<T>` to another thread when the underlying `T` is `Sync` because
+// it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs
+// `T` to be `Send` because any thread that has an `ARef<T>` may ultimately access `T` using a
+// mutable reference, for example, when the reference count reaches zero and `T` is dropped.
+unsafe impl<T: AlwaysRefCounted + Sync + Send> Send for ARef<T> {}
+
+// SAFETY: It is safe to send `&ARef<T>` to another thread when the underlying `T` is `Sync`
+// because it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally,
+// it needs `T` to be `Send` because any thread that has a `&ARef<T>` may clone it and get an
+// `ARef<T>` on that thread, so the thread may ultimately access `T` using a mutable reference, for
+// example, when the reference count reaches zero and `T` is dropped.
+unsafe impl<T: AlwaysRefCounted + Sync + Send> Sync for ARef<T> {}
+
impl<T: AlwaysRefCounted> ARef<T> {
/// Creates a new instance of [`ARef`].
///
--
2.41.0.rc0.172.g3f132b7071-goog
On Wed, May 31, 2023 at 5:00 PM Alice Ryhl <[email protected]> wrote:
>
> In Rust, the `Send` and `Sync` traits are used to mark in what ways a
> specific type is thread safe. In this patch series, I add some missing
> thread safety markers and improve the documentation related to them.
> This change will let you compile some code that would currently fail to
> compile even though it doesn't actually violate any thread safety rules.
>
> You can find a definition of what these marker traits mean at [1].
>
> Changes since v2:
>
> * Removed "for example" as suggested in [2].
> * Added Reviewed-by annotations from v2 review.
>
> [1]: https://stackoverflow.com/a/68708557/1704411
> [2]: https://lore.kernel.org/all/[email protected]/
Applied to `rust-next` -- thanks everyone (and Alice for taking the
effort to send this v3 even for the small change)!
Cheers,
Miguel