diff mbox series

[01/10] workqueue: Move pwq->max_active to wq->max_active

Message ID	20231220072529.1036099-2-tj@kernel.org
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel+bounces-6472-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3; Sender: Tejun Heo <htejun@gmail.com> From: Tejun Heo <tj@kernel.org> To: jiangshanlai@gmail.com Cc: linux-kernel@vger.kernel.org, Naohiro.Aota@wdc.com, kernel-team@meta.com, Tejun Heo <tj@kernel.org> Subject: [PATCH 01/10] workqueue: Move pwq->max_active to wq->max_active Date: Wed, 20 Dec 2023 16:24:32 +0900 Message-ID: <20231220072529.1036099-2-tj@kernel.org> In-Reply-To: <20231220072529.1036099-1-tj@kernel.org> References: <20231220072529.1036099-1-tj@kernel.org> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX
Series	[01/10] workqueue: Move pwq->max_active to wq->max_active \| [01/10] workqueue: Move pwq->max_active to wq->max_active [02/10] workqueue: Factor out pwq_is_empty() [03/10] workqueue: Replace pwq_activate_inactive_work() with [__]pwq_activate_work() [04/10] workqueue: Move nr_active handling into helpers [05/10] workqueue: Make wq_adjust_max_active() round-robin pwqs while activating [06/10] workqueue: Add first_possible_node and node_nr_cpus[] [07/10] workqueue: Move pwq_dec_nr_in_flight() to the end of work item handling [08/10] workqueue: Introduce struct wq_node_nr_active [09/10] workqueue: Implement system-wide nr_active enforcement for unbound workqueues [10/10] workqueue: Reimplement ordered workqueue using shared nr_active

Commit Message

Tejun Heo Dec. 20, 2023, 7:24 a.m. UTC

  max_active is a workqueue-wide setting and the configured value is stored in
wq->saved_max_active; however, the effective value was stored in
pwq->max_active. While this is harmless, it makes max_active update process
more complicated and gets in the way of the planned max_active semantic
updates for unbound workqueues.

This patches moves pwq->max_active to wq->max_active. This simplifies the
code and makes freezing and noop max_active updates cheaper too. No
user-visible behavior change is intended.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 kernel/workqueue.c | 119 +++++++++++++++++++--------------------------
 1 file changed, 51 insertions(+), 68 deletions(-)

Comments

Lai Jiangshan Dec. 26, 2023, 9:13 a.m. UTC | #1

On Wed, Dec 20, 2023 at 3:25 PM Tejun Heo <tj@kernel.org> wrote:

> +static void wq_adjust_max_active(struct workqueue_struct *wq)
> +{
> +       struct pool_workqueue *pwq;
> +
> +       lockdep_assert_held(&wq->mutex);
> +
> +       if ((wq->flags & WQ_FREEZABLE) && workqueue_freezing) {
> +               wq->max_active = 0;
> +               return;
> +       }
> +
> +       if (wq->max_active == wq->saved_max_active)
> +               return;
> +
> +       wq->max_active = wq->saved_max_active;
> +

If a work item gets queued now, it will get scheduled earlier than a
previous queued one which is still in the inactive list.

To solve it, I recommend adding wq->queue_max_active which will be
updated after the following code and used only when queue_work().
But it requires round-robin through PWQs the second time after
wq->queue_max_active is updated to catch the new inactivated items.

Or just keep pwq->max_active and will be
updated after activating inactivated items and used only when queue_work().

> +       for_each_pwq(pwq, wq) {
> +               unsigned long flags;
> +
> +               /* this function can be called during early boot w/ irq disabled */
> +               raw_spin_lock_irqsave(&pwq->pool->lock, flags);
> +
> +               while (!list_empty(&pwq->inactive_works) &&
> +                      pwq->nr_active < wq->max_active)
> +                       pwq_activate_first_inactive(pwq);
> +
> +               kick_pool(pwq->pool);
> +
> +               raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
> +       }
> +}
> +

Tejun Heo Dec. 26, 2023, 8:05 p.m. UTC | #2

