Creation of workqueues in Rust

When a workqueue is shut down, delayed work that is pending but not scheduled does not get properly cleaned up, so it's not safe to use enqueue_delayed on a workqueue that might be destroyed. To fix this, restricted enqueue_delayed to static queues.

Cc [email protected]

Fixes 7c098cd5eaae ("workqueue: rust: add delayed work items")

Reviewed-by John Hubbard <[email protected]>

Signed-off-by Alice Ryhl <[email protected]>

---
 rust/kernel/workqueue.rs | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

rust/kernel/workqueue.rs +0 -0

index 706e833e9702..1acd113c04ee 100644

rust/kernel/workqueue.rs +6 -1

+    ///
     /// The work item will be submitted using `WORK_CPU_UNBOUND`.
-    pub fn enqueue_delayed<W, const ID: u64>(&self, w: W, delay: Jiffies) -> W::EnqueueOutput
+    pub fn enqueue_delayed<W, const ID: u64>(
+        &'static self,
+        w: W,
+        delay: Jiffies,
+    ) -> W::EnqueueOutput
     where
         W: RawDelayedWorkItem<ID> + Send + 'static,
     {

Signature · 2 lines

-- 
2.53.0.473.g4a7958ca14-goog

When a workqueue is shut down, delayed work that is pending but not scheduled does not get properly cleaned up, so it's not safe to use enqueue_delayed on a workqueue that might be destroyed. To fix this, restricted enqueue_delayed to static queues.

Cc [email protected]

Fixes 7c098cd5eaae ("workqueue: rust: add delayed work items")

Reviewed-by John Hubbard <[email protected]>

Signed-off-by Alice Ryhl <[email protected]>

---
 rust/kernel/workqueue.rs | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

When a workqueue is shut down, delayed work that is pending but not scheduled does not get properly cleaned up, so it's not safe to use enqueue_delayed on a workqueue that might be destroyed. To fix this, restricted enqueue_delayed to static queues.

Quoted 4 lines · Alice Ryhl

When a workqueue is shut down, delayed work that is pending but not scheduled does not get properly cleaned up, so it's not safe to use enqueue_delayed on a workqueue that might be destroyed. To fix this, restricted enqueue_delayed to static queues.

C being C, we've been just chalking this up as "user error", but please feel free to add per-workqueue percpu ref for pending delayed work items if that'd help. That shouldn't be noticeably expensive and should help straighten this out for rust hopefully.

Quoted 10 lines · Tejun Heo

Quoted 4 lines · Alice Ryhl

When a workqueue is shut down, delayed work that is pending but not scheduled does not get properly cleaned up, so it's not safe to use enqueue_delayed on a workqueue that might be destroyed. To fix this, restricted enqueue_delayed to static queues.

C being C, we've been just chalking this up as "user error", but please feel free to add per-workqueue percpu ref for pending delayed work items if that'd help. That shouldn't be noticeably expensive and should help straighten this out for rust hopefully.

I had been thinking I would pick up this patch again:
https://lore.kernel.org/all/[email protected]/

but it sounds like you're suggesting a different solution?

Quoted 15 lines · Alice Ryhl

Quoted 10 lines · Tejun Heo

Quoted 4 lines · Alice Ryhl

When a workqueue is shut down, delayed work that is pending but not scheduled does not get properly cleaned up, so it's not safe to use enqueue_delayed on a workqueue that might be destroyed. To fix this, restricted enqueue_delayed to static queues.

C being C, we've been just chalking this up as "user error", but please feel free to add per-workqueue percpu ref for pending delayed work items if that'd help. That shouldn't be noticeably expensive and should help straighten this out for rust hopefully.

I had been thinking I would pick up this patch again:
https://lore.kernel.org/all/[email protected]/

Quoted 1 line

but it sounds like you're suggesting a different solution?

I'm not remembering much context at this point, but if it *could* work, percpu refcnt counting the number of delayed work items would be cheaper. Again, I could easily be forgetting why we didn't do that in the first place.

I guess the question is, what does destroy_workqueue() do?

- Does it wait for the timers to finish?
- Does it immediately run the delayed works?
- Does it exit without waiting for timers?

It sounds like the refcount approach is the last solution, where destroy_workqueue() just exits without waiting for timers, but then keeping the workqueue alive until the timers elapse.

The main concern I can see is that this means that delayed work can run after destroy_workqueue() is called. That may be a problem if destroy_workqueue() is used to guard module unload (or device unbind).

Hello,

Quoted 10 lines · Alice Ryhl

I guess the question is, what does destroy_workqueue() do?

- Does it wait for the timers to finish?
- Does it immediately run the delayed works?
- Does it exit without waiting for timers?

It sounds like the refcount approach is the last solution, where destroy_workqueue() just exits without waiting for timers, but then keeping the workqueue alive until the timers elapse.

Quoted 3 lines

The main concern I can see is that this means that delayed work can run after destroy_workqueue() is called. That may be a problem if destroy_workqueue() is used to guard module unload (or device unbind).

delayed_work is just pointing to the wq pointer. On destroy_workqueue(), we can shut it down and free all the supporting stuff while leaving zombie wq struct which noops execution and let the whole thing go away when refs reach zero?

Quoted 4 lines

delayed_work is just pointing to the wq pointer. On destroy_workqueue(), we can shut it down and free all the supporting stuff while leaving zombie wq struct which noops execution and let the whole thing go away when refs reach zero?

But isn't that a problem for e.g. self-freeing work? If we don't run the work, then its memory is just leaked.

Quoted 5 lines · Alice Ryhl

Quoted 4 lines

delayed_work is just pointing to the wq pointer. On destroy_workqueue(), we can shut it down and free all the supporting stuff while leaving zombie wq struct which noops execution and let the whole thing go away when refs reach zero?

Quoted 2 lines

But isn't that a problem for e.g. self-freeing work? If we don't run the work, then its memory is just leaked.

Yeah, good point. Maybe we should just keep the whole thing up while removing it from sysfs. Would that work?

Quoted 7 lines · Tejun Heo

Quoted 5 lines · Alice Ryhl

Quoted 4 lines

delayed_work is just pointing to the wq pointer. On destroy_workqueue(), we can shut it down and free all the supporting stuff while leaving zombie wq struct which noops execution and let the whole thing go away when refs reach zero?

Quoted 5 lines

Quoted 2 lines

But isn't that a problem for e.g. self-freeing work? If we don't run the work, then its memory is just leaked.

Yeah, good point. Maybe we should just keep the whole thing up while removing it from sysfs. Would that work?

We can but there are two variants of that:

If destroy_workqueue() waits for delayed work, then it may take a long time.

If destroy_workqueue() does not wait for delayed work, then I'm worried about bugs resulting from module unload and similar.

(quoted diff) +2 -0

+        self
+    }

