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-googOn 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
>
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
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
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
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
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
© 2016 - 2026 Red Hat, Inc.