Hello,

On Tue, Dec 26, 2023 at 05:13:40PM +0800, Lai Jiangshan wrote:
> On Wed, Dec 20, 2023 at 3:25 PM Tejun Heo <tj@kernel.org> wrote:
> 
> > +static void wq_adjust_max_active(struct workqueue_struct *wq)
> > +{
> > +       struct pool_workqueue *pwq;
> > +
> > +       lockdep_assert_held(&wq->mutex);
> > +
> > +       if ((wq->flags & WQ_FREEZABLE) && workqueue_freezing) {
> > +               wq->max_active = 0;
> > +               return;
> > +       }
> > +
> > +       if (wq->max_active == wq->saved_max_active)
> > +               return;
> > +
> > +       wq->max_active = wq->saved_max_active;
> > +
> 
> If a work item gets queued now, it will get scheduled earlier than a
> previous queued one which is still in the inactive list.

Is that a problem tho? There's no execution order guarantee except for
ordered workqueues which is not affected by this. In a later change, we
switch to list of pending pwqs instead of work items and the issue ordering
is lost anyway. This isn't a significant departure from previous behaviors
either given that there has never been ordering across pwq boundaries.

> To solve it, I recommend adding wq->queue_max_active which will be
> updated after the following code and used only when queue_work().
> But it requires round-robin through PWQs the second time after
> wq->queue_max_active is updated to catch the new inactivated items.

I'm reluctant to add complications for this given that it's not a real
problem to begin with and the operation is pretty cold.

> Or just keep pwq->max_active and will be
> updated after activating inactivated items and used only when queue_work().

This probably is simpler but would make things more complicated. I'm not
sure it's worth it.

Thanks.

Tejun Heo Dec. 26, 2023, 9:36 p.m. UTC | #3

On Wed, Dec 27, 2023 at 05:05:49AM +0900, Tejun Heo wrote:
> Is that a problem tho? There's no execution order guarantee except for
> ordered workqueues which is not affected by this. In a later change, we
> switch to list of pending pwqs instead of work items and the issue ordering
> is lost anyway. This isn't a significant departure from previous behaviors
> either given that there has never been ordering across pwq boundaries.

Thought more about it and I was wrong. This introduces reordering within pwq
which is new and can break ordered workqueues. Will fix.

Thanks.

diff mbox series

Patch

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 2989b57e154a..0e5dbeeb5778 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -250,7 +250,6 @@  struct pool_workqueue {
 	 * is marked with WORK_STRUCT_INACTIVE iff it is in pwq->inactive_works.
 	 */
 	int			nr_active;	/* L: nr of active works */
-	int			max_active;	/* L: max active works */
 	struct list_head	inactive_works;	/* L: inactive works */
 	struct list_head	pwqs_node;	/* WR: node on wq->pwqs */
 	struct list_head	mayday_node;	/* MD: node on wq->maydays */
@@ -298,7 +297,8 @@  struct workqueue_struct {
 	struct worker		*rescuer;	/* MD: rescue worker */
 
 	int			nr_drainers;	/* WQ: drain in progress */
-	int			saved_max_active; /* WQ: saved pwq max_active */
+	int			max_active;	/* WQ: max active works */
+	int			saved_max_active; /* WQ: saved max_active */
 
 	struct workqueue_attrs	*unbound_attrs;	/* PW: only for unbound wqs */
 	struct pool_workqueue	*dfl_pwq;	/* PW: only for unbound wqs */
@@ -1486,7 +1486,7 @@  static void pwq_dec_nr_in_flight(struct pool_workqueue *pwq, unsigned long work_
 		pwq->nr_active--;
 		if (!list_empty(&pwq->inactive_works)) {
 			/* one down, submit an inactive one */
-			if (pwq->nr_active < pwq->max_active)
+			if (pwq->nr_active < pwq->wq->max_active)
 				pwq_activate_first_inactive(pwq);
 		}
 	}
@@ -1787,7 +1787,7 @@  static void __queue_work(int cpu, struct workqueue_struct *wq,
 	pwq->nr_in_flight[pwq->work_color]++;
 	work_flags = work_color_to_flags(pwq->work_color);
 
-	if (likely(pwq->nr_active < pwq->max_active)) {
+	if (likely(pwq->nr_active < pwq->wq->max_active)) {
 		if (list_empty(&pool->worklist))
 			pool->watchdog_ts = jiffies;
 
@@ -4136,50 +4136,6 @@  static void pwq_release_workfn(struct kthread_work *work)
 	}
 }
 
-/**
- * pwq_adjust_max_active - update a pwq's max_active to the current setting
- * @pwq: target pool_workqueue
- *
- * If @pwq isn't freezing, set @pwq->max_active to the associated
- * workqueue's saved_max_active and activate inactive work items
- * accordingly.  If @pwq is freezing, clear @pwq->max_active to zero.
- */
-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;
-
-	/* for @wq->saved_max_active */
-	lockdep_assert_held(&wq->mutex);
-
-	/* fast exit for non-freezable wqs */
-	if (!freezable && pwq->max_active == wq->saved_max_active)
-		return;
-
-	/* this function can be called during early boot w/ irq disabled */
-	raw_spin_lock_irqsave(&pwq->pool->lock, flags);
-
-	/*
-	 * During [un]freezing, the caller is responsible for ensuring that
-	 * this function is called at least once after @workqueue_freezing
-	 * is updated and visible.
-	 */
-	if (!freezable || !workqueue_freezing) {
-		pwq->max_active = wq->saved_max_active;
-
-		while (!list_empty(&pwq->inactive_works) &&
-		       pwq->nr_active < pwq->max_active)
-			pwq_activate_first_inactive(pwq);
-
-		kick_pool(pwq->pool);
-	} else {
-		pwq->max_active = 0;
-	}
-
-	raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
-}
-
 /* initialize newly allocated @pwq which is associated with @wq and @pool */
 static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct *wq,
 		     struct worker_pool *pool)
@@ -4212,9 +4168,6 @@  static void link_pwq(struct pool_workqueue *pwq)
 	/* set the matching work_color */
 	pwq->work_color = wq->work_color;
 
-	/* sync max_active to the current setting */
-	pwq_adjust_max_active(pwq);
-
 	/* link in @pwq */
 	list_add_rcu(&pwq->pwqs_node, &wq->pwqs);
 }
@@ -4665,6 +4618,46 @@  static int init_rescuer(struct workqueue_struct *wq)
 	return 0;
 }
 
