[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230717144904.1857663-1-aliceryhl@google.com>
Date: Mon, 17 Jul 2023 14:49:04 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: benno.lossin@...ton.me
Cc: alex.gaynor@...il.com, aliceryhl@...gle.com,
bjorn3_gh@...tonmail.com, boqun.feng@...il.com, gary@...yguo.net,
jiangshanlai@...il.com, linux-kernel@...r.kernel.org,
ojeda@...nel.org, patches@...ts.linux.dev,
rust-for-linux@...r.kernel.org, tj@...nel.org, wedsonaf@...il.com
Subject: Re: [PATCH v3 6/9] rust: workqueue: add helper for defining
work_struct fields
Benno Lossin <benno.lossin@...ton.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
Powered by blists - more mailing lists