[PATCH] workqueue: rust: add creation of workqueues

[PATCH] workqueue: rust: add creation of workqueues

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

This patch is being developed in parallel with the new Owned type [1].
The OwnedQueue struct becomes redundant once [1] lands; at that point it
can be replaced with Owned<Queue>, and constructors can be moved to the
Queue type.

A wrapper type WqFlags is provided for workqueue flags. Since we only
provide the | operator for this wrapper type, this makes it impossible
to pass internal workqueue flags to the workqueue constructor. It has
the consequence that we need a separate constant for the no-flags case,
as the constructor does not accept a literal 0. I named this constant
"BOUND" to signify the opposite of UNBOUND.

Link: https://lore.kernel.org/rust-for-linux/20250325-unique-ref-v9-0-e91618c1de26@pm.me/ [1]
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
This patch is based on top of:
https://lore.kernel.org/all/20250411-workqueue-delay-v1-1-26b9427b1054@google.com/
---
 rust/kernel/workqueue.rs | 141 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 140 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index c30fe0185e7a6a89943a5ba9f5b36a5bca3edb85..eaee42e289c4d00c447727c42e9048298560122a 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -194,7 +194,7 @@
     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]
@@ -346,6 +346,145 @@ pub fn try_spawn<T: 'static + Send + FnOnce()>(
     }
 }
 
+/// Workqueue flags.
+///
+/// For details, please refer to `Documentation/core-api/workqueue.rst`.
+///
+/// # Invariants
+///
+/// Must contain a valid combination of workqueue flags that may be used with `alloc_workqueue`.
+#[repr(transparent)]
+#[derive(Copy, Clone)]
+pub struct WqFlags(bindings::wq_flags);
+
+impl WqFlags {
+    /// Bound to a cpu.
+    pub const BOUND: WqFlags = WqFlags(0);
+    /// Execute in bottom half (softirq) context.
+    pub const BH: WqFlags = WqFlags(bindings::wq_flags_WQ_BH);
+    /// Not bound to a cpu.
+    pub const UNBOUND: WqFlags = WqFlags(bindings::wq_flags_WQ_UNBOUND);
+    /// Freeze during suspend.
+    pub const FREEZABLE: WqFlags = WqFlags(bindings::wq_flags_WQ_FREEZABLE);
+    /// May be used for memory reclaim.
+    pub const MEM_RECLAIM: WqFlags = WqFlags(bindings::wq_flags_WQ_MEM_RECLAIM);
+    /// High priority.
+    pub const HIGHPRI: WqFlags = WqFlags(bindings::wq_flags_WQ_HIGHPRI);
+    /// Cpu intensive workqueue.
+    pub const CPU_INTENSIVE: WqFlags = WqFlags(bindings::wq_flags_WQ_CPU_INTENSIVE);
+    /// Visible in sysfs.
+    pub const SYSFS: WqFlags = WqFlags(bindings::wq_flags_WQ_SYSFS);
+    /// Power-efficient workqueue.
+    pub const POWER_EFFICIENT: WqFlags = WqFlags(bindings::wq_flags_WQ_POWER_EFFICIENT);
+}
+
+impl core::ops::BitOr for WqFlags {
+    type Output = WqFlags;
+    fn bitor(self, rhs: WqFlags) -> WqFlags {
+        WqFlags(self.0 | rhs.0)
+    }
+}
+
+/// An owned kernel work queue.
+///
+/// # Invariants
+///
+/// `queue` points at a valid workqueue that is owned by this `OwnedQueue`.
+pub struct OwnedQueue {
+    queue: NonNull<Queue>,
+}
+
+impl OwnedQueue {
+    /// Allocates a new workqueue.
+    ///
+    /// The provided name is used verbatim as the workqueue name.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::c_str;
+    /// use kernel::workqueue::{OwnedQueue, WqFlags};
+    ///
+    /// let wq = OwnedQueue::new(c_str!("my-wq"), WqFlags::UNBOUND, 0)?;
+    /// wq.try_spawn(
+    ///     GFP_KERNEL,
+    ///     || pr_warn!("Printing from my-wq"),
+    /// )?;
+    /// # Ok::<(), Error>(())
+    /// ```
+    #[inline]
+    pub fn new(name: &CStr, flags: WqFlags, max_active: usize) -> Result<OwnedQueue, AllocError> {
+        // SAFETY:
+        // * "%s\0" is compatible with passing the name as a c-string.
+        // * the flags argument does not include internal flags.
+        let ptr = unsafe {
+            bindings::alloc_workqueue(
+                b"%s\0".as_ptr(),
+                flags.0,
+                i32::try_from(max_active).unwrap_or(i32::MAX),
+                name.as_char_ptr(),
+            )
+        };
+
+        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;
+    /// use kernel::workqueue::{OwnedQueue, WqFlags};
+    ///
+    /// fn my_wq(num: u32) -> Result<OwnedQueue, AllocError> {
+    ///     OwnedQueue::new_fmt(format_args!("my-wq-{num}"), WqFlags::UNBOUND, 0)
+    /// }
+    /// ```
+    #[inline]
+    pub fn new_fmt(
+        name: core::fmt::Arguments<'_>,
+        flags: WqFlags,
+        max_active: usize,
+    ) -> Result<OwnedQueue, AllocError> {
+        // SAFETY:
+        // * "%pA\0" is compatible with passing an `Arguments` pointer.
+        // * the flags argument does not include internal flags.
+        let ptr = unsafe {
+            bindings::alloc_workqueue(
+                b"%pA\0".as_ptr(),
+                flags.0,
+                i32::try_from(max_active).unwrap_or(i32::MAX),
+                (&name) as *const _ as *const crate::ffi::c_void,
+            )
+        };
+
+        Ok(OwnedQueue {
+            queue: NonNull::new(ptr).ok_or(AllocError)?.cast(),
+        })
+    }
+}
+
+impl Deref for OwnedQueue {
+    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.
+        unsafe { bindings::destroy_workqueue(self.queue.as_ptr().cast()) }
+    }
+}
+
 /// A helper type used in [`try_spawn`].
 ///
 /// [`try_spawn`]: Queue::try_spawn

