Received-SPF: pass (google.com: domain of linux-kernel+bounces-99149-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
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> <CAH5fLghFKVBucpLGEXQ9CdRZRreEhT+u8tPFi+A9-+CX9_Mz1w@mail.gmail.com>
In-Reply-To: <CAH5fLghFKVBucpLGEXQ9CdRZRreEhT+u8tPFi+A9-+CX9_Mz1w@mail.gmail.com>
From: Alice Ryhl <aliceryhl@google.com>
Date: Mon, 11 Mar 2024 16:45:24 +0100
Message-ID: <CAH5fLgj-CH+DmvUjPe2Kacs6QQUQ0qj8ctFrx_jjJg-w1Xrc_w@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:35=E2=80=AFPM Alice Ryhl <aliceryhl@google.com> w=
rote:
>
> On Mon, Mar 11, 2024 at 4:15=E2=80=AFPM Benno Lossin <benno.lossin@proton=
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@pro=
ton.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 =3D unsafe { bindings::refcount_dec_and_test(r=
efcount) };
> > >>> +        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 `Un=
iqueArc`. It needs to be pinned,
> > >>> +            // since an `Arc` is pinned.
> > >>
> > >> The `unsafe` block is only needed due to the `new_unchecked` call, w=
hich
> > >> 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 i=
s
> > > 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