[PATCH v2 0/2] Arc methods for linked list

[PATCH v2 0/2] Arc methods for linked list

[Alice Ryhl]

This patchset contains two useful methods for the Arc type. They will be
used in my Rust linked list implementation, which Rust Binder uses. See
the Rust Binder RFC [1] for more information. Both these commits and
the linked list that uses them are present in the branch referenced by
the RFC.

I will send the linked list to the mailing list soon.

This patchset is based on rust-next and depends on [2].

Link: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-0-08ba9197f637@google.com/ [1]
Link: https://lore.kernel.org/all/20240215104601.1267763-1-aliceryhl@google.com/ [2]
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Changes in v2:
- Move raw_to_inner_ptr to ArcInner and rename to container_of.
- Reword safety conditions for raw_to_inner_ptr to match its users.
- Link to v1: https://lore.kernel.org/r/20240219-arc-for-list-v1-0-d98cd92c760b@google.com

---
Alice Ryhl (2):
      rust: sync: add `ArcBorrow::from_raw`
      rust: sync: add `Arc::into_unique_or_drop`

 rust/kernel/sync/arc.rs | 107 ++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 89 insertions(+), 18 deletions(-)
---
base-commit: e944171070b65521c0513c01c56174ec8fd7d249
change-id: 20240209-arc-for-list-a2c126c2ad5c

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

[PATCH v2 1/2] rust: sync: add `ArcBorrow::from_raw`

[Alice Ryhl]

Allows access to a value in an `Arc` that is currently held as a raw
pointer due to use of `Arc::into_raw`, without destroying or otherwise
consuming that raw pointer.

This is a dependency of the linked list that Rust Binder uses. The
linked list uses this method when iterating over the linked list.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/sync/arc.rs | 76 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 58 insertions(+), 18 deletions(-)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 7d4c4bf58388..53addb8876c2 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -137,6 +137,39 @@ struct ArcInner<T: ?Sized> {
     data: T,
 }
 
+impl<T: ?Sized> ArcInner<T> {
+    /// Converts a pointer to the contents of an [`Arc`] into a pointer to the [`ArcInner`].
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must have been returned by a previous call to [`Arc::into_raw`], and the `Arc` must
+    /// not yet have been destroyed.
+    unsafe fn container_of(ptr: *const T) -> NonNull<ArcInner<T>> {
+        let refcount_layout = Layout::new::<bindings::refcount_t>();
+        // SAFETY: The caller guarantees that the pointer is valid.
+        let val_layout = Layout::for_value(unsafe { &*ptr });
+        // SAFETY: We're computing the layout of a real struct that existed when compiling this
+        // binary, so its layout is not so large that it can trigger arithmetic overflow.
+        let val_offset = unsafe { refcount_layout.extend(val_layout).unwrap_unchecked().1 };
+
+        // Pointer casts leave the metadata unchanged. This is okay because the metadata of `T` and
+        // `ArcInner<T>` is the same since `ArcInner` is a struct with `T` as its last field.
+        //
+        // This is documented at:
+        // <https://doc.rust-lang.org/std/ptr/trait.Pointee.html>.
+        let ptr = ptr as *const ArcInner<T>;
+
+        // SAFETY: The pointer is in-bounds of an allocation both before and after offsetting the
+        // pointer, since it originates from a previous call to `Arc::into_raw` on an `Arc` that is
+        // still valid.
+        let ptr = unsafe { ptr.byte_sub(val_offset) };
+
+        // SAFETY: The pointer can't be null since you can't have an `ArcInner<T>` value at the null
+        // address.
+        unsafe { NonNull::new_unchecked(ptr.cast_mut()) }
+    }
+}
+
 // This is to allow [`Arc`] (and variants) to be used as the type of `self`.
 impl<T: ?Sized> core::ops::Receiver for Arc<T> {}
 
