[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241005141250.0fb0a3a9.gary@garyguo.net>
Date: Sat, 5 Oct 2024 14:12:50 +0100
From: Gary Guo <gary@...yguo.net>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Björn Roy Baron
<bjorn3_gh@...tonmail.com>, Benno Lossin <benno.lossin@...ton.me>, Andreas
Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>, Wedson
Almeida Filho <walmeida@...rosoft.com>, Valentin Obst
<kernel@...entinobst.de>, Alex Mantel <alexmantel93@...lbox.org>, Will
Deacon <will@...nel.org>, Peter Zijlstra <peterz@...radead.org>, Mark
Rutland <mark.rutland@....com>, Martin Rodriguez Reboredo
<yakoyoku@...il.com>, rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] rust: convert `Arc` to use `Refcount`
On Sat, 5 Oct 2024 14:06:48 +0200
Alice Ryhl <aliceryhl@...gle.com> wrote:
> On Fri, Oct 4, 2024 at 5:53 PM Gary Guo <gary@...yguo.net> wrote:
> >
> > With `Refcount` type created, `Arc` can use `Refcount` instead of
> > calling into FFI directly.
> >
> > Cc: Will Deacon <will@...nel.org>
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: Boqun Feng <boqun.feng@...il.com>
> > Cc: Mark Rutland <mark.rutland@....com>
> > Signed-off-by: Gary Guo <gary@...yguo.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
Powered by blists - more mailing lists