Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Jul 2023 14:49:04 +0000
In-Reply-To: <vyrVN334oIXneb9VWievGpPTOCy0irxvoG-mDR0sQX5-xN-TL2Hngl0cl_XrVUwjDB3rXkXfR9Mdjy_no6fzhe4yIf_c7xpKa5OURt43k9c=@proton.me>
Mime-Version: 1.0
References: <vyrVN334oIXneb9VWievGpPTOCy0irxvoG-mDR0sQX5-xN-TL2Hngl0cl_XrVUwjDB3rXkXfR9Mdjy_no6fzhe4yIf_c7xpKa5OURt43k9c=@proton.me>
Message-ID: <20230717144904.1857663-1-aliceryhl@google.com>
Subject: Re: [PATCH v3 6/9] rust: workqueue: add helper for defining
 work_struct fields
From:   Alice Ryhl <aliceryhl@google.com>
To:     benno.lossin@proton.me
Cc:     alex.gaynor@gmail.com, aliceryhl@google.com,
        bjorn3_gh@protonmail.com, boqun.feng@gmail.com, gary@garyguo.net,
        jiangshanlai@gmail.com, linux-kernel@vger.kernel.org,
        ojeda@kernel.org, patches@lists.linux.dev,
        rust-for-linux@vger.kernel.org, tj@kernel.org, wedsonaf@gmail.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Benno Lossin <benno.lossin@proton.me> writes:
>> +/// Links for a work item.
>> +///
>> +/// This struct contains a function pointer to the `run` function from the [`WorkItemPointer`]
>> +/// trait, and defines the linked list pointers necessary to enqueue a work item in a workqueue.
>> +///
>> +/// Wraps the kernel's C `struct work_struct`.
>> +///
>> +/// This is a helper type used to associate a `work_struct` with the [`WorkItem`] that uses it.
>> +#[repr(transparent)]
>> +pub struct Work<T: ?Sized, const ID: u64 = 0> {
>> +    work: Opaque<bindings::work_struct>,
>> +    _inner: PhantomData<T>,
>
> Should this really be `PhantomData<T>`? Are you dropping `T`s in the 
> destructor of `Work<T>`? I do not think so `PhantomData<fn() -> Box<T>>`
> should do the trick.

Hmm. The `work_struct` effectively has a field of type `fn(T::Pointer)`,
so arguably, that's the type that should be used for the `PhantomData`.
This makes `Work` contravariant instead.

That said, the type `T` is going to be the struct itself, so, for
example, a `MyStruct<'a>` might have a `&'a i32` and a
`Work<MyStruct<'a>>` field, so it ends up being invariant.

By that argument, perhaps we should just make `Work` invariant instead?
We can do that with `PhantomData<*mut T>`.

>> +pub unsafe trait HasWork<T, const ID: u64 = 0> {
> 
> Is there a reason that `T` is `Sized` here?

Hmm, that's an oversight. In v1 of this patchset, the Sized requirement
was correct since T was the pointer type, but now it isn't anymore.

Alice