@@ -232,27 +265,13 @@ pub fn into_raw(self) -> *const T {
     /// `ptr` must have been returned by a previous call to [`Arc::into_raw`]. Additionally, it
     /// must not be called more than once for each previous call to [`Arc::into_raw`].
     pub unsafe fn from_raw(ptr: *const T) -> Self {
-        let refcount_layout = Layout::new::<bindings::refcount_t>();
-        // SAFETY: The caller guarantees that the pointer is valid.
-        let val_layout = Layout::for_value(unsafe { &*ptr });
-        // SAFETY: We're computing the layout of a real struct that existed when compiling this
-        // binary, so its layout is not so large that it can trigger arithmetic overflow.
-        let val_offset = unsafe { refcount_layout.extend(val_layout).unwrap_unchecked().1 };
-
-        // Pointer casts leave the metadata unchanged. This is okay because the metadata of `T` and
-        // `ArcInner<T>` is the same since `ArcInner` is a struct with `T` as its last field.
-        //
-        // This is documented at:
-        // <https://doc.rust-lang.org/std/ptr/trait.Pointee.html>.
-        let ptr = ptr as *const ArcInner<T>;
-
-        // SAFETY: The pointer is in-bounds of an allocation both before and after offsetting the
-        // pointer, since it originates from a previous call to `Arc::into_raw` and is still valid.
-        let ptr = unsafe { ptr.byte_sub(val_offset) };
+        // SAFETY: The caller promises that this pointer originates from a call to `into_raw` on an
+        // `Arc` that is still valid.
+        let ptr = unsafe { ArcInner::container_of(ptr) };
 
         // SAFETY: By the safety requirements we know that `ptr` came from `Arc::into_raw`, so the
         // reference count held then will be owned by the new `Arc` object.
-        unsafe { Self::from_inner(NonNull::new_unchecked(ptr.cast_mut())) }
+        unsafe { Self::from_inner(ptr) }
     }
 
     /// Returns an [`ArcBorrow`] from the given [`Arc`].
@@ -453,6 +472,27 @@ unsafe fn new(inner: NonNull<ArcInner<T>>) -> Self {
             _p: PhantomData,
         }
     }
+
+    /// Creates an [`ArcBorrow`] to an [`Arc`] that has previously been deconstructed with
+    /// [`Arc::into_raw`].
+    ///
+    /// # Safety
+    ///
+    /// * The provided pointer must originate from a call to [`Arc::into_raw`].
+    /// * For the duration of the lifetime annotated on this `ArcBorrow`, the reference count must
+    ///   not hit zero.
+    /// * For the duration of the lifetime annotated on this `ArcBorrow`, there must not be a
+    ///   [`UniqueArc`] reference to this value.
+    pub unsafe fn from_raw(ptr: *const T) -> Self {
+        // SAFETY: The caller promises that this pointer originates from a call to `into_raw` on an
+        // `Arc` that is still valid.
+        let ptr = unsafe { ArcInner::container_of(ptr) };
+
+        // SAFETY: The caller promises that the value remains valid since the reference count must
+        // not hit zero, and no mutable reference will be created since that would involve a
+        // `UniqueArc`.
+        unsafe { Self::new(ptr) }
+    }
 }
 
 impl<T: ?Sized> From<ArcBorrow<'_, T>> for Arc<T> {

-- 
2.44.0.rc1.240.g4c46232300-goog

Re: [PATCH v2 1/2] rust: sync: add `ArcBorrow::from_raw`

[Benno Lossin]

