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]>
---
rust/kernel/types.rs | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 29db59d6119a..9c8d94c04deb 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -321,6 +321,17 @@ 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` directly, 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` for the
+// same reason as above. `T` needs to be `Send` as well because a thread can clone an `&ARef<T>`
+// into an `ARef<T>`, which may lead to `T` being accessed by the same reasoning as above.
+unsafe impl<T: AlwaysRefCounted + Sync + Send> Sync for ARef<T> {}
+
impl<T: AlwaysRefCounted> ARef<T> {
/// Creates a new instance of [`ARef`].
///
base-commit: ac9a78681b921877518763ba0e89202254349d1b
--
2.40.1.606.ga4b1b128d6-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]>
---
rust/kernel/task.rs | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 526d29a0ae27..4f1fe9aa9f6e 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -64,6 +64,11 @@ macro_rules! current {
#[repr(transparent)]
pub struct Task(pub(crate) Opaque<bindings::task_struct>);
+// SAFETY: The only situation in which this 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 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`).
--
2.40.1.606.ga4b1b128d6-goog
On Wed, May 17, 2023 at 09:59:04AM +0000, Alice Ryhl wrote:
> 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]>
> ---
> rust/kernel/types.rs | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 29db59d6119a..9c8d94c04deb 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -321,6 +321,17 @@ 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` directly, for
Does the "ultimately access `T` directly" here imply mutably or
exclusively? If so, it makes sense to me to call it out. I'm trying to
make sure we can agree on some "common terminologies" ;-)
Regards,
Boqun
> +// 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` for the
> +// same reason as above. `T` needs to be `Send` as well because a thread can clone an `&ARef<T>`
> +// into an `ARef<T>`, which may lead to `T` being accessed by the same reasoning as above.
> +unsafe impl<T: AlwaysRefCounted + Sync + Send> Sync for ARef<T> {}
> +
> impl<T: AlwaysRefCounted> ARef<T> {
> /// Creates a new instance of [`ARef`].
> ///
>
> base-commit: ac9a78681b921877518763ba0e89202254349d1b
> --
> 2.40.1.606.ga4b1b128d6-goog
>
On 5/18/23 23:24, Boqun Feng wrote:
> On Wed, May 17, 2023 at 09:59:04AM +0000, Alice Ryhl wrote:
>> +// 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` directly, for
>
> Does the "ultimately access `T` directly" here imply mutably or
> exclusively? If so, it makes sense to me to call it out. I'm trying to
> make sure we can agree on some "common terminologies" ;)
It means "access using a mutable reference". I agree that "directly" is a bit
unclear - I copied it from the safety comment on Arc.
Alice
Alice Ryhl <[email protected]> writes:
> 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]>
> ---
> rust/kernel/types.rs | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 29db59d6119a..9c8d94c04deb 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -321,6 +321,17 @@ 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` directly, 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` for the
> +// same reason as above. `T` needs to be `Send` as well because a thread can clone an `&ARef<T>`
> +// into an `ARef<T>`, which may lead to `T` being accessed by the same reasoning as above.
> +unsafe impl<T: AlwaysRefCounted + Sync + Send> Sync for ARef<T> {}
Nit: I would prefer repeating the safety comment details, in case the
two drift apart in the future.
BR Andreas
> +
> impl<T: AlwaysRefCounted> ARef<T> {
> /// Creates a new instance of [`ARef`].
> ///
>
> base-commit: ac9a78681b921877518763ba0e89202254349d1b
Alice Ryhl <[email protected]> writes:
> 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]>
> ---
> rust/kernel/task.rs | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index 526d29a0ae27..4f1fe9aa9f6e 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -64,6 +64,11 @@ macro_rules! current {
> #[repr(transparent)]
> pub struct Task(pub(crate) Opaque<bindings::task_struct>);
>
> +// SAFETY: The only situation in which this 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 {}
To enhance clarity, could you elaborate _why_ `Task` can never be
accessed mutably by Rust? Perhaps "By design, `Task` can only be
accessed thorough `&Task` and `Task` can never be owned by the Rust
side. Therefore the only situation ...".
> +
> // 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
> // synchronised by C code (e.g., `signal_pending`).