diff mbox series

[04/10] workqueue: Move nr_active handling into helpers

Message ID 20231220072529.1036099-5-tj@kernel.org
State New
Headers
Series [01/10] workqueue: Move pwq->max_active to wq->max_active |

Commit Message

Tejun Heo Dec. 20, 2023, 7:24 a.m. UTC 
  __queue_work(), pwq_dec_nr_in_flight() and wq_adjust_max_active() were
open-coding nr_active handling, which is fine given that the operations are
trivial. However, the planned unbound nr_active update will make them more
complicated, so let's move them into helpers.

- pwq_tryinc_nr_active() is added. It increments nr_active if under
  max_active limit and return a boolean indicating whether inc was
  successful. Note that the function is structured to accommodate future
  changes. __queue_work() is updated to use the new helper.

- pwq_activate_first_inactive() is updated to use pwq_tryinc_nr_active() and
  thus no longer assumes that nr_active is under max_active and returns a
  boolean to indicate whether a work item has been activated.

- wq_adjust_max_active() no longer tests directly whether a work item can be
  activated. Instead, it's updated to use the return value of
  pwq_activate_first_inactive() to tell whether a work item has been
  activated.

- nr_active decrement and activating the first inactive work item is
  factored into pwq_dec_nr_active().

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

Comments

Lai Jiangshan Dec. 26, 2023, 9:12 a.m. UTC | #1 
On Wed, Dec 20, 2023 at 3:25 PM Tejun Heo <tj@kernel.org> wrote:

> +static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq)
>  {
> -       struct work_struct *work = list_first_entry(&pwq->inactive_works,
> -                                                   struct work_struct, entry);
> +       struct workqueue_struct *wq = pwq->wq;
> +       struct worker_pool *pool = pwq->pool;
> +       bool obtained;
>
> -       pwq_activate_work(pwq, work);
> +       lockdep_assert_held(&pool->lock);
> +
> +       obtained = pwq->nr_active < wq->max_active;

It is better to use READ_ONCE(wq->max_active) here in case
the compiler issues code to calculate "obtained" multi-time.

Theoretically, READ_ONCE(wq->max_active) is recommended
from the patch1 in pwq_dec_nr_in_flight() and __queue_work()
since there is no wq->mutex for them.


> +
> +       if (obtained)
> +               pwq->nr_active++;
> +       return obtained;
> +}
> +
Tejun Heo Dec. 26, 2023, 8:06 p.m. UTC | #2 
On Tue, Dec 26, 2023 at 05:12:55PM +0800, Lai Jiangshan wrote:
> On Wed, Dec 20, 2023 at 3:25 PM Tejun Heo <tj@kernel.org> wrote:
> 
> > +static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq)
> >  {
> > -       struct work_struct *work = list_first_entry(&pwq->inactive_works,
> > -                                                   struct work_struct, entry);
> > +       struct workqueue_struct *wq = pwq->wq;
> > +       struct worker_pool *pool = pwq->pool;
> > +       bool obtained;
> >
> > -       pwq_activate_work(pwq, work);
> > +       lockdep_assert_held(&pool->lock);
> > +
> > +       obtained = pwq->nr_active < wq->max_active;
> 
> It is better to use READ_ONCE(wq->max_active) here in case
> the compiler issues code to calculate "obtained" multi-time.
> 
> Theoretically, READ_ONCE(wq->max_active) is recommended
> from the patch1 in pwq_dec_nr_in_flight() and __queue_work()
> since there is no wq->mutex for them.

Yeah, good point. Lemme add WRITE/READ_ONCE() around max_active accesses.

Thanks.
diff mbox series

Patch

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index adec03e63d50..21bd4e4351f7 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1484,12 +1484,66 @@  static bool pwq_activate_work(struct pool_workqueue *pwq,
 	return true;
 }
 
-static void pwq_activate_first_inactive(struct pool_workqueue *pwq)
+/**
+ * pwq_tryinc_nr_active - Try to increment nr_active for a pwq
+ * @pwq: pool_workqueue of interest
+ *
+ * Try to increment nr_active for @pwq. Returns %true if an nr_active count is
+ * successfully obtained. %false otherwise.
+ */
+static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq)
 {
-	struct work_struct *work = list_first_entry(&pwq->inactive_works,
-						    struct work_struct, entry);
+	struct workqueue_struct *wq = pwq->wq;
+	struct worker_pool *pool = pwq->pool;
+	bool obtained;
 
-	pwq_activate_work(pwq, work);
+	lockdep_assert_held(&pool->lock);
+
+	obtained = pwq->nr_active < wq->max_active;
+
+	if (obtained)
+		pwq->nr_active++;
+	return obtained;
+}
+
+/**
+ * pwq_activate_first_inactive - Activate the first inactive work item on a pwq
+ * @pwq: pool_workqueue of interest
+ *
+ * Activate the first inactive work item of @pwq if available and allowed by
+ * max_active limit.
+ *
+ * Returns %true if an inactive work item has been activated. %false if no
+ * inactive work item is found or max_active limit is reached.
+ */
+static bool pwq_activate_first_inactive(struct pool_workqueue *pwq)
+{
+	struct work_struct *work =
+		list_first_entry_or_null(&pwq->inactive_works,
+					 struct work_struct, entry);
+
+	if (work && pwq_tryinc_nr_active(pwq)) {
+		__pwq_activate_work(pwq, work);
+		return true;
+	} else {
+		return false;
+	}
+}
+
+/**
+ * pwq_dec_nr_active - Retire an active count
+ * @pwq: pool_workqueue of interest
+ *
+ * Decrement @pwq's nr_active and try to activate the first inactive work item.
+ */
+static void pwq_dec_nr_active(struct pool_workqueue *pwq)
+{
+	struct worker_pool *pool = pwq->pool;
+
+	lockdep_assert_held(&pool->lock);
+
+	pwq->nr_active--;
+	pwq_activate_first_inactive(pwq);
 }
 
 /**
@@ -1507,14 +1561,8 @@  static void pwq_dec_nr_in_flight(struct pool_workqueue *pwq, unsigned long work_
 {
 	int color = get_work_color(work_data);
 
-	if (!(work_data & WORK_STRUCT_INACTIVE)) {
-		pwq->nr_active--;
-		if (!list_empty(&pwq->inactive_works)) {
-			/* one down, submit an inactive one */
-			if (pwq->nr_active < pwq->wq->max_active)
-				pwq_activate_first_inactive(pwq);
-		}
-	}
+	if (!(work_data & WORK_STRUCT_INACTIVE))
+		pwq_dec_nr_active(pwq);
 
 	pwq->nr_in_flight[color]--;
 
@@ -1811,12 +1859,11 @@  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->wq->max_active)) {
+	if (pwq_tryinc_nr_active(pwq)) {
 		if (list_empty(&pool->worklist))
 			pool->watchdog_ts = jiffies;
 
 		trace_workqueue_activate_work(work);
-		pwq->nr_active++;
 		insert_work(pwq, work, &pool->worklist, work_flags);
 		kick_pool(pool);
 	} else {
@@ -4672,9 +4719,8 @@  static void wq_adjust_max_active(struct workqueue_struct *wq)
 		/* 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);
+		while (pwq_activate_first_inactive(pwq))
+			;
 
 		kick_pool(pwq->pool);