[PATCH 0/4] workqueue: cleanups for schedule callbacks part2

[PATCH 0/4] workqueue: cleanups for schedule callbacks part2

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

The commit 6d25be5782e4 ("sched/core, workqueues: Distangle worker
accounting from rq lock") changed the schedule callbacks for workqueue.

It simplified the connection between scheduler and workqueue.  But it
caused some code useless and some comments outdated in workqueue which
needs to be cleaned up.

Patch1-3 are the cleanups based on the fact that 6d25be5782e4 changed
to use pool lock in wq_worker_sleeping().

Patch4 is based on the fact that schedule callbacks were changed to be
only called from schedule() which means all modification to nr_running
is on local CPU when the worker is concurrent managed.

Part1:
https://lore.kernel.org/lkml/20211207073543.61092-1-jiangshanlai@gmail.com/

Most patches of part1 are queued.

Lai Jiangshan (4):
  workqueue: Remove the mb() pair between wq_worker_sleeping() and
    insert_work()
  workqueue: Change the comments of the synchronization about the
    idle_list
  workqueue: Use wake_up_worker() in wq_worker_sleeping() instead of
    open code
  workqueue: Convert the type of pool->nr_running to int

 kernel/workqueue.c | 58 ++++++++++++++++------------------------------
 1 file changed, 20 insertions(+), 38 deletions(-)

-- 
2.19.1.6.gb485710b

[PATCH 1/4] workqueue: Remove the mb() pair between wq_worker_sleeping() and insert_work()

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

In wq_worker_sleeping(), the access to worklist is protected by the
pool->lock, so the memory barrier is unneeded.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 kernel/workqueue.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 33f1106b4f99..29b070106f34 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -918,10 +918,6 @@ void wq_worker_sleeping(struct task_struct *task)
 	}
 
 	/*
-	 * The counterpart of the following dec_and_test, implied mb,
-	 * worklist not empty test sequence is in insert_work().
-	 * Please read comment there.
-	 *
 	 * NOT_RUNNING is clear.  This means that we're bound to and
 	 * running on the local cpu w/ rq lock held and preemption
 	 * disabled, which in turn means that none else could be
@@ -1372,13 +1368,6 @@ static void insert_work(struct pool_workqueue *pwq, struct work_struct *work,
 	list_add_tail(&work->entry, head);
 	get_pwq(pwq);
 
-	/*
-	 * Ensure either wq_worker_sleeping() sees the above
-	 * list_add_tail() or we see zero nr_running to avoid workers lying
-	 * around lazily while there are works to be processed.
-	 */
-	smp_mb();
-
 	if (__need_more_worker(pool))
 		wake_up_worker(pool);
 }
-- 
2.19.1.6.gb485710b

[PATCH 2/4] workqueue: Change the comments of the synchronization about the idle_list

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

The access to idle_list in wq_worker_sleeping() is changed to be
protected by pool->lock, so the comments above idle_list can be changed
to "L:" which is the meaning of "access with pool->lock held".