On 2/28/24 14:00, Alice Ryhl wrote:
> Allows access to a value in an `Arc` that is currently held as a raw
> pointer due to use of `Arc::into_raw`, without destroying or otherwise
> consuming that raw pointer.
> 
> This is a dependency of the linked list that Rust Binder uses. The
> linked list uses this method when iterating over the linked list.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>   rust/kernel/sync/arc.rs | 76 +++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 58 insertions(+), 18 deletions(-)
> 
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 7d4c4bf58388..53addb8876c2 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -137,6 +137,39 @@ struct ArcInner<T: ?Sized> {
>       data: T,
>   }
> 
> +impl<T: ?Sized> ArcInner<T> {
> +    /// Converts a pointer to the contents of an [`Arc`] into a pointer to the [`ArcInner`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must have been returned by a previous call to [`Arc::into_raw`], and the `Arc` must
> +    /// not yet have been destroyed.
> +    unsafe fn container_of(ptr: *const T) -> NonNull<ArcInner<T>> {
> +        let refcount_layout = Layout::new::<bindings::refcount_t>();
> +        // SAFETY: The caller guarantees that the pointer is valid.
> +        let val_layout = Layout::for_value(unsafe { &*ptr });
> +        // SAFETY: We're computing the layout of a real struct that existed when compiling this
> +        // binary, so its layout is not so large that it can trigger arithmetic overflow.
> +        let val_offset = unsafe { refcount_layout.extend(val_layout).unwrap_unchecked().1 };
> +
> +        // Pointer casts leave the metadata unchanged. This is okay because the metadata of `T` and
> +        // `ArcInner<T>` is the same since `ArcInner` is a struct with `T` as its last field.
> +        //
> +        // This is documented at:
> +        // <https://doc.rust-lang.org/std/ptr/trait.Pointee.html>.
> +        let ptr = ptr as *const ArcInner<T>;
> +
> +        // SAFETY: The pointer is in-bounds of an allocation both before and after offsetting the
> +        // pointer, since it originates from a previous call to `Arc::into_raw` on an `Arc` that is
> +        // still valid.
> +        let ptr = unsafe { ptr.byte_sub(val_offset) };
> +
> +        // SAFETY: The pointer can't be null since you can't have an `ArcInner<T>` value at the null
> +        // address.
> +        unsafe { NonNull::new_unchecked(ptr.cast_mut()) }
> +    }
> +}
> +
>   // This is to allow [`Arc`] (and variants) to be used as the type of `self`.
>   impl<T: ?Sized> core::ops::Receiver for Arc<T> {}
> 
> @@ -232,27 +265,13 @@ pub fn into_raw(self) -> *const T {
>       /// `ptr` must have been returned by a previous call to [`Arc::into_raw`]. Additionally, it
>       /// must not be called more than once for each previous call to [`Arc::into_raw`].
>       pub unsafe fn from_raw(ptr: *const T) -> Self {
> -        let refcount_layout = Layout::new::<bindings::refcount_t>();
> -        // SAFETY: The caller guarantees that the pointer is valid.
> -        let val_layout = Layout::for_value(unsafe { &*ptr });
> -        // SAFETY: We're computing the layout of a real struct that existed when compiling this
> -        // binary, so its layout is not so large that it can trigger arithmetic overflow.
> -        let val_offset = unsafe { refcount_layout.extend(val_layout).unwrap_unchecked().1 };
> -
> -        // Pointer casts leave the metadata unchanged. This is okay because the metadata of `T` and
> -        // `ArcInner<T>` is the same since `ArcInner` is a struct with `T` as its last field.
> -        //
> -        // This is documented at:
> -        // <https://doc.rust-lang.org/std/ptr/trait.Pointee.html>.
> -        let ptr = ptr as *const ArcInner<T>;
> -
> -        // SAFETY: The pointer is in-bounds of an allocation both before and after offsetting the
> -        // pointer, since it originates from a previous call to `Arc::into_raw` and is still valid.
> -        let ptr = unsafe { ptr.byte_sub(val_offset) };
> +        // SAFETY: The caller promises that this pointer originates from a call to `into_raw` on an
> +        // `Arc` that is still valid.
> +        let ptr = unsafe { ArcInner::container_of(ptr) };
> 
>           // SAFETY: By the safety requirements we know that `ptr` came from `Arc::into_raw`, so the
>           // reference count held then will be owned by the new `Arc` object.
> -        unsafe { Self::from_inner(NonNull::new_unchecked(ptr.cast_mut())) }
> +        unsafe { Self::from_inner(ptr) }
>       }
> 
>       /// Returns an [`ArcBorrow`] from the given [`Arc`].
> @@ -453,6 +472,27 @@ unsafe fn new(inner: NonNull<ArcInner<T>>) -> Self {
>               _p: PhantomData,
>           }
>       }
> +
> +    /// Creates an [`ArcBorrow`] to an [`Arc`] that has previously been deconstructed with
> +    /// [`Arc::into_raw`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// * The provided pointer must originate from a call to [`Arc::into_raw`].
> +    /// * For the duration of the lifetime annotated on this `ArcBorrow`, the reference count must
> +    ///   not hit zero.
> +    /// * For the duration of the lifetime annotated on this `ArcBorrow`, there must not be a
> +    ///   [`UniqueArc`] reference to this value.

I am a bit confused, this feels to me like it should be guaranteed by
`UniqueArc` and not by this function. Currently there is not even a way
of getting a `*const T` from a `UniqueArc`.
So I think we can remove this requirement and instead have the
requirement for creating `UniqueArc` that not only the refcount is
exactly 1, but also that no `ArcBorrow` exists.

-- 
Cheers,
Benno

> +    pub unsafe fn from_raw(ptr: *const T) -> Self {
> +        // SAFETY: The caller promises that this pointer originates from a call to `into_raw` on an
> +        // `Arc` that is still valid.
> +        let ptr = unsafe { ArcInner::container_of(ptr) };
> +
> +        // SAFETY: The caller promises that the value remains valid since the reference count must
> +        // not hit zero, and no mutable reference will be created since that would involve a
> +        // `UniqueArc`.
> +        unsafe { Self::new(ptr) }
> +    }
>   }
> 
>   impl<T: ?Sized> From<ArcBorrow<'_, T>> for Arc<T> {
> 
> --
> 2.44.0.rc1.240.g4c46232300-goog
> 

Re: [PATCH v2 1/2] rust: sync: add `ArcBorrow::from_raw`

