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

[Gary Guo]

On Fri Mar 13, 2026 at 1:25 PM GMT, Gary Guo wrote:
> On Thu Mar 12, 2026 at 10:56 PM GMT, Alice Ryhl wrote:
>> On Thu, Mar 12, 2026 at 04:39:00PM +0000, Gary Guo wrote:
>>> > +    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.
>
> Fair. I also don't think it's an issue, just awkward.
>
> I would rather make the uninhabited enums be just unit structs, though (like what we do
> for atomic ordering and device contexts).

One interesting idea with unit structs, is to allow the type to be passed in as
argument rather than having multiple new_ variants:

    Queue::builder(wq::TypeUnbound).max_active(..).build(...)

This is the trick that we use to make atomic API looks nice, although in this
case it seems that it's not worth the hassle as it needs an extra import...

Best,
Gary

>
>>
>>> > +    /// 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.
>
> It seems that there's no check for this in the __alloc_workqueue (which I guess
> makes sense, because there's a C macro for allocating ordered workqueue which
> doesn't take max_active and pass 1 along).
>
> The only assertion is in workqueue_set_max_active to prevent changing it after
> creation.
>
> Best,
> Gary
>
>>
>>> > +/// 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