Messages in this thread Patch in this message |  | | From | Onur Özkan <> | | Subject | Re: [PATCH v2 1/1] rust: add Work::disable_sync | | Date | Tue, 5 May 2026 12:16:12 +0300 |
| |
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
|  |