rust/kernel/workqueue.rs pub fn try_spawn<T: 'static + Send + FnOnce()>( · line 396

Quoted 3 lines · Alice Ryhl

rust/kernel/workqueue.rs +3 -0

+    /// Set the maximum number of active cpus.
+    ///
+    /// If not set, a reasonable default value is used. The maximum value is `WQ_MAX_ACTIVE`.

Should we just mention the default value?

rust/kernel/workqueue.rs pub fn try_spawn<T: 'static + Send + FnOnce()>( · line 399

Quoted 3 lines

rust/kernel/workqueue.rs +3 -0

+    #[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()?

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()?

Quoted 2 lines

(quoted diff) +2 -0

+        self
+    }

Quoted 11 lines · Danilo Krummrich

rust/kernel/workqueue.rs pub fn try_spawn<T: 'static + Send + FnOnce()>( · line 396

Quoted 3 lines · Alice Ryhl

rust/kernel/workqueue.rs +3 -0

+    /// Set the maximum number of active cpus.
+    ///
+    /// If not set, a reasonable default value is used. The maximum value is `WQ_MAX_ACTIVE`.

Should we just mention the default value?

rust/kernel/workqueue.rs pub fn try_spawn<T: 'static + Send + FnOnce()>( · line 399

Quoted 3 lines

