On 01.02.24 10:41, Alice Ryhl wrote: > 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. That would fit better as the safety comment for `Sync`, since it refers to "references". For `Send` I think this would be better: // SAFETY: // - `File::dec_ref` can be called from any thread. // - It is okay to send ownership of `File` across thread boundaries. -- Cheers, Benno
