[PATCH v1 1/2] rust: specify when `ARef` is thread safe

[PATCH v1 1/2] rust: specify when `ARef` is thread safe

[Alice Ryhl]

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 <aliceryhl@google.com>
---
 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

[PATCH v1 2/2] rust: task: add `Send` marker to `Task`

[Alice Ryhl]

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 <aliceryhl@google.com>
---
 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

Re: [PATCH v1 2/2] rust: task: add `Send` marker to `Task`

[Andreas Hindborg]

Alice Ryhl <aliceryhl@google.com> 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 <aliceryhl@google.com>
> ---
>  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`).

Re: [PATCH v1 1/2] rust: specify when `ARef` is thread safe

[Boqun Feng]

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 <aliceryhl@google.com>
> ---
>  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
> 

Re: [PATCH v1 1/2] rust: specify when `ARef` is thread safe

[Alice Ryhl]

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

Re: [PATCH v1 1/2] rust: specify when `ARef` is thread safe

[Andreas Hindborg]

Alice Ryhl <aliceryhl@google.com> 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 <aliceryhl@google.com>
> ---
>  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