Re: [PATCH v4 3/3] rust: workqueue: add creation of workqueues

[Alice Ryhl <aliceryhl@google.com>]

On Thu, Mar 12, 2026 at 04:39:00PM +0000, Gary Guo wrote:
> On Thu Mar 12, 2026 at 9:23 AM GMT, Alice Ryhl wrote:
> > Creating workqueues is needed by various GPU drivers. Not only does it
> > give you better control over execution, it also allows devices to ensure
> > that all tasks have exited before the device is unbound (or similar) by
> > running the workqueue destructor.
> >
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>

> > +use core::{
> > +    marker::PhantomData, //
> > +    ptr::{self, NonNull},
> 
> This is formatted incorrectly.

Sigh. Ok.

> > +};
> > +
> > +/// Workqueue builder.
> > +///
> > +/// A valid combination of workqueue flags contains one of the base flags (`WQ_UNBOUND`, `WQ_BH`,
> > +/// or `WQ_PERCPU`) and a combination of modifier flags that are compatible with the selected base
> > +/// flag.
> > +///
> > +/// For details, please refer to `Documentation/core-api/workqueue.rst`.
> > +pub struct Builder<T> {
> 
> I would name the generic parameter `Kind` rather than `T`. `T` is too generic to
> indicate it's for specific kind/type of workqueue.

I guess 'Kind' works here, yeah.

> > +    flags: bindings::wq_flags,
> > +    max_active: i32,
> > +    _type: PhantomData<T>,
> 
> Hmm, it is somewhat awkward to me that we are having a `PhantomData<T>` here,
> as `PhantomData` is documented to "behave as it it owns a `T`", but all the
> possible `T`s that we use here are uninhabited.

I don't think this is an issue.

> > +    /// Build a single-threaded workqueue that executes jobs in order.
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// use kernel::workqueue::Queue;
> > +    ///
> > +    /// let wq = Queue::new_ordered().build(c"my-wq")?;
> > +    /// wq.try_spawn(GFP_KERNEL, || pr_info!("Hello from ordered wq"))?;
> > +    /// # Ok::<(), Error>(())
> > +    /// ```
> > +    #[inline]
> > +    #[doc(alias = "alloc_ordered_workqueue")]
> > +    #[doc(alias = "__WQ_ORDERED")]
> > +    pub fn new_ordered() -> Builder<TypeOrdered> {
> > +        Builder {
> > +            flags: bindings::wq_flags_WQ_UNBOUND | bindings::wq_flags___WQ_ORDERED,
> > +            max_active: 0,
> 
> This should be 1 instead of 0.

Hmm, it's weird that the doc-test didn't catch this.

> > +/// An owned kernel work queue.
> > +///
> > +/// Dropping a workqueue blocks on all pending work.
> > +///
> > +/// # Invariants
> > +///
> > +/// `queue` points at a valid workqueue that is owned by this `OwnedQueue`.
> > +pub struct OwnedQueue {
> > +    queue: NonNull<Queue>,
> > +}
> 
> This looks like something that can become `Owned<Queue>` when Andreas' series
> land?

That's right.

Alice