---
base-commit: d11cfa069ac43b6e38f2f603a18e742ef048122a
change-id: 20250411-create-workqueue-d053158c7a4b

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

Re: [PATCH] workqueue: rust: add creation of workqueues

[Tejun Heo]

Hello,

On Fri, Apr 11, 2025 at 03:34:24PM +0000, 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.
> 
> This patch is being developed in parallel with the new Owned type [1].
> The OwnedQueue struct becomes redundant once [1] lands; at that point it
> can be replaced with Owned<Queue>, and constructors can be moved to the
> Queue type.
> 
> A wrapper type WqFlags is provided for workqueue flags. Since we only
> provide the | operator for this wrapper type, this makes it impossible
> to pass internal workqueue flags to the workqueue constructor. It has
> the consequence that we need a separate constant for the no-flags case,
> as the constructor does not accept a literal 0. I named this constant
> "BOUND" to signify the opposite of UNBOUND.

Maybe name it NONE or DUMMY? Doesn't affect this patch but [UN]BOUND are a
bit confusing and as a part of the effort to reduce unnecessary usage of
cpu-bound workqueues, there's a plan to flip the default and use PERCPU for
the cpu-bound workqueues.

Thanks.

-- 
tejun

Re: [PATCH] workqueue: rust: add creation of workqueues

[Danilo Krummrich]

On Fri, Apr 11, 2025 at 03:34:24PM +0000, Alice Ryhl wrote:
> 
> +/// An owned kernel work queue.

I'd suggest to document that dropping an OwnedQueue will wait for pending work.

Additionally, given that you're about to implement delayed work as well, we
should also mention that destroy_workqueue() currently does not cover waiting
for delayed work *before* it is scheduled and hence may cause WARN() splats or
even UAF bugs.

> +///
> +/// # Invariants
> +///
> +/// `queue` points at a valid workqueue that is owned by this `OwnedQueue`.
> +pub struct OwnedQueue {
> +    queue: NonNull<Queue>,
> +}

