On Thu, Feb 1, 2024 at 10:31 AM Benno Lossin <benno.lossin@xxxxxxxxx> wrote: > > 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. That's not what I meant. The wording of this safety comment is identical to the wording in other existing safety comments in the kernel, such as e.g. the one for `impl Send for Task`. > > 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.
