On 29.01.24 17:34, Alice Ryhl wrote: > On Fri, Jan 26, 2024 at 4:04 PM Benno Lossin <benno.lossin@xxxxxxxxx> wrote: >>> +/// closed. >>> +/// * A light refcount must be dropped before returning to userspace. >>> +#[repr(transparent)] >>> +pub struct File(Opaque<bindings::file>); >>> + >>> +// SAFETY: By design, the only way to access a `File` is via an immutable reference or an `ARef`. >>> +// This means that the only situation in which a `File` can be accessed mutably is when the >>> +// refcount drops to zero and the destructor runs. It is safe for that to happen on any thread, so >>> +// it is ok for this type to be `Send`. >> >> Technically, `drop` is never called for `File`, since it is only used >> via `ARef<File>` which calls `dec_ref` instead. Also since it only contains >> an `Opaque`, dropping it is a noop. >> But what does `Send` mean for this type? Since it is used together with >> `ARef`, being `Send` means that `File::dec_ref` can be called from any >> thread. I think we are missing this as a safety requirement on >> `AlwaysRefCounted`, do you agree? >> I think the safety justification here could be (with the requirement added >> to `AlwaysRefCounted`): >> >> SAFETY: >> - `File::drop` can be called from any thread. >> - `File::dec_ref` can be called from any thread. > > This wording was taken from rust/kernel/task.rs. I think it's out of > scope to reword it. Rewording the safety docs on `AlwaysRefCounted`, yes that is out of scope, I was just checking if you agree that the current wording is incomplete. > Besides, it says "destructor runs", not "drop runs". The destructor > can be interpreted to mean the right thing for ARef. To me "destructor runs" and "drop runs" are synonyms. > The right safety comment would probably be that dec_ref can be called > from any thread. Yes and no, I would prefer if you could remove the "By design, ..." part and only focus on `dec_ref` being callable from any thread and it being ok to send a `File` to a different thread. -- Cheers, Benno