Re: [PATCH] workqueue: rust: add creation of workqueues

[Alice Ryhl]

On Mon, Apr 14, 2025 at 08:15:41PM +0200, Danilo Krummrich wrote:
> On Fri, Apr 11, 2025 at 03:34:24PM +0000, Alice Ryhl wrote:
> > 
> > +/// An owned kernel work queue.
> 
> I'd suggest to document that dropping an OwnedQueue will wait for pending work.
> 
> Additionally, given that you're about to implement delayed work as well, we
> should also mention that destroy_workqueue() currently does not cover waiting
> for delayed work *before* it is scheduled and hence may cause WARN() splats or
> even UAF bugs.

Ah, that's a problem :(

Can we make destroy_workqueue() wait for delayed items too? And/or have
a variant of it that does so? I'm not sure what is best to do here...

Alice

Re: [PATCH] workqueue: rust: add creation of workqueues

[Alice Ryhl]

On Mon, Apr 14, 2025 at 07:23:42AM -1000, Tejun Heo wrote:
> Hello,
> 
> On Fri, Apr 11, 2025 at 03:34:24PM +0000, 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.
> > 
> > This patch is being developed in parallel with the new Owned type [1].
> > The OwnedQueue struct becomes redundant once [1] lands; at that point it
> > can be replaced with Owned<Queue>, and constructors can be moved to the
> > Queue type.
> > 
> > A wrapper type WqFlags is provided for workqueue flags. Since we only
> > provide the | operator for this wrapper type, this makes it impossible
> > to pass internal workqueue flags to the workqueue constructor. It has
> > the consequence that we need a separate constant for the no-flags case,
> > as the constructor does not accept a literal 0. I named this constant
> > "BOUND" to signify the opposite of UNBOUND.
> 
> Maybe name it NONE or DUMMY? Doesn't affect this patch but [UN]BOUND are a
> bit confusing and as a part of the effort to reduce unnecessary usage of
> cpu-bound workqueues, there's a plan to flip the default and use PERCPU for
> the cpu-bound workqueues.

Happy with whatever you think is best, but what about naming the
constant PERCPU, then? In fact, by adjusting how I declare the flags in
Rust, it is possible to *force* the user to include exactly one of
PERCPU, UNBOUND, or BH in the flags argument.

Of course, also happy to continue with NONE and adjust it when you flip
the default value.

Alice

Re: [PATCH] workqueue: rust: add creation of workqueues

[Danilo Krummrich]

On Tue, Apr 15, 2025 at 09:01:35AM +0000, Alice Ryhl wrote:
> On Mon, Apr 14, 2025 at 08:15:41PM +0200, Danilo Krummrich wrote:
> > On Fri, Apr 11, 2025 at 03:34:24PM +0000, Alice Ryhl wrote:
> > > 
> > > +/// An owned kernel work queue.
> > 
> > I'd suggest to document that dropping an OwnedQueue will wait for pending work.
> > 
> > Additionally, given that you're about to implement delayed work as well, we
> > should also mention that destroy_workqueue() currently does not cover waiting
> > for delayed work *before* it is scheduled and hence may cause WARN() splats or
> > even UAF bugs.
> 
> Ah, that's a problem :(
> 
> Can we make destroy_workqueue() wait for delayed items too? And/or have
> a variant of it that does so? I'm not sure what is best to do here...

I think the problem is that the workq is not aware of all the timers in flight
and simply queues the work in the timer callback. See also [1].