[Alice Ryhl]

On Sat, Mar 9, 2024 at 1:56 PM Benno Lossin <benno.lossin@proton.me> wrote:
> > +    /// Creates an [`ArcBorrow`] to an [`Arc`] that has previously been deconstructed with
> > +    /// [`Arc::into_raw`].
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// * The provided pointer must originate from a call to [`Arc::into_raw`].
> > +    /// * For the duration of the lifetime annotated on this `ArcBorrow`, the reference count must
> > +    ///   not hit zero.
> > +    /// * For the duration of the lifetime annotated on this `ArcBorrow`, there must not be a
> > +    ///   [`UniqueArc`] reference to this value.
>
> I am a bit confused, this feels to me like it should be guaranteed by
> `UniqueArc` and not by this function. Currently there is not even a way
> of getting a `*const T` from a `UniqueArc`.
> So I think we can remove this requirement and instead have the
> requirement for creating `UniqueArc` that not only the refcount is
> exactly 1, but also that no `ArcBorrow` exists.

If you combine this with `into_unique_or_drop` that is introduced in
the next patch of this series, then you could perform these
operations:

* Arc::into_raw
* ArcBorrow::from_raw
* Arc::from_raw
* Arc::into_unique_or_drop
* And then use the ArcBorrow

If we drop the final safety requirement from `ArcBorrow::from_raw`,
then the above would be allowed. The refcount does not hit zero at any
point during these operations. The only unsafe functions are
Arc::into_raw, Arc::from_raw, and ArcBorrow::from_raw, so this safety
requirement must go on one of them. It seems to me that, out of these,
ArcBorrow::from_raw is the most appropriate choice.

Thoughts?

Alice

Re: [PATCH v2 1/2] rust: sync: add `ArcBorrow::from_raw`

[Benno Lossin]

On 3/11/24 09:58, Alice Ryhl wrote:
> On Sat, Mar 9, 2024 at 1:56 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>> +    /// Creates an [`ArcBorrow`] to an [`Arc`] that has previously been deconstructed with
>>> +    /// [`Arc::into_raw`].
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// * The provided pointer must originate from a call to [`Arc::into_raw`].
>>> +    /// * For the duration of the lifetime annotated on this `ArcBorrow`, the reference count must
>>> +    ///   not hit zero.
>>> +    /// * For the duration of the lifetime annotated on this `ArcBorrow`, there must not be a
>>> +    ///   [`UniqueArc`] reference to this value.
>>
>> I am a bit confused, this feels to me like it should be guaranteed by
>> `UniqueArc` and not by this function. Currently there is not even a way
>> of getting a `*const T` from a `UniqueArc`.
>> So I think we can remove this requirement and instead have the
>> requirement for creating `UniqueArc` that not only the refcount is
>> exactly 1, but also that no `ArcBorrow` exists.
> 
> If you combine this with `into_unique_or_drop` that is introduced in
> the next patch of this series, then you could perform these
> operations:
> 
> * Arc::into_raw
> * ArcBorrow::from_raw
> * Arc::from_raw
> * Arc::into_unique_or_drop
> * And then use the ArcBorrow
> 
> If we drop the final safety requirement from `ArcBorrow::from_raw`,
> then the above would be allowed. The refcount does not hit zero at any
> point during these operations. The only unsafe functions are
> Arc::into_raw, Arc::from_raw, and ArcBorrow::from_raw, so this safety
> requirement must go on one of them. It seems to me that, out of these,
> ArcBorrow::from_raw is the most appropriate choice.
> 
> Thoughts?

I see, it is a bit unfortunate that we have to put the constraint onto
`ArcBorrow::from_raw`, but I also do not see a better place. Thus:

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

-- 
Cheers,
Benno

[PATCH v2 2/2] rust: sync: add `Arc::into_unique_or_drop`

[Alice Ryhl]

Decrement the refcount of an `Arc`, but handle the case where it hits
zero by taking ownership of the now-unique `Arc`, instead of destroying
and deallocating it.

This is a dependency of the linked list that Rust Binder uses. The
linked list uses this method as part of its `ListArc` abstraction.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/sync/arc.rs | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 53addb8876c2..df2dfe0de83c 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -290,6 +290,37 @@ pub fn as_arc_borrow(&self) -> ArcBorrow<'_, T> {
     pub fn ptr_eq(this: &Self, other: &Self) -> bool {
         core::ptr::eq(this.ptr.as_ptr(), other.ptr.as_ptr())
     }
