[PATCH 0/2] workqueue: Simplify color management

[PATCH 0/2] workqueue: Simplify color management

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

Use pwq->work_color for wq_barrier which also makes all work items use
pwq->work_color as its color and move the common code into insert_work().

Lai Jiangshan (2):
  workqueue: Use pwq->work_color for wq_barrier
  workqueue: Move flush color assignment code into insert_work()

 kernel/workqueue.c          | 20 +++++---------------
 kernel/workqueue_internal.h |  1 -
 2 files changed, 5 insertions(+), 16 deletions(-)

-- 
2.19.1.6.gb485710b

[PATCH 1/2] workqueue: Use pwq->work_color for wq_barrier

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

wq_barrier work items are internal and do not count as user work items.
They do not participate in flush_workqueue() except for a sanity check,
since wq_barrier owns the PWQ reference. Therefore, any work color is
acceptable; just use the latest pwq->work_color.

See the commit d812796eb390 ("workqueue: Assign a color to barrier work
items").

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 kernel/workqueue.c          | 9 ++-------
 kernel/workqueue_internal.h | 1 -
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 253311af47c6..4d817c7144cf 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3187,7 +3187,6 @@ __acquires(&pool->lock)
 	if (worker->task)
 		worker->current_at = worker->task->se.sum_exec_runtime;
 	work_data = *work_data_bits(work);
-	worker->current_color = get_work_color(work_data);
 
 	/*
 	 * Record wq name for cmdline and debug reporting, may get
@@ -3308,7 +3307,6 @@ __acquires(&pool->lock)
 	worker->current_work = NULL;
 	worker->current_func = NULL;
 	worker->current_pwq = NULL;
-	worker->current_color = INT_MAX;
 
 	/* must be the last step, see the function comment */
 	pwq_dec_nr_in_flight(pwq, work_data);
@@ -3796,7 +3794,6 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
 {
 	static __maybe_unused struct lock_class_key bh_key, thr_key;
 	unsigned int work_flags = 0;
-	unsigned int work_color;
 	struct list_head *head;
 
 	/*
@@ -3826,19 +3823,17 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
 	 */
 	if (worker) {
 		head = worker->scheduled.next;
-		work_color = worker->current_color;
 	} else {
 		unsigned long *bits = work_data_bits(target);
 
 		head = target->entry.next;
 		/* there can already be other linked works, inherit and set */
 		work_flags |= *bits & WORK_STRUCT_LINKED;
-		work_color = get_work_color(*bits);
 		__set_bit(WORK_STRUCT_LINKED_BIT, bits);
 	}
 
-	pwq->nr_in_flight[work_color]++;
-	work_flags |= work_color_to_flags(work_color);
+	pwq->nr_in_flight[pwq->work_color]++;
+	work_flags |= work_color_to_flags(pwq->work_color);
 
 	insert_work(pwq, &barr->work, head, work_flags);
 }
diff --git a/kernel/workqueue_internal.h b/kernel/workqueue_internal.h
index f6275944ada7..76d2d99b26ab 100644
--- a/kernel/workqueue_internal.h
+++ b/kernel/workqueue_internal.h
@@ -32,7 +32,6 @@ struct worker {
 	work_func_t		current_func;	/* K: function */
 	struct pool_workqueue	*current_pwq;	/* K: pwq */
 	u64			current_at;	/* K: runtime at start or last wakeup */
-	unsigned int		current_color;	/* K: color */
 
 	int			sleeping;	/* S: is worker sleeping? */
 
-- 
2.19.1.6.gb485710b

Re: [PATCH 1/2] workqueue: Use pwq->work_color for wq_barrier

[Tejun Heo]

Hello,

On Tue, Nov 25, 2025 at 06:31:24PM +0800, Lai Jiangshan wrote:
> From: Lai Jiangshan <jiangshan.ljs@antgroup.com>
> 
> wq_barrier work items are internal and do not count as user work items.
> They do not participate in flush_workqueue() except for a sanity check,
> since wq_barrier owns the PWQ reference. Therefore, any work color is
> acceptable; just use the latest pwq->work_color.

Maybe it is but I really don't like it. It becomes a lot harder to think
about and that makes things more fragile in the long term. I think it should
either not participate at all or do something straightforward like
inheriting the color of the work item it's flushing.

Even just thinking about the original problem that added flush color to
barrier work items becomes trickier. We can no longer just think that "oh
yeah, no new work items and all barriers match the target work items, so
flushing should wait for the whole thing". It now becomes "what happens if a
new flush_work() is queued after the latest flush_workqueue()? does that
still wait for that new barrier item?". In fact, I'm not sure it does. So,
please don't do this. Let's keep things conceptually straightforward as much
as possible even if that costs a bit more code. Code is often a lot cheaper
than cognitive overhead.

Thanks.

-- 
tejun

Re: [PATCH 1/2] workqueue: Use pwq->work_color for wq_barrier

[Lai Jiangshan]

Hello, Tejun,

On Wed, Dec 3, 2025 at 1:37 AM Tejun Heo <tj@kernel.org> wrote:
> "oh
> yeah, no new work items and all barriers match the target work items, so
> flushing should wait for the whole thing".

This is only true after commit d812796eb390 ("workqueue: Assign
a color to barrier work items") is applied.

Users should not depend on this behavior; if they do, their code
is already fragile.

Anyway, I’ll withdraw this patchset.

Thanks
Lai

[PATCH 2/2] workqueue: Move flush color assignment code into insert_work()

[Lai Jiangshan]

From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

Move the open code into insert_work() and simplify its callers.

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 kernel/workqueue.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 4d817c7144cf..2654fbd481a1 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2179,6 +2179,9 @@ static void insert_work(struct pool_workqueue *pwq, struct work_struct *work,
 	/* record the work call stack in order to print it in KASAN reports */
 	kasan_record_aux_stack(work);
 
+	pwq->nr_in_flight[pwq->work_color]++;
+	extra_flags |= work_color_to_flags(pwq->work_color);
+
 	/* we own @work, set data and link */
 	set_work_pwq(work, pwq, extra_flags);
 	list_add_tail(&work->entry, head);
@@ -2231,7 +2234,6 @@ static void __queue_work(int cpu, struct workqueue_struct *wq,
 {
 	struct pool_workqueue *pwq;
 	struct worker_pool *last_pool, *pool;
-	unsigned int work_flags;
 	unsigned int req_cpu = cpu;
 
 	/*
@@ -2319,9 +2321,6 @@ static void __queue_work(int cpu, struct workqueue_struct *wq,
 	if (WARN_ON(!list_empty(&work->entry)))
 		goto out;
 
-	pwq->nr_in_flight[pwq->work_color]++;
-	work_flags = work_color_to_flags(pwq->work_color);
-
 	/*
 	 * Limit the number of concurrently active work items to max_active.
 	 * @work must also queue behind existing inactive work items to maintain
@@ -2332,11 +2331,10 @@ static void __queue_work(int cpu, struct workqueue_struct *wq,
 			pool->watchdog_ts = jiffies;
 
 		trace_workqueue_activate_work(work);
-		insert_work(pwq, work, &pool->worklist, work_flags);
+		insert_work(pwq, work, &pool->worklist, 0);
 		kick_pool(pool);
 	} else {
-		work_flags |= WORK_STRUCT_INACTIVE;
-		insert_work(pwq, work, &pwq->inactive_works, work_flags);
+		insert_work(pwq, work, &pwq->inactive_works, WORK_STRUCT_INACTIVE);
 	}
 
 out:
@@ -3832,9 +3830,6 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
 		__set_bit(WORK_STRUCT_LINKED_BIT, bits);
 	}
 
-	pwq->nr_in_flight[pwq->work_color]++;
-	work_flags |= work_color_to_flags(pwq->work_color);
-
 	insert_work(pwq, &barr->work, head, work_flags);
 }
 
-- 
2.19.1.6.gb485710b