Re: [PATCH v3 2/2] rust: workqueue: add creation of workqueues

[Alice Ryhl <aliceryhl@google.com>]

On Sat, Feb 28, 2026 at 03:43:02PM +0100, Danilo Krummrich wrote:
> On Sat Feb 28, 2026 at 1:59 PM CET, Alice Ryhl wrote:
> > On Fri, Feb 27, 2026 at 08:23:44PM +0100, Danilo Krummrich wrote:
> >> On Fri Feb 27, 2026 at 8:05 PM CET, Alice Ryhl wrote:
> >> > On Fri, Feb 27, 2026 at 04:30:59PM +0100, Danilo Krummrich wrote:
> >> >> On Fri Feb 27, 2026 at 3:53 PM CET, Alice Ryhl wrote:
> >> >> > +    #[inline]
> >> >> > +    pub fn max_active(mut self, max_active: u32) -> Builder {
> >> >> > +        self.max_active = i32::try_from(max_active).unwrap_or(i32::MAX);
> >> >> 
> >> >> The workqueue code prints a warning for max_active >  WQ_MAX_ACTIVE. Maybe use
> >> >> debug_assert()?
> >> >
> >> > What's wrong with just making use of the C-side warning?
> >> 
> >> IIRC, we have the same pattern in other Rust code that we use debug_assert()
> >> when a value got clamped, e.g. in udelay().
> >
> > In udelay(), the clamping happens on the Rust side, so it makes sense
> > that Rust is the one to warn about it.
> >
> > Here, the clamping happens in C code. To warn about it, I'd have to
> > duplicate the existing C-side check to clamp in Rust.
> 
> That's fair, although I also think that it is not unreasonable. Given that this
> uses the builder pattern, I think it would be nice to ensure that nothing
> "invalid" can be built in the first place.
> 
> Maybe we can use a bounded integer?

Bounded integers allow zero, which is also illegal.

I think it's a bit much honestly.

> >> >> It's also a bit unfortunate that alloc_ordered_workqueue() becomes
> >> >> .max_active(1).
> >> >> 
> >> >> At the same time having a separate ordered() method competes with max_active().
> >> >> 
> >> >> Mybe a type state, i.e. Builder<Ordered> that doesn't have max_active()?
> >> >
> >> > Sorry I'm a bit confused by this. Why does an ordered() compete with
> >> > max_active()?
> >> 
> >> Because you could get an inconsistent state with __WQ_ORDERED and
> >> max_active > 1.
> >> 
> >> It also conflicts with sysfs() I think [1].
> >> 
> >> [1] https://elixir.bootlin.com/linux/v6.19.3/source/kernel/workqueue.c#L7417
> >
> > And I guess the further argument is that we have a use-case for ordered
> > workqueues?
> 
> In the context of
> 
> 	GPU drivers often need to create their own workqueues for various
> 	reasons. Add the ability to do so.
> 
> I think we do.
> 
> Depending on the final implementation details and the driver it may be needed by
> the job queue.
> 
> They are also pretty common outside the scheduler use-case in GPU drivers. I
> think panthor has one as well, so you might also need one in Tyr. In nova-core I
> expect this to be used in MM code.
> 
> But even without that, I think it would be reasonble to consider ordered queues
> for this abstraction, since alloc_ordered_workqueue() and
> create_singlethread_workqueue() seem to have more users than the non-ordered
> constructors (without checking whether alloc_workqueue() is also used directly
> to create ordered queues).

Ok, I'll consider this for next version.

Alice