+/**
+ * wq_adjust_max_active - update a wq's max_active to the current setting
+ * @wq: target workqueue
+ *
+ * If @wq isn't freezing, set @wq->max_active to the saved_max_active and
+ * activate inactive work items accordingly. If @wq is freezing, clear
+ * @wq->max_active to zero.
+ */
+static void wq_adjust_max_active(struct workqueue_struct *wq)
+{
+	struct pool_workqueue *pwq;
+
+	lockdep_assert_held(&wq->mutex);
+
+	if ((wq->flags & WQ_FREEZABLE) && workqueue_freezing) {
+		wq->max_active = 0;
+		return;
+	}
+
+	if (wq->max_active == wq->saved_max_active)
+		return;
+
+	wq->max_active = wq->saved_max_active;
+
+	for_each_pwq(pwq, wq) {
+		unsigned long flags;
+
+		/* this function can be called during early boot w/ irq disabled */
+		raw_spin_lock_irqsave(&pwq->pool->lock, flags);
+
+		while (!list_empty(&pwq->inactive_works) &&
+		       pwq->nr_active < wq->max_active)
+			pwq_activate_first_inactive(pwq);
+
+		kick_pool(pwq->pool);
+
+		raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
+	}
+}
+
 __printf(1, 4)
 struct workqueue_struct *alloc_workqueue(const char *fmt,
 					 unsigned int flags,
@@ -4672,7 +4665,6 @@  struct workqueue_struct *alloc_workqueue(const char *fmt,
 {
 	va_list args;
 	struct workqueue_struct *wq;
-	struct pool_workqueue *pwq;
 
 	/*
 	 * Unbound && max_active == 1 used to imply ordered, which is no longer
@@ -4707,6 +4699,7 @@  struct workqueue_struct *alloc_workqueue(const char *fmt,
 
 	/* init wq */
 	wq->flags = flags;
+	wq->max_active = max_active;
 	wq->saved_max_active = max_active;
 	mutex_init(&wq->mutex);
 	atomic_set(&wq->nr_pwqs_to_flush, 0);
@@ -4735,8 +4728,7 @@  struct workqueue_struct *alloc_workqueue(const char *fmt,
 	mutex_lock(&wq_pool_mutex);
 
 	mutex_lock(&wq->mutex);
-	for_each_pwq(pwq, wq)
-		pwq_adjust_max_active(pwq);
+	wq_adjust_max_active(wq);
 	mutex_unlock(&wq->mutex);
 
 	list_add_tail_rcu(&wq->list, &workqueues);
@@ -4874,8 +4866,6 @@  EXPORT_SYMBOL_GPL(destroy_workqueue);
  */
 void workqueue_set_max_active(struct workqueue_struct *wq, int max_active)
 {
-	struct pool_workqueue *pwq;
-
 	/* disallow meddling with max_active for ordered workqueues */
 	if (WARN_ON(wq->flags & __WQ_ORDERED_EXPLICIT))
 		return;
@@ -4886,10 +4876,7 @@  void workqueue_set_max_active(struct workqueue_struct *wq, int max_active)
 
 	wq->flags &= ~__WQ_ORDERED;
 	wq->saved_max_active = max_active;
-
-	for_each_pwq(pwq, wq)
-		pwq_adjust_max_active(pwq);
-
+	wq_adjust_max_active(wq);
 	mutex_unlock(&wq->mutex);
 }
 EXPORT_SYMBOL_GPL(workqueue_set_max_active);
@@ -5135,8 +5122,8 @@  static void show_pwq(struct pool_workqueue *pwq)
 	pr_info("  pwq %d:", pool->id);
 	pr_cont_pool_info(pool);
 
-	pr_cont(" active=%d/%d refcnt=%d%s\n",
-		pwq->nr_active, pwq->max_active, pwq->refcnt,
+	pr_cont(" active=%d refcnt=%d%s\n",
+		pwq->nr_active, pwq->refcnt,
 		!list_empty(&pwq->mayday_node) ? " MAYDAY" : "");
 
 	hash_for_each(pool->busy_hash, bkt, worker, hentry) {
@@ -5684,7 +5671,6 @@  EXPORT_SYMBOL_GPL(work_on_cpu_safe_key);
 void freeze_workqueues_begin(void)
 {
 	struct workqueue_struct *wq;
-	struct pool_workqueue *pwq;
 
 	mutex_lock(&wq_pool_mutex);
 
@@ -5693,8 +5679,7 @@  void freeze_workqueues_begin(void)
 
 	list_for_each_entry(wq, &workqueues, list) {
 		mutex_lock(&wq->mutex);
-		for_each_pwq(pwq, wq)
-			pwq_adjust_max_active(pwq);
+		wq_adjust_max_active(wq);
 		mutex_unlock(&wq->mutex);
 	}
 
@@ -5759,7 +5744,6 @@  bool freeze_workqueues_busy(void)
 void thaw_workqueues(void)
 {
 	struct workqueue_struct *wq;
-	struct pool_workqueue *pwq;
 
 	mutex_lock(&wq_pool_mutex);
 
@@ -5771,8 +5755,7 @@  void thaw_workqueues(void)
 	/* restore max_active and repopulate worklist */
 	list_for_each_entry(wq, &workqueues, list) {
 		mutex_lock(&wq->mutex);
-		for_each_pwq(pwq, wq)
-			pwq_adjust_max_active(pwq);
+		wq_adjust_max_active(wq);
 		mutex_unlock(&wq->mutex);
 	}