And the outdated comments in wq_worker_sleeping() is removed since
the function is not called with rq lock held any more, idle_list is
dereferenced with pool lock now.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 kernel/workqueue.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 29b070106f34..b3207722671c 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -162,7 +162,7 @@ struct worker_pool {
 	int			nr_workers;	/* L: total number of workers */
 	int			nr_idle;	/* L: currently idle workers */
 
-	struct list_head	idle_list;	/* X: list of idle workers */
+	struct list_head	idle_list;	/* L: list of idle workers */
 	struct timer_list	idle_timer;	/* L: worker idle timeout */
 	struct timer_list	mayday_timer;	/* L: SOS timer for workers */
 
@@ -826,7 +826,7 @@ static bool too_many_workers(struct worker_pool *pool)
  * Wake up functions.
  */
 
-/* Return the first idle worker.  Safe with preemption disabled */
+/* Return the first idle worker.  Called with pool->lock held. */
 static struct worker *first_idle_worker(struct worker_pool *pool)
 {
 	if (unlikely(list_empty(&pool->idle_list)))
@@ -917,13 +917,6 @@ void wq_worker_sleeping(struct task_struct *task)
 		return;
 	}
 
-	/*
-	 * NOT_RUNNING is clear.  This means that we're bound to and
-	 * running on the local cpu w/ rq lock held and preemption
-	 * disabled, which in turn means that none else could be
-	 * manipulating idle_list, so dereferencing idle_list without pool
-	 * lock is safe.
-	 */
 	if (atomic_dec_and_test(&pool->nr_running) &&
 	    !list_empty(&pool->worklist)) {
 		next = first_idle_worker(pool);
-- 
2.19.1.6.gb485710b

[PATCH 3/4] workqueue: Use wake_up_worker() in wq_worker_sleeping() instead of open code

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

The wakeup code in wq_worker_sleeping() is the same as wake_up_worker().

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 kernel/workqueue.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index b3207722671c..69cbe9e62bf1 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -887,7 +887,7 @@ void wq_worker_running(struct task_struct *task)
  */
 void wq_worker_sleeping(struct task_struct *task)
 {
-	struct worker *next, *worker = kthread_data(task);
+	struct worker *worker = kthread_data(task);
 	struct worker_pool *pool;
 
 	/*
@@ -918,11 +918,8 @@ void wq_worker_sleeping(struct task_struct *task)
 	}
 
 	if (atomic_dec_and_test(&pool->nr_running) &&
-	    !list_empty(&pool->worklist)) {
-		next = first_idle_worker(pool);
-		if (next)
-			wake_up_process(next->task);
-	}
+	    !list_empty(&pool->worklist))
+		wake_up_worker(pool);
 	raw_spin_unlock_irq(&pool->lock);
 }
 
-- 
2.19.1.6.gb485710b

[PATCH 4/4] workqueue: Convert the type of pool->nr_running to int

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

It is only modified in associated CPU, so it doesn't need to be atomic.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 kernel/workqueue.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 69cbe9e62bf1..dd3b3aa68954 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -154,8 +154,13 @@ struct worker_pool {
 
 	unsigned long		watchdog_ts;	/* L: watchdog timestamp */
 
-	/* The current concurrency level. */
-	atomic_t		nr_running;
+	/*
+	 * The current concurrency level.
+	 * increase: process context in associated CPU (preemption disabled).
+	 * decrease and reset: process context in associated CPU & pool->lock.
+	 * read: pool->lock. Ensured to be seen when decreased or reset to zero.
+	 */
+	int			nr_running;
 
 	struct list_head	worklist;	/* L: list of pending works */
 
@@ -777,7 +782,7 @@ static bool work_is_canceling(struct work_struct *work)
 
 static bool __need_more_worker(struct worker_pool *pool)
 {
-	return !atomic_read(&pool->nr_running);
+	return !pool->nr_running;
 }
 
 /*
@@ -802,8 +807,7 @@ static bool may_start_working(struct worker_pool *pool)
 /* Do I need to keep working?  Called from currently running workers. */
 static bool keep_working(struct worker_pool *pool)
 {
-	return !list_empty(&pool->worklist) &&
-		atomic_read(&pool->nr_running) <= 1;
+	return !list_empty(&pool->worklist) && (pool->nr_running <= 1);
 }
 
 /* Do we need a new worker?  Called from manager. */
@@ -873,7 +877,7 @@ void wq_worker_running(struct task_struct *task)
 	 */
 	preempt_disable();
 	if (!(worker->flags & WORKER_NOT_RUNNING))
-		atomic_inc(&worker->pool->nr_running);
+		worker->pool->nr_running++;
 	preempt_enable();
 	worker->sleeping = 0;
 }
@@ -917,8 +921,8 @@ void wq_worker_sleeping(struct task_struct *task)
 		return;
 	}
 
-	if (atomic_dec_and_test(&pool->nr_running) &&
-	    !list_empty(&pool->worklist))
+	pool->nr_running--;
+	if (need_more_worker(pool))
 		wake_up_worker(pool);
 	raw_spin_unlock_irq(&pool->lock);
 }
@@ -973,7 +977,7 @@ static inline void worker_set_flags(struct worker *worker, unsigned int flags)
 	/* If transitioning into NOT_RUNNING, adjust nr_running. */
 	if ((flags & WORKER_NOT_RUNNING) &&
 	    !(worker->flags & WORKER_NOT_RUNNING)) {
-		atomic_dec(&pool->nr_running);
+		pool->nr_running--;
 	}
 
 	worker->flags |= flags;
@@ -1005,7 +1009,7 @@ static inline void worker_clr_flags(struct worker *worker, unsigned int flags)
 	 */
 	if ((flags & WORKER_NOT_RUNNING) && (oflags & WORKER_NOT_RUNNING))
 		if (!(worker->flags & WORKER_NOT_RUNNING))
-			atomic_inc(&pool->nr_running);
+			pool->nr_running++;
 }
 
 /**
@@ -1806,8 +1810,7 @@ static void worker_enter_idle(struct worker *worker)
 		mod_timer(&pool->idle_timer, jiffies + IDLE_WORKER_TIMEOUT);
 
 	/* Sanity check nr_running. */
-	WARN_ON_ONCE(pool->nr_workers == pool->nr_idle &&
-		     atomic_read(&pool->nr_running));
+	WARN_ON_ONCE(pool->nr_workers == pool->nr_idle && pool->nr_running);
 }
 
 /**
@@ -4985,7 +4988,7 @@ static void unbind_workers(int cpu)
 		 * an unbound (in terms of concurrency management) pool which
 		 * are served by workers tied to the pool.
 		 */
-		atomic_set(&pool->nr_running, 0);
+		pool->nr_running = 0;
 
 		/*
 		 * With concurrency management just turned off, a busy
-- 
2.19.1.6.gb485710b

Re: [PATCH 0/4] workqueue: cleanups for schedule callbacks part2

[Lai Jiangshan]

Ping

Re: [PATCH 0/4] workqueue: cleanups for schedule callbacks part2

[Tejun Heo]

On Thu, Dec 23, 2021 at 08:31:36PM +0800, Lai Jiangshan wrote:
> From: Lai Jiangshan <laijs@linux.alibaba.com>
> 
> The commit 6d25be5782e4 ("sched/core, workqueues: Distangle worker
> accounting from rq lock") changed the schedule callbacks for workqueue.
> 
> It simplified the connection between scheduler and workqueue.  But it
> caused some code useless and some comments outdated in workqueue which
> needs to be cleaned up.
> 
> Patch1-3 are the cleanups based on the fact that 6d25be5782e4 changed
> to use pool lock in wq_worker_sleeping().
> 
> Patch4 is based on the fact that schedule callbacks were changed to be
> only called from schedule() which means all modification to nr_running
> is on local CPU when the worker is concurrent managed.

Applied 1-4 to wq/for-5.18. I updated the comment on the last patch.

Thanks.

-- 
tejun