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. -- Cheers, Benno >>> 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.
