Messages in this thread | | | Date | Fri, 2 Jun 2023 09:37:41 +0000 | Subject | Re: [PATCH v2 5/8] rust: workqueue: add helper for defining work_struct fields | From | Alice Ryhl <> |
| |
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
| |