Re: [PATCH 2/3] rust: convert `Arc` to use `Refcount`

[Gary Guo <gary@garyguo.net>]

On Sat, 5 Oct 2024 14:06:48 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

> On Fri, Oct 4, 2024 at 5:53 PM Gary Guo <gary@garyguo.net> wrote:
> >
> > With `Refcount` type created, `Arc` can use `Refcount` instead of
> > calling into FFI directly.
> >
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Boqun Feng <boqun.feng@gmail.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Signed-off-by: Gary Guo <gary@garyguo.net>  
> 
> [...]
> 
> > -            // 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)) };
> > +            // We have exclusive access to the arc, so we can modify the refcount at will.
> > +            refcount.set(1);  
> 
> Why are you changing this to an atomic write? We just took ownership,
> so we have exclusive access and can perform an unsynchronized write.

Because I can avoid an unsafe, and use the new method. This is a
relaxed write, so I don't think there'll be any real difference.

> 
> >  impl<T: ?Sized> Drop for Arc<T> {
> >      fn drop(&mut self) {
> > -        // SAFETY: By the type invariant, there is necessarily a reference to the object. We cannot
> > -        // touch `refcount` after it's decremented to a non-zero value because another thread/CPU
> > -        // may concurrently decrement it to zero and free it. It is ok to have a raw pointer to
> > -        // freed/invalid memory as long as it is never dereferenced.
> > -        let refcount = unsafe { self.ptr.as_ref() }.refcount.get();
> > -
> >          // INVARIANT: If the refcount reaches zero, there are no other instances of `Arc`, and
> >          // this instance is being dropped, so the broken invariant is not observable.
> > -        // SAFETY: Also by the type invariant, we are allowed to decrement the refcount.
> > -        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
> > +        // SAFETY: By the type invariant, there is necessarily a reference to the object.
> > +        // NOTE: we cannot touch `refcount` after it's decremented to a non-zero value because
> > +        // another thread/CPU may concurrently decrement it to zero and free it. However it is okay
> > +        // to have a transient reference to decrement the refcount, see
> > +        // https://github.com/rust-lang/rust/issues/55005.
> > +        let is_zero = unsafe { self.ptr.as_ref().refcount.dec_and_test() };  
> 
> This code needs to make use of this guarantee for correctness:
> 
> For both `&T` without `UnsafeCell<_>` and `&mut T`, you must also not
> deallocate the data until the reference expires. As a special
> exception, given an `&T`, any part of it that is inside an
> `UnsafeCell<_>` may be deallocated during the lifetime of the
> reference, after the last time the reference is used (dereferenced or
> reborrowed). Since you cannot deallocate a part of what a reference
> points to, this means the memory an `&T` points to can be deallocated
> only if *every part of it* (including padding) is inside an
> `UnsafeCell`.
> 
> from https://doc.rust-lang.org/stable/std/cell/struct.UnsafeCell.html
> 
> so when invoking `dec_and_test()` you can have a reference to the
> `Refcount`, but not necessarily to other parts of `ArcInner` like you
> do here.

This is fine.

A reference only lives until it's last used. This has been the case
ever since NLL. This is totally legal code:

	let x = Box::new(42);
	let y = &*x;
	drop(x);

This is true for safe code and unsafe code. You can dereference a
pointer and hold onto a reference, and then free the pointer. As long
as you don't use that reference after it's freed, it's fine.

The only exception is when the reference is passed in through a
function parameter, and then the text you quoted matters. Normally these
references are assumed to be alive for the duration of function, except
when `UnsafeCell` is used, then this requirement is cancelled. I linked
to a closed Rust issue, and I think the paragraph you have quoted is a
direct result of it.

Now back to this particular case. Before calling `dec_and_test`, the
entire `ArcInner` is still alive, so it's valid to create a reference
to it. That reference is then used to derive a reference to the
`Refcount` field, which is needed for the function call. Then we call
`dec_and_test`. The call is okay because all bytes `Refcount` are
covered by `UnsafeCell`. The reference to `ArcInner` is never used
after the call, so we are all good.

In fact, the standard library also does
`self.inner().refcount.fetch_sub`, which also creates this transient
reference to `ArcInner`.

Best,
Gary