rust/kernel/workqueue.rs +3 -0

+    #[inline]
+    pub fn max_active(mut self, max_active: u32) -> Builder {
+        self.max_active = i32::try_from(max_active).unwrap_or(i32::MAX);

Quoted 5 lines

The workqueue code prints a warning for max_active > WQ_MAX_ACTIVE. Maybe use debug_assert()?

It's also a bit unfortunate that alloc_ordered_workqueue() becomes .max_active(1).

I think ordered workqueue still to have __WQ_ORDERED flag, too?

Quoted 1 line

Quoted 1 line

At the same time having a separate ordered() method competes with max_active().

I don't think its too bad to have both, and before constructing checks that it's used correctly:

    warn_on!(self.flags & __WQ_ORDERED != 0 && self.max_active != 1);

but type state works, too.

rust/kernel/workqueue.rs pub fn try_spawn<T: 'static + Send + FnOnce()>( · line 396

Quoted 3 lines · Alice Ryhl

rust/kernel/workqueue.rs +3 -0

+    /// Set the maximum number of active cpus.
+    ///
+    /// If not set, a reasonable default value is used. The maximum value is `WQ_MAX_ACTIVE`.

Should we just mention the default value?

rust/kernel/workqueue.rs pub fn try_spawn<T: 'static + Send + FnOnce()>( · line 399

Quoted 3 lines

rust/kernel/workqueue.rs +3 -0

+    #[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()?

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()?

Quoted 6 lines · Danilo Krummrich

rust/kernel/workqueue.rs pub fn try_spawn<T: 'static + Send + FnOnce()>( · line 396

Quoted 3 lines · Alice Ryhl

rust/kernel/workqueue.rs +3 -0

+    /// Set the maximum number of active cpus.
+    ///
+    /// If not set, a reasonable default value is used. The maximum value is `WQ_MAX_ACTIVE`.

Should we just mention the default value?

I can mention the constant name, but I'd like to avoid mentioning a value that might change.

Quoted 6 lines

rust/kernel/workqueue.rs pub fn try_spawn<T: 'static + Send + FnOnce()>( · line 399

Quoted 3 lines

rust/kernel/workqueue.rs +3 -0

+    #[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?

Quoted 6 lines

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()?

Quoted 6 lines · Alice Ryhl

Quoted 5 lines · Danilo Krummrich

Quoted 3 lines · Alice Ryhl

rust/kernel/workqueue.rs +3 -0

+    #[inline]
+    pub fn max_active(mut self, max_active: u32) -> Builder {
+        self.max_active = i32::try_from(max_active).unwrap_or(i32::MAX);

Quoted 4 lines

Quoted 2 lines

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().

Quoted 9 lines

Quoted 6 lines

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

Quoted 12 lines · Danilo Krummrich

Quoted 10 lines · Alice Ryhl

Quoted 7 lines · Danilo Krummrich

rust/kernel/workqueue.rs pub fn try_spawn<T: 'static + Send + FnOnce()>( · line 399

Quoted 3 lines · Alice Ryhl

rust/kernel/workqueue.rs +3 -0

+    #[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?

Quoted 2 lines

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.

Quoted 13 lines

Quoted 9 lines

Quoted 6 lines

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.

Quoted 3 lines

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?

Quoted 19 lines · Alice Ryhl

Quoted 7 lines · Danilo Krummrich

Quoted 6 lines · Alice Ryhl

Quoted 5 lines · Danilo Krummrich

Quoted 3 lines · Alice Ryhl

rust/kernel/workqueue.rs +3 -0

+    #[inline]
+    pub fn max_active(mut self, max_active: u32) -> Builder {
+        self.max_active = i32::try_from(max_active).unwrap_or(i32::MAX);

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 2 lines

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?

Quoted 13 lines

Quoted 13 lines

Quoted 9 lines

Quoted 6 lines

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.

Quoted 6 lines

Quoted 3 lines

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).

Quoted 27 lines · Danilo Krummrich

Quoted 13 lines · Alice Ryhl

Quoted 12 lines · Danilo Krummrich

Quoted 10 lines · Alice Ryhl

Quoted 7 lines · Danilo Krummrich

Quoted 3 lines · Alice Ryhl

+    #[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?

Quoted 8 lines

Quoted 2 lines

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.

Quoted 1 line

Maybe we can use a bounded integer?

Bounded integers allow zero, which is also illegal.

I think it's a bit much honestly.

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 <[email protected]>

---
 rust/helpers/workqueue.c |   7 ++
 rust/kernel/workqueue.rs | 190 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 194 insertions(+), 3 deletions(-)

rust/helpers/workqueue.c +7 -0

index ce1c3a5b2150..e4b9d1b3d6bf 100644
@@ -14,3 +14,10 @@ __rust_helper void rust_helper_init_work_with_key(struct work_struct *work,
 	INIT_LIST_HEAD(&work->entry);
 	work->func = func;
 }
+
+__rust_helper
+struct workqueue_struct *rust_helper_alloc_workqueue(const char *fmt, unsigned int flags,
+						     int max_active, const void *data)
+{
+	return alloc_workqueue(fmt, flags, max_active, data);
+}

rust/kernel/workqueue.rs +16 -3

index 1acd113c04ee..4ef02a537cd9 100644
@@ -186,7 +186,10 @@
 //! C header: [`include/linux/workqueue.h`](srctree/include/linux/workqueue.h)
 
 use crate::{
-    alloc::{AllocError, Flags},
+    alloc::{
+        self,
+        AllocError, //
+    },
     container_of,
     prelude::*,
     sync::Arc,

@@ -194,7 +197,11 @@
     time::Jiffies,
     types::Opaque,
 };
-use core::marker::PhantomData;
+use core::{
+    marker::PhantomData,
+    ops::Deref,
+    ptr::NonNull, //
+};
 
 /// Creates a [`Work`] initialiser with the given name and a newly-created lock class.
 #[macro_export]

@@ -340,7 +347,7 @@ pub fn enqueue_delayed<W, const ID: u64>(
     /// This method can fail because it allocates memory to store the work item.
     pub fn try_spawn<T: 'static + Send + FnOnce()>(
         &self,
-        flags: Flags,
+        flags: alloc::Flags,
         func: T,
     ) -> Result<(), AllocError> {
         let init = pin_init!(ClosureWork {

@@ -353,6 +360,183 @@ pub fn try_spawn<T: 'static + Send + FnOnce()>(
     }
 }
 
+/// 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.
+///

rust/kernel/workqueue.rs +81 -0

+        Builder {
+            flags: bindings::wq_flags_WQ_UNBOUND,
+            max_active: 0,
+        }
+    }
+
+    /// Bound to a specific cpu.
+    #[inline]
+    pub fn percpu() -> Builder {
+        Builder {
+            flags: bindings::wq_flags_WQ_PERCPU,
+            max_active: 0,
+        }
+    }
+
+    /// Set the maximum number of active cpus.
+    ///
+    /// If not set, a reasonable default value is used. The maximum value is `WQ_MAX_ACTIVE`.
+    #[inline]
+    pub fn max_active(mut self, max_active: u32) -> Builder {
+        self.max_active = i32::try_from(max_active).unwrap_or(i32::MAX);
+        self
+    }
+
+    /// Allow this workqueue to be frozen during suspend.
+    #[inline]
+    pub fn freezable(mut self) -> Self {
+        self.flags |= bindings::wq_flags_WQ_FREEZABLE;
+        self
+    }
+
+    /// This workqueue may be used during memory reclaim.
+    #[inline]
+    pub fn mem_reclaim(mut self) -> Self {
+        self.flags |= bindings::wq_flags_WQ_MEM_RECLAIM;
+        self
+    }
+
+    /// Mark this workqueue as cpu intensive.
+    #[inline]
+    pub fn cpu_intensive(mut self) -> Self {
+        self.flags |= bindings::wq_flags_WQ_CPU_INTENSIVE;
+        self
+    }
+
+    /// Make this workqueue visible in sysfs.
+    #[inline]
+    pub fn sysfs(mut self) -> Self {
+        self.flags |= bindings::wq_flags_WQ_SYSFS;
+        self
+    }
+
+    /// Mark this workqueue high priority.
+    #[inline]
+    pub fn highpri(mut self) -> Self {
+        self.flags |= bindings::wq_flags_WQ_HIGHPRI;
+        self
+    }
+
+    /// Allocates a new workqueue.
+    ///
+    /// The provided name is used verbatim as the workqueue name.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::workqueue;
+    ///
+    /// // create an unbound workqueue registered with sysfs
+    /// let wq = workqueue::Builder::unbound().sysfs().build(c"my-wq")?;
+    ///
+    /// // spawn a work item on it
+    /// wq.try_spawn(
+    ///     GFP_KERNEL,
+    ///     || pr_warn!("Printing from my-wq"),
+    /// )?;
+    /// # Ok::<(), Error>(())
+    /// ```
+    #[inline]
+    pub fn build(self, name: &CStr) -> Result<OwnedQueue, AllocError> {
+        // SAFETY:

rust/kernel/workqueue.rs +56 -0

+
+        Ok(OwnedQueue {
+            queue: NonNull::new(ptr).ok_or(AllocError)?.cast(),
+        })
+    }
+
+    /// Allocates a new workqueue.
+    ///
+    /// # Examples
+    ///
+    /// This example shows how to pass a Rust string formatter to the workqueue name, creating
+    /// workqueues with names such as `my-wq-1` and `my-wq-2`.
+    ///
+    /// ```
+    /// use kernel::{
+    ///     alloc::AllocError,
+    ///     workqueue::{self, OwnedQueue},
+    /// };
+    ///
+    /// fn my_wq(num: u32) -> Result<OwnedQueue, AllocError> {
+    ///     // create a percpu workqueue called my-wq-{num}
+    ///     workqueue::Builder::percpu().build_fmt(fmt!("my-wq-{num}"))
+    /// }
+    /// ```
+    #[inline]
+    pub fn build_fmt(self, name: kernel::fmt::Arguments<'_>) -> Result<OwnedQueue, AllocError> {
+        // SAFETY:
+        // * c"%pA" is compatible with passing an `Arguments` pointer.
+        // * the builder only permits valid flag combinations
+        let ptr = unsafe {
+            bindings::alloc_workqueue(
+                c"%pA".as_char_ptr(),
+                self.flags,
+                self.max_active,
+                core::ptr::from_ref(&name).cast::<c_void>(),
+            )
+        };
+
+        Ok(OwnedQueue {
+            queue: NonNull::new(ptr).ok_or(AllocError)?.cast(),
+        })
+    }
+}
+
+/// 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>,
+}
+
+impl Deref for OwnedQueue {

rust/kernel/workqueue.rs +4 -0

+        unsafe { bindings::destroy_workqueue(self.queue.as_ptr().cast()) }
+    }
+}
+
 /// A helper type used in [`try_spawn`].
 ///
 /// [`try_spawn`]: Queue::try_spawn