I'm not sure there's an easy solution to that, without adding extra overhead,
such as keeping a list of timers in flight in the workqueue end. :(

[1] https://elixir.bootlin.com/linux/v6.13.7/source/kernel/workqueue.c#L2489

Re: [PATCH] workqueue: rust: add creation of workqueues

[Tejun Heo]

On Tue, Apr 15, 2025 at 09:05:16AM +0000, Alice Ryhl wrote:
> On Mon, Apr 14, 2025 at 07:23:42AM -1000, Tejun Heo wrote:
> > Hello,
> > 
> > On Fri, Apr 11, 2025 at 03:34:24PM +0000, 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.
> > > 
> > > This patch is being developed in parallel with the new Owned type [1].
> > > The OwnedQueue struct becomes redundant once [1] lands; at that point it
> > > can be replaced with Owned<Queue>, and constructors can be moved to the
> > > Queue type.
> > > 
> > > A wrapper type WqFlags is provided for workqueue flags. Since we only
> > > provide the | operator for this wrapper type, this makes it impossible
> > > to pass internal workqueue flags to the workqueue constructor. It has
> > > the consequence that we need a separate constant for the no-flags case,
> > > as the constructor does not accept a literal 0. I named this constant
> > > "BOUND" to signify the opposite of UNBOUND.
> > 
> > Maybe name it NONE or DUMMY? Doesn't affect this patch but [UN]BOUND are a
> > bit confusing and as a part of the effort to reduce unnecessary usage of
> > cpu-bound workqueues, there's a plan to flip the default and use PERCPU for
> > the cpu-bound workqueues.
> 
> Happy with whatever you think is best, but what about naming the
> constant PERCPU, then? In fact, by adjusting how I declare the flags in
> Rust, it is possible to *force* the user to include exactly one of
> PERCPU, UNBOUND, or BH in the flags argument.

Oh yeah, if you can force a choice among those three, that sounds great.

Thanks.

-- 
tejun

Re: [PATCH] workqueue: rust: add creation of workqueues

[Alice Ryhl]

On Tue, Apr 15, 2025 at 12:48:48PM +0200, Danilo Krummrich wrote:
> On Tue, Apr 15, 2025 at 09:01:35AM +0000, Alice Ryhl wrote:
> > On Mon, Apr 14, 2025 at 08:15:41PM +0200, Danilo Krummrich wrote:
> > > On Fri, Apr 11, 2025 at 03:34:24PM +0000, Alice Ryhl wrote:
> > > > 
> > > > +/// An owned kernel work queue.
> > > 
> > > I'd suggest to document that dropping an OwnedQueue will wait for pending work.
> > > 
> > > Additionally, given that you're about to implement delayed work as well, we
> > > should also mention that destroy_workqueue() currently does not cover waiting
> > > for delayed work *before* it is scheduled and hence may cause WARN() splats or
> > > even UAF bugs.
> > 
> > Ah, that's a problem :(
> > 
> > Can we make destroy_workqueue() wait for delayed items too? And/or have
> > a variant of it that does so? I'm not sure what is best to do here...
> 
> I think the problem is that the workq is not aware of all the timers in flight
> and simply queues the work in the timer callback. See also [1].
> 
> I'm not sure there's an easy solution to that, without adding extra overhead,
> such as keeping a list of timers in flight in the workqueue end. :(
> 
> [1] https://elixir.bootlin.com/linux/v6.13.7/source/kernel/workqueue.c#L2489

It looks like panthor handles this by only having a single delayed work
item on each queue and using cancel_delayed_work_sync before calling
destroy_workqueue.

Tejun, what do you suggest? The goal of the Rust API is to make it
impossible to accidentally trigger a UAF, so we need to design the API
to prevent this mistake.

Alice

Re: [PATCH] workqueue: rust: add creation of workqueues

[Alice Ryhl]

On Wed, Apr 16, 2025 at 2:17 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Tue, Apr 15, 2025 at 12:48:48PM +0200, Danilo Krummrich wrote:
> > On Tue, Apr 15, 2025 at 09:01:35AM +0000, Alice Ryhl wrote:
> > > On Mon, Apr 14, 2025 at 08:15:41PM +0200, Danilo Krummrich wrote:
> > > > On Fri, Apr 11, 2025 at 03:34:24PM +0000, Alice Ryhl wrote:
> > > > >
> > > > > +/// An owned kernel work queue.
> > > >
> > > > I'd suggest to document that dropping an OwnedQueue will wait for pending work.
> > > >
> > > > Additionally, given that you're about to implement delayed work as well, we
> > > > should also mention that destroy_workqueue() currently does not cover waiting
> > > > for delayed work *before* it is scheduled and hence may cause WARN() splats or
> > > > even UAF bugs.
> > >
> > > Ah, that's a problem :(
> > >
> > > Can we make destroy_workqueue() wait for delayed items too? And/or have
> > > a variant of it that does so? I'm not sure what is best to do here...
> >
> > I think the problem is that the workq is not aware of all the timers in flight
> > and simply queues the work in the timer callback. See also [1].
> >
> > I'm not sure there's an easy solution to that, without adding extra overhead,
> > such as keeping a list of timers in flight in the workqueue end. :(
> >
> > [1] https://elixir.bootlin.com/linux/v6.13.7/source/kernel/workqueue.c#L2489
>
> It looks like panthor handles this by only having a single delayed work
> item on each queue and using cancel_delayed_work_sync before calling
> destroy_workqueue.
>
> Tejun, what do you suggest? The goal of the Rust API is to make it
> impossible to accidentally trigger a UAF, so we need to design the API
> to prevent this mistake.

Ok, looks like I'm not the only one with this problem.
https://lore.kernel.org/all/20250404101543.74262-2-phasta@kernel.org/

I think the most natural behavior is that destroy_workqueue() should
also wait for delayed work items, since the workqueue does not know
how to cancel them. Anyone who wants them cancelled can manually
cancel those work items before calling destroy_workqueue(), and that
would be no different from what they have to do today.

I thought about implementation approaches. The first thought that
sprang to mind is add a list of all delayed work items, but now I
think we can do better. We can have an atomic counter tracking the
number of delayed work items, and have destroy_workqueue() do this:

retry:
drain_workqueue(wq);
if (has_delayed_work_items(wq)) {
    wait_for_delayed_to_be_scheduled(wq);
    goto retry;
}

where wait_for_delayed_to_be_scheduled() either waits for the counter
to hit zero, or waits for at least waits for one of them to be
scheduled. For example, maybe wait_for_delayed_to_be_scheduled() could
add a dummy work item *without* waking up the worker threads, and then
wait for that work item to get executed, which would effectively mean
that it sleeps until something else wakes up a worker.

Thoughts?

Alice

Re: [PATCH] workqueue: rust: add creation of workqueues

[Tejun Heo]

Hello,

On Wed, Apr 16, 2025 at 12:17:48PM +0000, Alice Ryhl wrote:
...
> It looks like panthor handles this by only having a single delayed work
> item on each queue and using cancel_delayed_work_sync before calling
> destroy_workqueue.

That sounds a bit too restrictive.

> Tejun, what do you suggest? The goal of the Rust API is to make it
> impossible to accidentally trigger a UAF, so we need to design the API
> to prevent this mistake.

This is a hole in the C API, which isn't great but also not the end of the
world in C. I think it'd be best to solve this from C side rather than
working around it from rust side. e.g.:

- Have per-cpu list_head per workqueue (or system-wide).

- When queueing a delayed_work on timer, add the work item to the cpu's
  per-cpu list. This shouldn't need adding more fields to delayed_work given
  that the work part isn't being used yet. Also need to record the cpu.

- When timer expires, unlink from the per-cpu list and then do the normal
  queueing. This wil happen on the same cpu in most cases.

- Flush the lists from drain/destroy_workqueue(). If using system-wide
  per-cpu lists, we'd have to scan the lists and match the target wq.

This should be pretty cheap and we can probably enable this for everyone,
but if the overhead is noticeable, this can be an optional behavior
depending on a workqueue flag.

Thanks.

-- 
tejun

Re: [PATCH] workqueue: rust: add creation of workqueues

[Tejun Heo]

Hello, Alice.

On Wed, Apr 16, 2025 at 09:41:21PM +0200, Alice Ryhl wrote:
...
> I thought about implementation approaches. The first thought that
> sprang to mind is add a list of all delayed work items, but now I
> think we can do better. We can have an atomic counter tracking the
> number of delayed work items, and have destroy_workqueue() do this:
> 
> retry:
> drain_workqueue(wq);
> if (has_delayed_work_items(wq)) {
>     wait_for_delayed_to_be_scheduled(wq);
>     goto retry;
> }
> 
> where wait_for_delayed_to_be_scheduled() either waits for the counter
> to hit zero, or waits for at least waits for one of them to be
> scheduled. For example, maybe wait_for_delayed_to_be_scheduled() could
> add a dummy work item *without* waking up the worker threads, and then
> wait for that work item to get executed, which would effectively mean
> that it sleeps until something else wakes up a worker.

I suppose that can work too but the delays can be pretty long, so while
correct, I'm not sure it'd be very pleasant to use. If we per-cpu lists, I
don't think the overhead would be all that noticeable, so may as well do
that?

Thanks.

-- 
tejun

Re: [PATCH] workqueue: rust: add creation of workqueues

[Alice Ryhl]

On Wed, Apr 16, 2025 at 9:53 PM Tejun Heo <tj@kernel.org> wrote:
>
> Hello,
>
> On Wed, Apr 16, 2025 at 12:17:48PM +0000, Alice Ryhl wrote:
> ...
> > It looks like panthor handles this by only having a single delayed work
> > item on each queue and using cancel_delayed_work_sync before calling
> > destroy_workqueue.
>
> That sounds a bit too restrictive.
>
> > Tejun, what do you suggest? The goal of the Rust API is to make it
> > impossible to accidentally trigger a UAF, so we need to design the API
> > to prevent this mistake.
>
> This is a hole in the C API, which isn't great but also not the end of the
> world in C. I think it'd be best to solve this from C side rather than
> working around it from rust side. e.g.:

Agreed. I think this fits on the C side.

> - Have per-cpu list_head per workqueue (or system-wide).
>
> - When queueing a delayed_work on timer, add the work item to the cpu's
>   per-cpu list. This shouldn't need adding more fields to delayed_work given
>   that the work part isn't being used yet. Also need to record the cpu.
>
> - When timer expires, unlink from the per-cpu list and then do the normal
>   queueing. This wil happen on the same cpu in most cases.
>
> - Flush the lists from drain/destroy_workqueue(). If using system-wide
>   per-cpu lists, we'd have to scan the lists and match the target wq.
>
> This should be pretty cheap and we can probably enable this for everyone,
> but if the overhead is noticeable, this can be an optional behavior
> depending on a workqueue flag.

My only concern is that we're executing work items *before* the
deadline they specified. There could be work items that assume this
doesn't happen? But maybe it's okay. Otherwise, what you suggest seems
reasonable enough to me.

Alice

Re: [PATCH] workqueue: rust: add creation of workqueues

[Tejun Heo]

On Wed, Apr 16, 2025 at 10:08:35PM +0200, Alice Ryhl wrote:
...
> > This should be pretty cheap and we can probably enable this for everyone,
> > but if the overhead is noticeable, this can be an optional behavior
> > depending on a workqueue flag.
> 
> My only concern is that we're executing work items *before* the
> deadline they specified. There could be work items that assume this
> doesn't happen? But maybe it's okay. Otherwise, what you suggest seems
> reasonable enough to me.

That's already what flush_delayed_work() does, so I don't think it'd be too
surprising. Alternatively, we can go for canceling on draining/destruction
but that'd be more surprising I think. As long as the behavior is documented
clearly, I don't see problems with running and flushing them.

Thanks.

-- 
tejun

Re: [PATCH] workqueue: rust: add creation of workqueues

[Alice Ryhl]

On Wed, Apr 16, 2025 at 10:10 PM Tejun Heo <tj@kernel.org> wrote:
>
> On Wed, Apr 16, 2025 at 10:08:35PM +0200, Alice Ryhl wrote:
> ...
> > > This should be pretty cheap and we can probably enable this for everyone,
> > > but if the overhead is noticeable, this can be an optional behavior
> > > depending on a workqueue flag.
> >
> > My only concern is that we're executing work items *before* the
> > deadline they specified. There could be work items that assume this
> > doesn't happen? But maybe it's okay. Otherwise, what you suggest seems
> > reasonable enough to me.
>
> That's already what flush_delayed_work() does, so I don't think it'd be too
> surprising. Alternatively, we can go for canceling on draining/destruction
> but that'd be more surprising I think. As long as the behavior is documented
> clearly, I don't see problems with running and flushing them.

Sounds good. I'll go ahead and submit a patch for this.

Alice

Re: [PATCH] workqueue: rust: add creation of workqueues

[Tejun Heo]

On Wed, Apr 16, 2025 at 10:10:41AM -1000, Tejun Heo wrote:
> On Wed, Apr 16, 2025 at 10:08:35PM +0200, Alice Ryhl wrote:
> ...
> > > This should be pretty cheap and we can probably enable this for everyone,
> > > but if the overhead is noticeable, this can be an optional behavior
> > > depending on a workqueue flag.
> > 
> > My only concern is that we're executing work items *before* the
> > deadline they specified. There could be work items that assume this
> > doesn't happen? But maybe it's okay. Otherwise, what you suggest seems
> > reasonable enough to me.
> 
> That's already what flush_delayed_work() does, so I don't think it'd be too
> surprising. Alternatively, we can go for canceling on draining/destruction
> but that'd be more surprising I think. As long as the behavior is documented
> clearly, I don't see problems with running and flushing them.

Also, note that self-requeueing work items may still be pending after
draining a workqueue as the draining is best effort. This is considered a
bug in the caller and, we trigger a warn and just skip freeing the
workqueue. This is again not great but may be acceptable for rust too. If
one wants to improve this, now that we have disable_work(), we can probably
trigger warn after X retries and then switch to disabling & flushing
afterwards.

Thanks.

-- 
tejun

Re: [PATCH] workqueue: rust: add creation of workqueues

[Alice Ryhl]

On Wed, Apr 16, 2025 at 10:14 PM Tejun Heo <tj@kernel.org> wrote:
>
> On Wed, Apr 16, 2025 at 10:10:41AM -1000, Tejun Heo wrote:
> > On Wed, Apr 16, 2025 at 10:08:35PM +0200, Alice Ryhl wrote:
> > ...
> > > > This should be pretty cheap and we can probably enable this for everyone,
> > > > but if the overhead is noticeable, this can be an optional behavior
> > > > depending on a workqueue flag.
> > >
> > > My only concern is that we're executing work items *before* the
> > > deadline they specified. There could be work items that assume this
> > > doesn't happen? But maybe it's okay. Otherwise, what you suggest seems
> > > reasonable enough to me.
> >
> > That's already what flush_delayed_work() does, so I don't think it'd be too
> > surprising. Alternatively, we can go for canceling on draining/destruction
> > but that'd be more surprising I think. As long as the behavior is documented
> > clearly, I don't see problems with running and flushing them.
>
> Also, note that self-requeueing work items may still be pending after
> draining a workqueue as the draining is best effort. This is considered a
> bug in the caller and, we trigger a warn and just skip freeing the
> workqueue. This is again not great but may be acceptable for rust too. If
> one wants to improve this, now that we have disable_work(), we can probably
> trigger warn after X retries and then switch to disabling & flushing
> afterwards.

Rust doesn't let you call methods on a struct while its destructor is
running, so I don't think this bug is possible in Rust, since it
involves calling `enqueue` on a `Queue` whose destructor is currently
running.

Alice

Re: [PATCH] workqueue: rust: add creation of workqueues

[Philipp Stanner]

On Wed, 2025-04-16 at 09:57 -1000, Tejun Heo wrote:
> Hello, Alice.
> 
> On Wed, Apr 16, 2025 at 09:41:21PM +0200, Alice Ryhl wrote:
> ...
> > I thought about implementation approaches. The first thought that
> > sprang to mind is add a list of all delayed work items, but now I
> > think we can do better. We can have an atomic counter tracking the
> > number of delayed work items, and have destroy_workqueue() do this:
> > 
> > retry:
> > drain_workqueue(wq);
> > if (has_delayed_work_items(wq)) {
> >     wait_for_delayed_to_be_scheduled(wq);
> >     goto retry;
> > }
> > 
> > where wait_for_delayed_to_be_scheduled() either waits for the
> > counter
> > to hit zero, or waits for at least waits for one of them to be
> > scheduled. For example, maybe wait_for_delayed_to_be_scheduled()
> > could
> > add a dummy work item *without* waking up the worker threads, and
> > then
> > wait for that work item to get executed, which would effectively
> > mean
> > that it sleeps until something else wakes up a worker.
> 
> I suppose that can work too but the delays can be pretty long, so
> while
> correct, I'm not sure it'd be very pleasant to use. If we per-cpu
> lists, I
> don't think the overhead would be all that noticeable, so may as well
> do
> that?

I assume you, ultimately, mean that the list of delayed_work's would be
accessible through workqueue_struct, correct?

And then destroy_workqueue() could loop over all of them with
cancel_delayed_work_sync()?

I think that would be the cleanest possible solution.

P.

> 
> Thanks.
> 

Re: [PATCH] workqueue: rust: add creation of workqueues

[Tejun Heo]

On Thu, Apr 17, 2025 at 09:22:40AM +0200, Philipp Stanner wrote:
> I assume you, ultimately, mean that the list of delayed_work's would be
> accessible through workqueue_struct, correct?
> 
> And then destroy_workqueue() could loop over all of them with
> cancel_delayed_work_sync()?

Yeap, I was thinking flush_delayed_work() but maybe
cancel_delayed_work_sync() is better.

Thanks.

-- 
tejun

Re: [PATCH] workqueue: rust: add creation of workqueues

[Alice Ryhl]

On Thu, Apr 17, 2025 at 9:28 AM Tejun Heo <tj@kernel.org> wrote:
>
> On Thu, Apr 17, 2025 at 09:22:40AM +0200, Philipp Stanner wrote:
> > I assume you, ultimately, mean that the list of delayed_work's would be
> > accessible through workqueue_struct, correct?
> >
> > And then destroy_workqueue() could loop over all of them with
> > cancel_delayed_work_sync()?
>
> Yeap, I was thinking flush_delayed_work() but maybe
> cancel_delayed_work_sync() is better.

But doesn't that have a cleanup problem? If the work item owns an
allocation or a refcount that's cleared by the work item's run
function, then using cancel_delayed_work_sync() will fail to clean
that up. Whereas flush_delayed_work() avoids this problem.

Alice

Re: [PATCH] workqueue: rust: add creation of workqueues

[Danilo Krummrich]

On Thu, Apr 17, 2025 at 10:26:04PM +0200, Alice Ryhl wrote:
> On Thu, Apr 17, 2025 at 9:28 AM Tejun Heo <tj@kernel.org> wrote:
> >
> > On Thu, Apr 17, 2025 at 09:22:40AM +0200, Philipp Stanner wrote:
> > > I assume you, ultimately, mean that the list of delayed_work's would be
> > > accessible through workqueue_struct, correct?
> > >
> > > And then destroy_workqueue() could loop over all of them with
> > > cancel_delayed_work_sync()?
> >
> > Yeap, I was thinking flush_delayed_work() but maybe
> > cancel_delayed_work_sync() is better.
> 
> But doesn't that have a cleanup problem? If the work item owns an
> allocation or a refcount that's cleared by the work item's run
> function, then using cancel_delayed_work_sync() will fail to clean
> that up. Whereas flush_delayed_work() avoids this problem.

I also think it may be a bit unexpected to users if pending "normal" work "will
be done first", but delayed work is canceled instead.

That sounds like a good source for wrong use. :(

Re: [PATCH] workqueue: rust: add creation of workqueues

[Tejun Heo]

Hello,

On Thu, Apr 17, 2025 at 10:26:04PM +0200, Alice Ryhl wrote:
...
> But doesn't that have a cleanup problem? If the work item owns an
> allocation or a refcount that's cleared by the work item's run
> function, then using cancel_delayed_work_sync() will fail to clean
> that up. Whereas flush_delayed_work() avoids this problem.

True, especially for self-freeing work items. flush it is, I suppose.

Thanks.

-- 
tejun