[RFC,1/4] workqueue: support pausing ordered workqueues
Commit Message
From: Johannes Berg <johannes.berg@intel.com>
Add some infrastructure to support pausing ordered
workqueues, so that no work items are executing nor
can execute while the workqueue is paused.
This can be used to simplify locking between work
structs and other processes (e.g. userspace calls)
when the workqueue is paused while other code is
running, where we can then more easily avoid issues
in code paths needing to cancel works.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
include/linux/workqueue.h | 26 ++++++++++++++++++++++++++
kernel/workqueue.c | 27 ++++++++++++++++++++++++++-
2 files changed, 52 insertions(+), 1 deletion(-)
Comments
Hello,
This looks great to me in general.
On Wed, May 10, 2023 at 06:04:25PM +0200, Johannes Berg wrote:
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index b8b541caed48..418d99ff8325 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3863,10 +3863,16 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
> struct workqueue_struct *wq = pwq->wq;
> bool freezable = wq->flags & WQ_FREEZABLE;
> unsigned long flags;
> + int new_max_active;
>
> - /* for @wq->saved_max_active */
> + /* for @wq->saved_max_active and @wq->flags */
> lockdep_assert_held(&wq->mutex);
>
> + if (wq->flags & __WQ_PAUSED)
> + new_max_active = 0;
> + else
> + new_max_active = wq->saved_max_active;
Nothing is using new_max_active and I think we can probably combine this
with the freezing test.
> +void __workqueue_pause_resume(struct workqueue_struct *wq, bool pause)
> +{
> + struct pool_workqueue *pwq;
> +
> + mutex_lock(&wq->mutex);
> + if (pause)
> + wq->flags |= __WQ_PAUSED;
> + else
> + wq->flags &= ~__WQ_PAUSED;
> +
> + for_each_pwq(pwq, wq)
> + pwq_adjust_max_active(pwq);
> + mutex_unlock(&wq->mutex);
> +
> + if (pause)
> + flush_workqueue(wq);
> +}
> +EXPORT_SYMBOL_GPL(__workqueue_pause_resume);
I'd just make pause and resume separate functions. The sharing ratio doesn't
seem that high.
Thanks.
Hi,
> > - /* for @wq->saved_max_active */
> > + /* for @wq->saved_max_active and @wq->flags */
> > lockdep_assert_held(&wq->mutex);
> >
> > + if (wq->flags & __WQ_PAUSED)
> > + new_max_active = 0;
> > + else
> > + new_max_active = wq->saved_max_active;
>
> Nothing is using new_max_active and I think we can probably combine this
> with the freezing test.
Err, yikes. Of course this was meant to be used in the remainder of the
code, oops.
Regarding the freezing test, yeah, maybe. It seemed harder to follow,
but I'll take another look.
> > +void __workqueue_pause_resume(struct workqueue_struct *wq, bool pause)
> > +{
> > + struct pool_workqueue *pwq;
> > +
> > + mutex_lock(&wq->mutex);
> > + if (pause)
> > + wq->flags |= __WQ_PAUSED;
> > + else
> > + wq->flags &= ~__WQ_PAUSED;
> > +
> > + for_each_pwq(pwq, wq)
> > + pwq_adjust_max_active(pwq);
> > + mutex_unlock(&wq->mutex);
> > +
> > + if (pause)
> > + flush_workqueue(wq);
> > +}
> > +EXPORT_SYMBOL_GPL(__workqueue_pause_resume);
>
> I'd just make pause and resume separate functions. The sharing ratio doesn't
> seem that high.
>
Yeah, I wasn't really sure about that either. I keep thinking the
EXPORT_SYMBOL_GPL() itself is really big, but I'm not even sure about
that, and it's probably not a great reason anyway.
johannes
@@ -340,6 +340,7 @@ enum {
__WQ_ORDERED = 1 << 17, /* internal: workqueue is ordered */
__WQ_LEGACY = 1 << 18, /* internal: create*_workqueue() */
__WQ_ORDERED_EXPLICIT = 1 << 19, /* internal: alloc_ordered_workqueue() */
+ __WQ_PAUSED = 1 << 20, /* internal: workqueue_pause() */
WQ_MAX_ACTIVE = 512, /* I like 512, better ideas? */
WQ_MAX_UNBOUND_PER_CPU = 4, /* 4 * #cpus for unbound wq */
@@ -474,6 +475,31 @@ extern void print_worker_info(const char *log_lvl, struct task_struct *task);
extern void show_all_workqueues(void);
extern void show_one_workqueue(struct workqueue_struct *wq);
extern void wq_worker_comm(char *buf, size_t size, struct task_struct *task);
+extern void __workqueue_pause_resume(struct workqueue_struct *wq, bool pause);
+
+/**
+ * workqueue_pause - pause a workqueue
+ * @wq: workqueue to pause
+ *
+ * Pause (and flush) the given workqueue so it's not executing any
+ * work structs and won't until workqueue_resume() is called.
+ */
+static inline void workqueue_pause(struct workqueue_struct *wq)
+{
+ __workqueue_pause_resume(wq, true);
+}
+
+/**
+ * workqueue_resume - resume a paused workqueue
+ * @wq: workqueue to resume
+ *
+ * Resume the given workqueue that was paused previously to
+ * make it run work structs again.
+ */
+static inline void workqueue_resume(struct workqueue_struct *wq)
+{
+ __workqueue_pause_resume(wq, false);
+}
/**
* queue_work - queue work on a workqueue
@@ -3863,10 +3863,16 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
struct workqueue_struct *wq = pwq->wq;
bool freezable = wq->flags & WQ_FREEZABLE;
unsigned long flags;
+ int new_max_active;
- /* for @wq->saved_max_active */
+ /* for @wq->saved_max_active and @wq->flags */
lockdep_assert_held(&wq->mutex);
+ if (wq->flags & __WQ_PAUSED)
+ new_max_active = 0;
+ else
+ new_max_active = wq->saved_max_active;
+
/* fast exit for non-freezable wqs */
if (!freezable && pwq->max_active == wq->saved_max_active)
return;
@@ -4642,6 +4648,25 @@ void workqueue_set_max_active(struct workqueue_struct *wq, int max_active)
}
EXPORT_SYMBOL_GPL(workqueue_set_max_active);
+void __workqueue_pause_resume(struct workqueue_struct *wq, bool pause)
+{
+ struct pool_workqueue *pwq;
+
+ mutex_lock(&wq->mutex);
+ if (pause)
+ wq->flags |= __WQ_PAUSED;
+ else
+ wq->flags &= ~__WQ_PAUSED;
+
+ for_each_pwq(pwq, wq)
+ pwq_adjust_max_active(pwq);
+ mutex_unlock(&wq->mutex);
+
+ if (pause)
+ flush_workqueue(wq);
+}
+EXPORT_SYMBOL_GPL(__workqueue_pause_resume);
+
/**
* current_work - retrieve %current task's work struct
*