[RFC,1/4] workqueue: support pausing ordered workqueues

Commit Message

Johannes Berg May 10, 2023, 4:04 p.m. UTC

From: Johannes Berg <[e-mail]>

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 <[e-mail]>
---
 include/linux/workqueue.h | 26 ++++++++++++++++++++++++++
 kernel/workqueue.c        | 27 ++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)

Comments

Tejun Heo May 10, 2023, 6:33 p.m. UTC | #1

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.

Johannes Berg May 10, 2023, 6:40 p.m. UTC | #2

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

Patch

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index ac551b8ee7d9..5e2413017a89 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -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
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;
+
 	/* 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
  *