Re: [PATCH v2 5/8] rust: workqueue: add helper for defining work_struct fields

Alice Ryhl
2023-06-02T09:37:52Z

Boqun Feng <boqun.feng@gmail.com> writes:
> On Thu, Jun 01, 2023 at 01:49:43PM +0000, Alice Ryhl wrote:
>> +/// Defines the method that should be called directly when a work item is executed.
>> +///
>> +/// Typically you would implement [`WorkItem`] instead. The `run` method on this trait will
>> +/// usually just perform the appropriate `container_of` translation and then call into the `run`
>> +/// method from the [`WorkItem`] trait.
>> +///
>> +/// This trait is used when the `work_struct` field is defined using the [`Work`] helper.
>> +///
>> +/// # Safety
>> +///
>> +/// Implementers must ensure that [`__enqueue`] uses a `work_struct` initialized with the [`run`]
>> +/// method of this trait as the function pointer.
>> +///
>> +/// [`__enqueue`]: RawWorkItem::__enqueue
>> +/// [`run`]: WorkItemPointer::run
>> +pub unsafe trait WorkItemPointer<const ID: u64>: RawWorkItem<ID> {
>> + /// Run this work item.
>> + ///
>> + /// # Safety
>> + ///
>> + /// The provided `work_struct` pointer must originate from a previous call to `__enqueue` where
>> + /// the `queue_work_on` closure returned true, and the pointer must still be valid.
>> + unsafe extern "C" fn run(ptr: *mut bindings::work_struct);
>> +}
>> +
>> +/// Defines the method that should be called when this work item is executed.
>> +///
>> +/// This trait is used when the `work_struct` field is defined using the [`Work`] helper.
>> +pub trait WorkItem<const ID: u64 = 0> {
>> + /// The pointer type that this struct is wrapped in. This will typically be `Arc<Self>` or
>> + /// `Pin<Box<Self>>`.
>> + type Pointer: WorkItemPointer<ID>;
>
> This being an associate type makes me wonder how do we want to support
> the following (totally made-up by me, but I think it makes sense)?:
>
> Say we have a struct
>
> pub struct Foo {
> work: Work<Foo>,
> data: Data,
> }
>
> impl Foo {
> pub fn do_sth(&self) {
> ...
> }
> }
>
> and we want to queue both Pin<Box<Foo>> and Arc<Foo> as work items, but
> the following doesn't work:
>
> // Pin<Box<Foo>> can be queued.
> impl WorkItem for Foo {
> type Pointer = Pin<Box<Foo>>;
> fn run(ptr: Self::Pointer) {
> ptr.do_sth();
> }
> }
>
> // Arc<Foo> can also be queued.
> impl WorkItem for Foo {
> type Pointer = Arc<Foo>;
> fn run(ptr: Self::Pointer) {
> ptr.do_sth();
> }
> }
>
> Of course, we can use new type idiom, but that's not really great, and
> we may have more smart pointer types in the future.
>
> Am I missing something here?

Basically, you're asking "is it possible to use the same `work_struct`
field for several different pointer types"?

When creating the function pointer to store in the `work_struct`, the
function pointer _must_ know what the pointer type is. Otherwise it
cannot call the right `WorkItem::run` method or perform the correct
`container_of` operation. (E.g. an `Arc` embeds a `refcount_t` before
the struct, but a `Box` does not.)

With an associated type there is no problem with that. Associated types
force you to make a choice, which means that the `work_struct` knows
what the pointer type is when you create it. Supporting what you suggest
means that we must be able to change the function pointer stored in the
`work_struct` after initializing it.

This is rather tricky because you can call `enqueue` from several
threads in parallel; just setting the function pointer before you call
`queue_work_on` would be a data race. I suppose you could do it by
implementing our own `queue_work_on` that sets the function pointer
_after_ successfully setting the `WORK_STRUCT_PENDING_BIT`, but I don't
think this patchset should do that.

Alice