Quoted 74 lines · Alice Ryhl

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 <[email protected]>

---
 rust/helpers/workqueue.c |   7 ++
 rust/kernel/workqueue.rs | 190 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 194 insertions(+), 3 deletions(-)

rust/helpers/workqueue.c +7 -0

index ce1c3a5b2150..e4b9d1b3d6bf 100644
@@ -14,3 +14,10 @@ __rust_helper void rust_helper_init_work_with_key(struct work_struct *work,
 	INIT_LIST_HEAD(&work->entry);
 	work->func = func;
 }
+
+__rust_helper
+struct workqueue_struct *rust_helper_alloc_workqueue(const char *fmt, unsigned int flags,
+						     int max_active, const void *data)
+{
+	return alloc_workqueue(fmt, flags, max_active, data);
+}

rust/kernel/workqueue.rs +16 -3

index 1acd113c04ee..4ef02a537cd9 100644
@@ -186,7 +186,10 @@
 //! C header: [`include/linux/workqueue.h`](srctree/include/linux/workqueue.h)
 
 use crate::{
-    alloc::{AllocError, Flags},
+    alloc::{
+        self,
+        AllocError, //
+    },
     container_of,
     prelude::*,
     sync::Arc,

@@ -194,7 +197,11 @@
     time::Jiffies,
     types::Opaque,
 };
