Received-SPF: pass (google.com: domain of linux-kernel+bounces-99152-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 11 Mar 2024 15:47:35 +0000
To: Alice Ryhl <aliceryhl@google.com>
From: Benno Lossin <benno.lossin@proton.me>
Cc: Miguel Ojeda <ojeda@kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>, Wedson Almeida Filho <wedsonaf@gmail.com>, Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, =?utf-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>, Andreas Hindborg <a.hindborg@samsung.com>, rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] rust: sync: add `Arc::into_unique_or_drop`
Message-ID: <47d24edd-6252-445b-a46e-6eb450f7786b@proton.me>
In-Reply-To: <CAH5fLgj-CH+DmvUjPe2Kacs6QQUQ0qj8ctFrx_jjJg-w1Xrc_w@mail.gmail.com>
References: <20240228-arc-for-list-v2-0-ae93201426b4@google.com> <20240228-arc-for-list-v2-2-ae93201426b4@google.com> <d52c5d95-99cd-4bd4-864f-b704299d3b1b@proton.me> <CAH5fLggrRANz1GrjEa671Vj0m9=UDeEcGV5vhOxq8XtR6EjUSg@mail.gmail.com> <7976d136-3cf7-427d-a047-41c29d25ad32@proton.me> <CAH5fLghFKVBucpLGEXQ9CdRZRreEhT+u8tPFi+A9-+CX9_Mz1w@mail.gmail.com> <CAH5fLgj-CH+DmvUjPe2Kacs6QQUQ0qj8ctFrx_jjJg-w1Xrc_w@mail.gmail.com>
Feedback-ID: 71780778:user:proton
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On 3/11/24 16:45, Alice Ryhl wrote:
> On Mon, Mar 11, 2024 at 4:35=E2=80=AFPM Alice Ryhl <aliceryhl@google.com>=
 wrote:
>>
>> On Mon, Mar 11, 2024 at 4:15=E2=80=AFPM Benno Lossin <benno.lossin@proto=
n.me> wrote:
>>>
>>> On 3/11/24 10:03, Alice Ryhl wrote:
>>>> On Sat, Mar 9, 2024 at 2:02=E2=80=AFPM Benno Lossin <benno.lossin@prot=
on.me> wrote:
>>>>>
>>>>> On 2/28/24 14:00, Alice Ryhl wrote:
>>>>>> +        // SAFETY: If the refcount reaches a non-zero value, then w=
e have destroyed this `Arc` and
>>>>>> +        // will return without further touching the `Arc`. If the r=
efcount 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 =3D unsafe { bindings::refcount_dec_and_test(re=
fcount) };
>>>>>> +        if is_zero {
>>>>>> +            // SAFETY: We have exclusive access to the arc, so we c=
an 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 `Uni=
queArc`. It needs to be pinned,
>>>>>> +            // since an `Arc` is pinned.
>>>>>
>>>>> The `unsafe` block is only needed due to the `new_unchecked` call, wh=
ich
>>>>> 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.
>=20
> After looking at the code, I've changed my mind. I will write it like thi=
s:
>=20
> Some(Pin::from(UniqueArc { inner: ManuallyDrop::into_inner(me) }))
>=20
> With an INVARIANT comment. Does that make sense for you?

That also looks good to me.

--=20
Cheers,
Benno