Re: [PATCH v2 1/1] rust: add Work::disable_sync

["Onur Özkan" <work@onurozkan.dev>]

On Tue, 05 May 2026 08:47:45 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> On Tue, May 05, 2026 at 09:07:19AM +0300, Onur Özkan wrote:
> > On Mon, 04 May 2026 07:54:54 +0000
> > Alice Ryhl <aliceryhl@google.com> wrote:
> > 
> > > On Fri, May 01, 2026 at 10:11:22PM +0300, Onur Özkan wrote:
> > > > Adds Work::disable_sync() as a safe wrapper for disable_work_sync().
> > > > 
> > > > Drivers can use this during teardown to stop new queueing and wait for
> > > > queued or running work to finish before dropping related resources.
> > > > 
> > > > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> > > > ---
> > > >  rust/kernel/workqueue.rs | 121 ++++++++++++++++++++++++++-------------
> > > >  1 file changed, 81 insertions(+), 40 deletions(-)
> > > > 
> > > > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> > > > index 7e253b6f299c..d0f9b4ba7f27 100644
> > > > --- a/rust/kernel/workqueue.rs
> > > > +++ b/rust/kernel/workqueue.rs
> > > > @@ -267,7 +267,7 @@ pub unsafe fn from_raw<'a>(ptr: *const bindings::workqueue_struct) -> &'a Queue
> > > >  
> > > >      /// Enqueues a work item.
> > > >      ///
> > > > -    /// This may fail if the work item is already enqueued in a workqueue.
> > > > +    /// This may fail if the work item is already enqueued in a workqueue or disabled.
> > > >      ///
> > > >      /// The work item will be submitted using `WORK_CPU_UNBOUND`.
> > > >      pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput
> > > 
> > > Can you elaborate on the case where disable leads to failure here? Can
> > > you not enqueue a work item again after disabling it? Is there a doc
> > > test illustrating this case that I can run for myself to see the
> > > behavior in action?
> > 
> > As we discussed on yesterday's call, I looked into cancel_work_sync and
> > it seems we can make this work in the tyr reset implementation. We already
> > store an atomic reset state, it can be used to prevent future enqueues in
> > reset scheduling.
> > 
> > I will send a patch for the cancel_sync function soon.
> 
> I did hear from Boris that Panthor actually does make use of the disable
> feature. 

I don't have any idea what Boris said, perhaps he should write it here as well.
Maybe Boris said something that isn't covered on the tyr reset yet in my series,
I don't know.

What I do know is that I can achieve essentially the same behavior using
cancel_work_sync instead of disable_work_sync on tyr. Like i said there's an
atomic reset state we can use to prevent future work from being enqueued. The
only real difference is that supporting disable_work_sync in the Rust workqueue
would introduce more complexity than supporting cancel_work_sync. There is also
the corresponding enable part we have to support if we want to go with
disable_work_sync path.

	Onur

> 
> What I would probably suggest here is to choose whether you can disable
> a work item based on the pointer type. If using a pointer type where
> enqueue is *already* fallible, we might as well also support disabling
> it.
> 
> pub trait SupportsDisable<const ID: u64>: WorkItemPointer { }
> 
> unsafe impl<T, const ID: u64> SupportsDisable<ID> for ARef<T>
> where
>     Self: WorkItemPointer<ID>,
>     T: AlwaysRefCounted,
> {
> }
> 
> So you can implement the trait as above for ARef and Arc, but not for
> Box.
> 
> Then, when you add the method for actually performing a disable, you add
> a requirement that the pointer type implements this trait:
> 
> /// Disables this work item and waits for queued/running executions to finish.
> ///
> /// # Note
> ///
> /// Should be called from a sleepable context if the work was last queued on a non-BH
> /// workqueue.
> #[inline]
> pub fn disable_sync(&self)
> where
>     T: WorkItem<ID>,
>     <T as WorkItem<ID>>::Pointer: SupportsDisable<ID>,
> { ... }
> 
> This way the pointer type in use determines whether to opt-in to
> exposing the disable bit.
> 
> I do not think there is any harm to not exposing `disable_sync()` for
> box, because you can't even call it on an enqueued work item to begin
> with. If it's enqueued, the workqueue has ownership over the box, so you
> have no way of obtaining a reference to it, which you would need to call
> disable_sync(). So we would only be taking away the ability to call that
> method in cases where you *know* the work item is not enqueued anyway.
> 
> Alice