-use core::marker::PhantomData;
+use core::{
+    marker::PhantomData,
+    ops::Deref,
+    ptr::NonNull, //
+};
 
 /// Creates a [`Work`] initialiser with the given name and a newly-created lock class.
 #[macro_export]

@@ -340,7 +347,7 @@ pub fn enqueue_delayed<W, const ID: u64>(
     /// This method can fail because it allocates memory to store the work item.
     pub fn try_spawn<T: 'static + Send + FnOnce()>(
         &self,
-        flags: Flags,
+        flags: alloc::Flags,
         func: T,
     ) -> Result<(), AllocError> {
         let init = pin_init!(ClosureWork {

@@ -353,6 +360,183 @@ pub fn try_spawn<T: 'static + Send + FnOnce()>(
     }
 }
 
+/// 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.
+///

rust/kernel/workqueue.rs pub fn try_spawn<T: 'static + Send + FnOnce()>( · line 378

Quoted 10 lines

rust/kernel/workqueue.rs +10 -0

+/// For details, please refer to `Documentation/core-api/workqueue.rst`.
+pub struct Builder {
+    flags: bindings::wq_flags,
+    max_active: i32,
+}
+
+impl Builder {
+    /// Not bound to any cpu.
+    #[inline]
+    pub fn unbound() -> Builder {

These can all be "-> Self".

Quoted 81 lines

rust/kernel/workqueue.rs +81 -0

+        Builder {
+            flags: bindings::wq_flags_WQ_UNBOUND,
+            max_active: 0,
+        }
+    }
+
+    /// Bound to a specific cpu.
+    #[inline]
+    pub fn percpu() -> Builder {
+        Builder {
+            flags: bindings::wq_flags_WQ_PERCPU,
+            max_active: 0,
+        }
+    }
+
+    /// Set the maximum number of active cpus.
+    ///
+    /// If not set, a reasonable default value is used. The maximum value is `WQ_MAX_ACTIVE`.
+    #[inline]
+    pub fn max_active(mut self, max_active: u32) -> Builder {
+        self.max_active = i32::try_from(max_active).unwrap_or(i32::MAX);
+        self
+    }
+
+    /// Allow this workqueue to be frozen during suspend.
+    #[inline]
+    pub fn freezable(mut self) -> Self {
+        self.flags |= bindings::wq_flags_WQ_FREEZABLE;
+        self
+    }
+
+    /// This workqueue may be used during memory reclaim.
+    #[inline]
+    pub fn mem_reclaim(mut self) -> Self {
+        self.flags |= bindings::wq_flags_WQ_MEM_RECLAIM;
+        self
+    }
+
+    /// Mark this workqueue as cpu intensive.
+    #[inline]
+    pub fn cpu_intensive(mut self) -> Self {
+        self.flags |= bindings::wq_flags_WQ_CPU_INTENSIVE;
+        self
+    }
+
+    /// Make this workqueue visible in sysfs.
+    #[inline]
+    pub fn sysfs(mut self) -> Self {
+        self.flags |= bindings::wq_flags_WQ_SYSFS;
+        self
+    }
+
+    /// Mark this workqueue high priority.
+    #[inline]
+    pub fn highpri(mut self) -> Self {
+        self.flags |= bindings::wq_flags_WQ_HIGHPRI;
+        self
+    }
+
+    /// Allocates a new workqueue.
+    ///
+    /// The provided name is used verbatim as the workqueue name.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::workqueue;
+    ///
+    /// // create an unbound workqueue registered with sysfs
+    /// let wq = workqueue::Builder::unbound().sysfs().build(c"my-wq")?;
+    ///
+    /// // spawn a work item on it
+    /// wq.try_spawn(
+    ///     GFP_KERNEL,
+    ///     || pr_warn!("Printing from my-wq"),
+    /// )?;
+    /// # Ok::<(), Error>(())
+    /// ```
+    #[inline]
+    pub fn build(self, name: &CStr) -> Result<OwnedQueue, AllocError> {
+        // SAFETY:

rust/kernel/workqueue.rs pub fn try_spawn<T: 'static + Send + FnOnce()>( · line 469

Quoted 10 lines

rust/kernel/workqueue.rs +10 -0

+        // * c"%s" is compatible with passing the name as a c-string.
+        // * the builder only permits valid flag combinations
+        let ptr = unsafe {
+            bindings::alloc_workqueue(
+                c"%s".as_char_ptr(),
+                self.flags,
+                self.max_active,
+                name.as_char_ptr().cast::<c_void>(),
+            )
+        };

INVARIANT comments?

Quoted 56 lines

rust/kernel/workqueue.rs +56 -0

+
+        Ok(OwnedQueue {
+            queue: NonNull::new(ptr).ok_or(AllocError)?.cast(),
+        })
+    }
+
+    /// Allocates a new workqueue.
+    ///
+    /// # Examples
+    ///
+    /// This example shows how to pass a Rust string formatter to the workqueue name, creating
+    /// workqueues with names such as `my-wq-1` and `my-wq-2`.
+    ///
+    /// ```
+    /// use kernel::{
+    ///     alloc::AllocError,
+    ///     workqueue::{self, OwnedQueue},
+    /// };
+    ///
+    /// fn my_wq(num: u32) -> Result<OwnedQueue, AllocError> {
+    ///     // create a percpu workqueue called my-wq-{num}
+    ///     workqueue::Builder::percpu().build_fmt(fmt!("my-wq-{num}"))
+    /// }
+    /// ```
+    #[inline]
+    pub fn build_fmt(self, name: kernel::fmt::Arguments<'_>) -> Result<OwnedQueue, AllocError> {
+        // SAFETY:
+        // * c"%pA" is compatible with passing an `Arguments` pointer.
+        // * the builder only permits valid flag combinations
+        let ptr = unsafe {
+            bindings::alloc_workqueue(
+                c"%pA".as_char_ptr(),
+                self.flags,
+                self.max_active,
+                core::ptr::from_ref(&name).cast::<c_void>(),
+            )
+        };
+
+        Ok(OwnedQueue {
+            queue: NonNull::new(ptr).ok_or(AllocError)?.cast(),
+        })
+    }
+}
+
+/// 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>,
+}
+
+impl Deref for OwnedQueue {

rust/kernel/workqueue.rs pub fn try_spawn<T: 'static + Send + FnOnce()>( · line 535

Quoted 10 lines

rust/kernel/workqueue.rs +10 -0

+    type Target = Queue;
+    fn deref(&self) -> &Queue {
+        // SAFETY: By the type invariants, this pointer references a valid queue.
+        unsafe { &*self.queue.as_ptr() }
+    }
+}
+
+impl Drop for OwnedQueue {
+    fn drop(&mut self) {
+        // SAFETY: The `OwnedQueue` is being destroyed, so we can destroy the workqueue it owns.

Hmm, is this correct? This should say *why* the call is safe, not what it is doing.

I think this should mention: (1) the pointer is valid and (2) no delayed work is being scheduled on this queue.