Message-ID: <nFDPJFnzE9Q5cqY7FwSMByRH2OAn_BpI4H53NQfWIlN6I2qfmAqnkp2wRqn0XjMO65OyZY4h6P4K2nAGKJpAOSzksYXaiAK_FoH_8QbgBI4=@proton.me>
Date:   Thu, 30 Nov 2023 12:17:14 +0000
From:   Benno Lossin <benno.lossin@...ton.me>
To:     Alice Ryhl <aliceryhl@...gle.com>
Cc:     brauner@...nel.org, a.hindborg@...sung.com, alex.gaynor@...il.com,
        arve@...roid.com, bjorn3_gh@...tonmail.com, boqun.feng@...il.com,
        cmllamas@...gle.com, dan.j.williams@...el.com, dxu@...uu.xyz,
        gary@...yguo.net, gregkh@...uxfoundation.org,
        joel@...lfernandes.org, keescook@...omium.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        maco@...roid.com, ojeda@...nel.org, peterz@...radead.org,
        rust-for-linux@...r.kernel.org, surenb@...gle.com,
        tglx@...utronix.de, tkjos@...roid.com, viro@...iv.linux.org.uk,
        wedsonaf@...il.com, willy@...radead.org
Subject: Re: [PATCH 4/7] rust: file: add `FileDescriptorReservation`

On 30.11.23 12:54, Alice Ryhl wrote:
> Christian Brauner <brauner@...nel.org> writes:
>> On Thu, Nov 30, 2023 at 09:17:56AM +0000, Alice Ryhl wrote:
>>> Christian Brauner <brauner@...nel.org> writes:
>>>>>>> +    /// Prevent values of this type from being moved to a different task.
>>>>>>> +    ///
>>>>>>> +    /// This is necessary because the C FFI calls assume that `current` is set to the task that
>>>>>>> +    /// owns the fd in question.
>>>>>>> +    _not_send_sync: PhantomData<*mut ()>,
>>>>>>
>>>>>> I don't fully understand this. Can you explain in a little more detail
>>>>>> what you mean by this and how this works?
>>>>>
>>>>> Yeah, so, this has to do with the Rust trait `Send` that controls
>>>>> whether it's okay for a value to get moved from one thread to another.
>>>>> In this case, we don't want it to be `Send` so that it can't be moved to
>>>>> another thread, since current might be different there.
>>>>>
>>>>> The `Send` trait is automatically applied to structs whenever *all*
>>>>> fields of the struct are `Send`. So to ensure that a struct is not
>>>>> `Send`, you add a field that is not `Send`.
>>>>>
>>>>> The `PhantomData` type used here is a special zero-sized type.
>>>>> Basically, it says "pretend this struct has a field of type `*mut ()`,
>>>>> but don't actually add the field". So for the purposes of `Send`, it has
>>>>> a non-Send field, but since its wrapped in `PhantomData`, the field is
>>>>> not there at runtime.
>>>>
>>>> This probably a stupid suggestion, question. But while PhantomData gives
>>>> the right hint of what is happening I wouldn't mind if that was very
>>>> explicitly called NoSendTrait or just add the explanatory comment. Yes,
>>>> that's a lot of verbiage but you'd help us a lot.
>>>
>>> I suppose we could add a typedef:
>>>
>>> type NoSendTrait = PhantomData<*mut ()>;
>>>
>>> and use that as the field type. The way I did it here is the "standard"
>>> way of doing it, and if you look at code outside the kernel, you will
>>> also find them using `PhantomData` like this. However, I don't mind
>>> adding the typedef if you think it is helpful.
>>
>> I'm fine with just a comment as well. I just need to be able to read
>> this a bit faster. I'm basically losing half a day just dealing with
>> this patchset and that's not realistic if I want to keep up with other
>> patches that get sent.
>>
>> And if you resend and someone else review you might have to answer the
>> same question again.
> 
> What do you think about this wording?
> 
> /// Prevent values of this type from being moved to a different task.
> ///
> /// This field has the type `PhantomData<*mut ()>`, which does not
> /// implement the Send trait. By adding a field with this property, we
> /// ensure that the `FileDescriptorReservation` struct will not
> /// implement the Send trait either. This has the consequence that the
> /// compiler will prevent you from moving values of type
> /// `FileDescriptorReservation` into a different task, which we want
> /// because other tasks might have a different value of `current`. We
> /// want to avoid that because `fd_install` assumes that the value of
> /// `current` is unchanged since the call to `get_unused_fd_flags`.
> ///
> /// The `PhantomData` type has size zero, so the field does not exist at
> /// runtime.
> 
> Alice

I don't think it is a good idea to add this big comment to every
`PhantomData` field. I would much rather have a type alias:

    /// Zero-sized type to mark types not [`Send`].
    ///
    /// Add this type as a field to your struct if your type should not be sent to a different task.
    /// Since [`Send`] is an auto trait, adding a single field that is [`!Send`] will ensure that the
    /// whole type is [`!Send`].
    ///
    /// If a type is [`!Send`] it is impossible to give control over an instance of the type to another
    /// task. This is useful when a type stores task-local information for example file descriptors.
    pub type NotSend = PhantomData<*mut ()>;

If you have suggestions for improving the doc comment or the name,
please go ahead.

This doesn't mean that there should be no comment on the `NotSend`
field of `FileDescriptorReservation`, but I don't want to repeat
the `Send` stuff all over the place (since it comes up a lot):

    /// Ensure that `FileDescriptorReservation` cannot be sent to a different task, since there the
    /// value of `current` is different. We want to avoid that because `fd_install` assumes that the
    /// value of `current` is unchanged since the call to `get_unused_fd_flags`.
    _not_send: NotSend,

-- 
Cheers,
Benno

Powered by blists - more mailing lists