Received-SPF: pass (google.com: domain of linux-kernel+bounces-99138-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
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>
In-Reply-To: <7976d136-3cf7-427d-a047-41c29d25ad32@proton.me>
From: Alice Ryhl <aliceryhl@google.com>
Date: Mon, 11 Mar 2024 16:35:56 +0100
Message-ID: <CAH5fLghFKVBucpLGEXQ9CdRZRreEhT+u8tPFi+A9-+CX9_Mz1w@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] rust: sync: add `Arc::into_unique_or_drop`
To: 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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Mar 11, 2024 at 4:15=E2=80=AFPM Benno Lossin <benno.lossin@proton.m=
e> 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@proto=
n.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 re=
fcount 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, b=
ut
> >> not as a SAFETY comment.
> >
> > I'll reword.
> >
> >>> +        let is_zero =3D unsafe { bindings::refcount_dec_and_test(ref=
count) };
> >>> +        if is_zero {
> >>> +            // SAFETY: We have exclusive access to the arc, so we ca=
n perform unsynchronized
> >>> +            // accesses to the refcount.
> >>> +            unsafe { core::ptr::write(refcount, bindings::REFCOUNT_I=
NIT(1)) };
> >>> +
> >>> +            // SAFETY: We own one refcount, so we can create a `Uniq=
ueArc`. It needs to be pinned,
> >>> +            // since an `Arc` is pinned.
> >>
> >> The `unsafe` block is only needed due to the `new_unchecked` call, whi=
ch
> >> 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