Message-ID: <Z8LAFz_Q07qhio-O@laptop>
Date: Sat, 01 Mar 2025 08:06:49 +0000
From: Oliver Mangold <oliver.mangold@...me>
To: Benoît du Garreau <benoit@...arreau.fr>
Cc: Benoît du Garreau <bdgdlm@...look.com>, Andreas Hindborg <a.hindborg@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Benno Lossin <benno.lossin@...ton.me>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH] rust: adding UniqueRefCounted and UniqueRef types

On 250301 0041, Benoît du Garreau wrote:
> From: Benoît du Garreau <bdgdlm@...look.com>
> 
> It would be great for this trait to only have a `is_unique` method, and that functions here
> do the actual work. It would make it easier to implement and would avoid duplicating this
> work.
Hi,

I see where you are coming from, but there are good reasons for it to be like this.

mq::Request (for which this work was initially done) has the surprising property,
that it is possible to create a reference to it 'out of band'
(i.e. without deriving it from an existing one) through mq::TagSet::tag_to_rq().

As this means try_shared_to_unique() to an UniqueRef can race
with tag_to_rq(), a non-standard reference counting scheme needs to be used
which can distinguish between 'a single ARef exists' and 'an UniqueRef exists',
and requires the is_unique() check and the necessary modification of the
reference count in try_shared_to_unique() have to be done
in one combined atomic operation.

Of course the whole thing could be refactored that both ways work, this one
and one like you suggest with just a to_unique(). I will think about it
for a bit, but I'm not sure it is worth the effort.

What probably makes sense is to at least have a default implementation
for unique_to_shared() which just does a
ARef::from_raw(UniqueRef::into_raw()) leaving only try_shared_to_unique()
as mandatory to implemented.

Thoughts?
> Maybe this could even be a new method on `AlwaysRefCounted`?
>
I didn't do that intentionally, because I think for many AlwaysRefCounted
implementors it would be a pain, as they simply use some get() and put()
methods to inc/dec the refcount which are provided by the C API,
while the actual refcount stays hidden from them.
> 
> I think you meant "no other refcount" or "only references borrowed from this".
>
Yes, you are right. Thanks. I will fix that.
> 
> `UniqueRef` is essentially a `Box`, so it should have the same `Send`/`Sync` implementations. Here
> I don't see how sending a `UniqueRef<T>` is sharing a `&T`.
> 
> Same here: you can only get a `&T` from a `UniqueRef<T>`, definitely not clone it.
>
I just copy/pasted that from ARef. You might be right, I have to think about that when
my minds is less foggy than right now :)
> > +
> > +
> > +impl<T: UniqueRefCounted> From<&T> for UniqueRef<T> {
> > +    /// Converts the [`UniqueRef`] into an [`ARef`]
> > +    /// by calling [`UniqueRefCounted::unique_to_shared`] on it.
> > +    fn from(b: &T) -> Self {
> > +        b.inc_ref();
> > +        // SAFETY: We just incremented the refcount above.
> > +        unsafe { Self::from_raw(NonNull::from(b)) }
> > +    }
> > +}
> 
> This is wrong: the reference borrows from a refcount (as per `AlwaysRefCounted`), and this
> method increments it once more. It cannot be unique when the function returns.
> Actually the only way such conversion could be written is by cloning `T`, which is probably
> not what we want.
> 
Good catch. That was also an relict of copy-paste which I missed. The method should
just be deleted. I will do that.

Best,

Oliver

Powered by blists - more mailing lists