Re: [PATCH v2 1/4] workqueue: park kicked worker on pool->kicked_list

[Breno Leitao <leitao@debian.org>]

Hello Tejun,

On Wed, Jun 03, 2026 at 10:50:13PM -1000, Tejun Heo wrote:
> On Wed, Jun 03, 2026 at 06:40:08AM -0700, Breno Leitao wrote:
> > kick_pool() picks an idle worker and wakes it, but leaves it WORKER_IDLE
> > on pool->idle_list until the woken kthread schedules in and runs
> > worker_leave_idle().  idle_cull_fn() only checks WORKER_IDLE, not the
> 
> A lot of the existing comments in the file are double spaced but let's not
> do that anymore. Please use single space to separate sentences in desc and
> comments.

I suppose you mean double space after double space? Sorry, I will avoid
it.

> > task state, so a kicked-but-not-yet-scheduled worker is still a valid
> > cull victim -- the cull can reap it before it consumes the just-enqueued
> > work, stranding the item.  The window is narrow today but later patches
> > in this series defer the wakeup outside pool->lock, widening it.
> 
> Have you actually reproduced this?

No -- not without artificially changing the timeout in the kernel. So far this
is a theoretical race window rather than something I've hit in
practice; it was flagged as a critical issue by sashiko:

    https://sashiko.dev/#/patchset/20260526-fastwake-v1-0-e69ad86923e6%40debian.org

The only way I could get it to actually strand an item was by shrinking
IDLE_WORKER_TIMEOUT 150,000x (300s -> 2ms), so that a worker counts as
"timed out" almost immediately.

> Idle culling never dips below 2 idle workers. Wakeup is from the head
> of the list and culling from the end.

Right -- idle_cull_fn() reaps from the tail (list_last_entry), i.e. the
worker with the oldest last_active, and stops once nr_idle drops to 2,
while kick_pool() wakes the head. So in the common case the kicked
worker (head) and the cull victim (tail) are different workers.

What I'm less sure about: the cull decision is purely time based -- it
reaps the tail worker only if worker->last_active + IDLE_WORKER_TIMEOUT
has elapsed -- but last_active is written in exactly one place,
worker_enter_idle(). Neither kick_pool()/kick_pool_pick() nor
worker_leave_idle() update it; it is only read by the two cull timers.

So being kicked gives a worker no grace period against the cull: a
worker that has already been idle ~IDLE_WORKER_TIMEOUT and is then
kicked still looks expired, and if it drifts off the head (other
workers going idle ahead of it) before it gets a chance to run, the
cull can still reap it.

Would it make sense to refresh last_active when a worker is kicked? The
cull walks tail->head and breaks at the first non-expired worker, so a
freshly-stamped kicked worker would simply be skipped while genuinely
old workers behind it are still reaped.

Thanks for the review,
--breno