+
+    /// Converts this [`Arc`] into a [`UniqueArc`], or destroys it if it is not unique.
+    ///
+    /// When this destroys the `Arc`, it does so while properly avoiding races. This means that
+    /// this method will never call the destructor of the value.
+    pub fn into_unique_or_drop(self) -> Option<Pin<UniqueArc<T>>> {
+        // We will manually manage the refcount in this method, so we disable the destructor.
+        let me = ManuallyDrop::new(self);
+        // SAFETY: We own a refcount, so the pointer is still valid.
+        let refcount = unsafe { me.ptr.as_ref() }.refcount.get();
+
+        // SAFETY: If the refcount reaches a non-zero value, then we have destroyed this `Arc` and
+        // will return without further touching the `Arc`. If the refcount reaches zero, then there
+        // are no other arcs, and we can create a `UniqueArc`.
+        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
+        if is_zero {
+            // SAFETY: We have exclusive access to the arc, so we can perform unsynchronized
+            // accesses to the refcount.
+            unsafe { core::ptr::write(refcount, bindings::REFCOUNT_INIT(1)) };
+
+            // SAFETY: We own one refcount, so we can create a `UniqueArc`. It needs to be pinned,
+            // since an `Arc` is pinned.
+            unsafe {
+                Some(Pin::new_unchecked(UniqueArc {
+                    inner: Arc::from_inner(me.ptr),
+                }))
+            }
+        } else {
+            None
+        }
+    }
 }
 
 impl<T: 'static> ForeignOwnable for Arc<T> {

-- 
2.44.0.rc1.240.g4c46232300-goog

Re: [PATCH v2 2/2] rust: sync: add `Arc::into_unique_or_drop`

[Benno Lossin]

On 2/28/24 14:00, Alice Ryhl wrote:
> Decrement the refcount of an `Arc`, but handle the case where it hits
> zero by taking ownership of the now-unique `Arc`, instead of destroying
> and deallocating it.
> 
> This is a dependency of the linked list that Rust Binder uses. The
> linked list uses this method as part of its `ListArc` abstraction.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>   rust/kernel/sync/arc.rs | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
> 
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 53addb8876c2..df2dfe0de83c 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -290,6 +290,37 @@ pub fn as_arc_borrow(&self) -> ArcBorrow<'_, T> {
>       pub fn ptr_eq(this: &Self, other: &Self) -> bool {
>           core::ptr::eq(this.ptr.as_ptr(), other.ptr.as_ptr())
>       }
> +
> +    /// Converts this [`Arc`] into a [`UniqueArc`], or destroys it if it is not unique.
> +    ///
> +    /// When this destroys the `Arc`, it does so while properly avoiding races. This means that
> +    /// this method will never call the destructor of the value.
> +    pub fn into_unique_or_drop(self) -> Option<Pin<UniqueArc<T>>> {
> +        // We will manually manage the refcount in this method, so we disable the destructor.
> +        let me = ManuallyDrop::new(self);
> +        // SAFETY: We own a refcount, so the pointer is still valid.
> +        let refcount = unsafe { me.ptr.as_ref() }.refcount.get();
> +
> +        // SAFETY: If the refcount reaches a non-zero value, then we have destroyed this `Arc` and
> +        // will return without further touching the `Arc`. If the refcount reaches zero, then there
> +        // are no other arcs, and we can create a `UniqueArc`.

This comment is not explaining why it is safe to call
`refcount_dec_and_test` on `refcount`.
It dose however explain what you are going to do, so please keep it, but
not as a SAFETY comment.

> +        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
> +        if is_zero {
> +            // SAFETY: We have exclusive access to the arc, so we can perform unsynchronized
> +            // accesses to the refcount.
> +            unsafe { core::ptr::write(refcount, bindings::REFCOUNT_INIT(1)) };
> +
> +            // SAFETY: We own one refcount, so we can create a `UniqueArc`. It needs to be pinned,
> +            // since an `Arc` is pinned.

The `unsafe` block is only needed due to the `new_unchecked` call, which
you could avoid by using `.into()`. The `SAFETY` should also be an
`INVARIANT` comment instead.

-- 
Cheers,
Benno

> +            unsafe {
> +                Some(Pin::new_unchecked(UniqueArc {
> +                    inner: Arc::from_inner(me.ptr),
> +                }))
> +            }
> +        } else {
> +            None
> +        }
> +    }
>   }
> 
>   impl<T: 'static> ForeignOwnable for Arc<T> {
> 
> --
> 2.44.0.rc1.240.g4c46232300-goog
> 

Re: [PATCH v2 2/2] rust: sync: add `Arc::into_unique_or_drop`

[Alice Ryhl]

