On Thu, Feb 1, 2024 at 10:38 AM Benno Lossin <benno.lossin@xxxxxxxxx> wrote: > > On 01.02.24 10:33, Alice Ryhl wrote: > > 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`. > > Ah I see. But I still think changing it is better, since it would only get > shorter. The comment on `Task` can be fixed later. > Or do you want to keep consistency here? Because I would prefer to make > this right and then change `Task` later. What would you like me to change it to? For example: // SAFETY: It is okay to send references to a File across thread boundaries. Alice