On Sat, Mar 9, 2024 at 2:02 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 2/28/24 14:00, Alice Ryhl wrote:
> > +        // SAFETY: If the refcount reaches a non-zero value, then we have destroyed this `Arc` and
> > +        // will return without further touching the `Arc`. If the refcount reaches zero, then there
> > +        // are no other arcs, and we can create a `UniqueArc`.
>
> This comment is not explaining why it is safe to call
> `refcount_dec_and_test` on `refcount`.
> It dose however explain what you are going to do, so please keep it, but
> not as a SAFETY comment.

I'll reword.

> > +        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
> > +        if is_zero {
> > +            // SAFETY: We have exclusive access to the arc, so we can perform unsynchronized
> > +            // accesses to the refcount.
> > +            unsafe { core::ptr::write(refcount, bindings::REFCOUNT_INIT(1)) };
> > +
> > +            // SAFETY: We own one refcount, so we can create a `UniqueArc`. It needs to be pinned,
> > +            // since an `Arc` is pinned.
>
> The `unsafe` block is only needed due to the `new_unchecked` call, which
> you could avoid by using `.into()`. The `SAFETY` should also be an
> `INVARIANT` comment instead.
>
> > +            unsafe {
> > +                Some(Pin::new_unchecked(UniqueArc {
> > +                    inner: Arc::from_inner(me.ptr),
> > +                }))
> > +            }

The from_inner method is also unsafe.

I think that using new_unchecked here makes more sense. That method is
usually used in the case where something is already pinned, whereas
into() is usually used to pin something that was not previously
pinned.

Alice

Re: [PATCH v2 2/2] rust: sync: add `Arc::into_unique_or_drop`

[Benno Lossin]

On 3/11/24 10:03, Alice Ryhl wrote:
> On Sat, Mar 9, 2024 at 2:02 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> On 2/28/24 14:00, Alice Ryhl wrote:
>>> +        // SAFETY: If the refcount reaches a non-zero value, then we have destroyed this `Arc` and
>>> +        // will return without further touching the `Arc`. If the refcount reaches zero, then there
>>> +        // are no other arcs, and we can create a `UniqueArc`.
>>
>> This comment is not explaining why it is safe to call
>> `refcount_dec_and_test` on `refcount`.
>> It dose however explain what you are going to do, so please keep it, but
>> not as a SAFETY comment.
> 
> I'll reword.
> 
>>> +        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
>>> +        if is_zero {
>>> +            // SAFETY: We have exclusive access to the arc, so we can perform unsynchronized
>>> +            // accesses to the refcount.
>>> +            unsafe { core::ptr::write(refcount, bindings::REFCOUNT_INIT(1)) };
>>> +
>>> +            // SAFETY: We own one refcount, so we can create a `UniqueArc`. It needs to be pinned,
>>> +            // since an `Arc` is pinned.
>>
>> The `unsafe` block is only needed due to the `new_unchecked` call, which
>> you could avoid by using `.into()`. The `SAFETY` should also be an
>> `INVARIANT` comment instead.
>>
>>> +            unsafe {
>>> +                Some(Pin::new_unchecked(UniqueArc {
>>> +                    inner: Arc::from_inner(me.ptr),
>>> +                }))
>>> +            }
> 
> The from_inner method is also unsafe.

Ah I missed that, might be a good reason to split the block.
It confused me that the SAFETY comment did not mention why calling
`new_unchecked` is sound.

> I think that using new_unchecked here makes more sense. That method is
> usually used in the case where something is already pinned, whereas
> into() is usually used to pin something that was not previously
> pinned.

I get your argument, but doing it this way avoids an unsafe function
call. I think it would be fine to use `.into()` in this case.
Splitting the unsafe block would also be fine with me.

-- 
Cheers,
Benno

Re: [PATCH v2 2/2] rust: sync: add `Arc::into_unique_or_drop`

[Alice Ryhl]

On Mon, Mar 11, 2024 at 4:15 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 3/11/24 10:03, Alice Ryhl wrote:
> > On Sat, Mar 9, 2024 at 2:02 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >>
> >> On 2/28/24 14:00, Alice Ryhl wrote:
> >>> +        // SAFETY: If the refcount reaches a non-zero value, then we have destroyed this `Arc` and
> >>> +        // will return without further touching the `Arc`. If the refcount reaches zero, then there
> >>> +        // are no other arcs, and we can create a `UniqueArc`.
> >>
> >> This comment is not explaining why it is safe to call
> >> `refcount_dec_and_test` on `refcount`.
> >> It dose however explain what you are going to do, so please keep it, but
> >> not as a SAFETY comment.
> >
> > I'll reword.
> >
> >>> +        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
> >>> +        if is_zero {
> >>> +            // SAFETY: We have exclusive access to the arc, so we can perform unsynchronized
> >>> +            // accesses to the refcount.
> >>> +            unsafe { core::ptr::write(refcount, bindings::REFCOUNT_INIT(1)) };
> >>> +
> >>> +            // SAFETY: We own one refcount, so we can create a `UniqueArc`. It needs to be pinned,
> >>> +            // since an `Arc` is pinned.
> >>
> >> The `unsafe` block is only needed due to the `new_unchecked` call, which
> >> you could avoid by using `.into()`. The `SAFETY` should also be an
> >> `INVARIANT` comment instead.
> >>
> >>> +            unsafe {
> >>> +                Some(Pin::new_unchecked(UniqueArc {
> >>> +                    inner: Arc::from_inner(me.ptr),
> >>> +                }))
> >>> +            }
> >
> > The from_inner method is also unsafe.
>
> Ah I missed that, might be a good reason to split the block.
> It confused me that the SAFETY comment did not mention why calling
> `new_unchecked` is sound.

I don't mind splitting up the unsafe block into several pieces.

> > I think that using new_unchecked here makes more sense. That method is
> > usually used in the case where something is already pinned, whereas
> > into() is usually used to pin something that was not previously
> > pinned.
>
> I get your argument, but doing it this way avoids an unsafe function
> call. I think it would be fine to use `.into()` in this case.
> Splitting the unsafe block would also be fine with me.

If you are okay with splitting the unsafe block instead, then I prefer
that. I don't think avoiding unsafe blocks is always the best answer;
especially not when you're already using unsafe right next to it.

This reminds me of NonNull::new_unchecked(Box::into_raw(my_box)) vs
NonNull::from(Box::leak(my_box)). The latter is safe, but I don't
necessarily think that makes it the better choice. It's also important
that your code carries the right intent.

Another way to go around it could be to add UniqueArc::from_raw or
from_inner methods, as well as from_raw_pinned and from_inner_pinned,
and then use those here.

Alice

Re: [PATCH v2 2/2] rust: sync: add `Arc::into_unique_or_drop`

[Alice Ryhl]

On Mon, Mar 11, 2024 at 4:35 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Mon, Mar 11, 2024 at 4:15 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >
> > On 3/11/24 10:03, Alice Ryhl wrote:
> > > On Sat, Mar 9, 2024 at 2:02 PM Benno Lossin <benno.lossin@proton.me> wrote:
> > >>
> > >> On 2/28/24 14:00, Alice Ryhl wrote:
> > >>> +        // SAFETY: If the refcount reaches a non-zero value, then we have destroyed this `Arc` and
> > >>> +        // will return without further touching the `Arc`. If the refcount reaches zero, then there
> > >>> +        // are no other arcs, and we can create a `UniqueArc`.
> > >>
> > >> This comment is not explaining why it is safe to call
> > >> `refcount_dec_and_test` on `refcount`.
> > >> It dose however explain what you are going to do, so please keep it, but
> > >> not as a SAFETY comment.
> > >
> > > I'll reword.
> > >
> > >>> +        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
> > >>> +        if is_zero {
> > >>> +            // SAFETY: We have exclusive access to the arc, so we can perform unsynchronized
> > >>> +            // accesses to the refcount.
> > >>> +            unsafe { core::ptr::write(refcount, bindings::REFCOUNT_INIT(1)) };
> > >>> +
> > >>> +            // SAFETY: We own one refcount, so we can create a `UniqueArc`. It needs to be pinned,
> > >>> +            // since an `Arc` is pinned.
> > >>
> > >> The `unsafe` block is only needed due to the `new_unchecked` call, which
> > >> you could avoid by using `.into()`. The `SAFETY` should also be an
> > >> `INVARIANT` comment instead.
> > >>
> > >>> +            unsafe {
> > >>> +                Some(Pin::new_unchecked(UniqueArc {
> > >>> +                    inner: Arc::from_inner(me.ptr),
> > >>> +                }))
> > >>> +            }
> > >
> > > The from_inner method is also unsafe.
> >
> > Ah I missed that, might be a good reason to split the block.
> > It confused me that the SAFETY comment did not mention why calling
> > `new_unchecked` is sound.
>
> I don't mind splitting up the unsafe block into several pieces.
>
> > > I think that using new_unchecked here makes more sense. That method is
> > > usually used in the case where something is already pinned, whereas
> > > into() is usually used to pin something that was not previously
> > > pinned.
> >
> > I get your argument, but doing it this way avoids an unsafe function
> > call. I think it would be fine to use `.into()` in this case.
> > Splitting the unsafe block would also be fine with me.
>
> If you are okay with splitting the unsafe block instead, then I prefer
> that. I don't think avoiding unsafe blocks is always the best answer;
> especially not when you're already using unsafe right next to it.
>
> This reminds me of NonNull::new_unchecked(Box::into_raw(my_box)) vs
> NonNull::from(Box::leak(my_box)). The latter is safe, but I don't
> necessarily think that makes it the better choice. It's also important
> that your code carries the right intent.
>
> Another way to go around it could be to add UniqueArc::from_raw or
> from_inner methods, as well as from_raw_pinned and from_inner_pinned,
> and then use those here.

After looking at the code, I've changed my mind. I will write it like this:

Some(Pin::from(UniqueArc { inner: ManuallyDrop::into_inner(me) }))

With an INVARIANT comment. Does that make sense for you?

Alice

Re: [PATCH v2 2/2] rust: sync: add `Arc::into_unique_or_drop`

[Benno Lossin]

On 3/11/24 16:45, Alice Ryhl wrote:
> On Mon, Mar 11, 2024 at 4:35 PM Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> On Mon, Mar 11, 2024 at 4:15 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>>
>>> On 3/11/24 10:03, Alice Ryhl wrote:
>>>> On Sat, Mar 9, 2024 at 2:02 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>>>>
>>>>> On 2/28/24 14:00, Alice Ryhl wrote:
>>>>>> +        // SAFETY: If the refcount reaches a non-zero value, then we have destroyed this `Arc` and
>>>>>> +        // will return without further touching the `Arc`. If the refcount reaches zero, then there
>>>>>> +        // are no other arcs, and we can create a `UniqueArc`.
>>>>>
>>>>> This comment is not explaining why it is safe to call
>>>>> `refcount_dec_and_test` on `refcount`.
>>>>> It dose however explain what you are going to do, so please keep it, but
>>>>> not as a SAFETY comment.
>>>>
>>>> I'll reword.
>>>>
>>>>>> +        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
>>>>>> +        if is_zero {
>>>>>> +            // SAFETY: We have exclusive access to the arc, so we can perform unsynchronized
>>>>>> +            // accesses to the refcount.
>>>>>> +            unsafe { core::ptr::write(refcount, bindings::REFCOUNT_INIT(1)) };
>>>>>> +
>>>>>> +            // SAFETY: We own one refcount, so we can create a `UniqueArc`. It needs to be pinned,
>>>>>> +            // since an `Arc` is pinned.
>>>>>
>>>>> The `unsafe` block is only needed due to the `new_unchecked` call, which
>>>>> you could avoid by using `.into()`. The `SAFETY` should also be an
>>>>> `INVARIANT` comment instead.
>>>>>
>>>>>> +            unsafe {
>>>>>> +                Some(Pin::new_unchecked(UniqueArc {
>>>>>> +                    inner: Arc::from_inner(me.ptr),
>>>>>> +                }))
>>>>>> +            }
>>>>
>>>> The from_inner method is also unsafe.
>>>
>>> Ah I missed that, might be a good reason to split the block.
>>> It confused me that the SAFETY comment did not mention why calling
>>> `new_unchecked` is sound.
>>
>> I don't mind splitting up the unsafe block into several pieces.
>>
>>>> I think that using new_unchecked here makes more sense. That method is
>>>> usually used in the case where something is already pinned, whereas
>>>> into() is usually used to pin something that was not previously
>>>> pinned.
>>>
>>> I get your argument, but doing it this way avoids an unsafe function
>>> call. I think it would be fine to use `.into()` in this case.
>>> Splitting the unsafe block would also be fine with me.
>>
>> If you are okay with splitting the unsafe block instead, then I prefer
>> that. I don't think avoiding unsafe blocks is always the best answer;
>> especially not when you're already using unsafe right next to it.
>>
>> This reminds me of NonNull::new_unchecked(Box::into_raw(my_box)) vs
>> NonNull::from(Box::leak(my_box)). The latter is safe, but I don't
>> necessarily think that makes it the better choice. It's also important
>> that your code carries the right intent.
>>
>> Another way to go around it could be to add UniqueArc::from_raw or
>> from_inner methods, as well as from_raw_pinned and from_inner_pinned,
>> and then use those here.
> 
> After looking at the code, I've changed my mind. I will write it like this:
> 
> Some(Pin::from(UniqueArc { inner: ManuallyDrop::into_inner(me) }))
> 
> With an INVARIANT comment. Does that make sense for you?

That also looks good to me.

-- 
Cheers,
Benno