Re: [PATCH V2] workqueue: Process rescuer work items one-by-one using a positional marker

[Tejun Heo]

Hello, Lai.

On Tue, Nov 18, 2025 at 05:38:31PM +0800, Lai Jiangshan wrote:
> @@ -286,6 +286,7 @@ struct pool_workqueue {
>  	struct list_head	pending_node;	/* LN: node on wq_node_nr_active->pending_pwqs */
>  	struct list_head	pwqs_node;	/* WR: node on wq->pwqs */
>  	struct list_head	mayday_node;	/* MD: node on wq->maydays */
> +	struct work_struct	mayday_pos_work;/* L: position on pool->worklist */

Maybe mayday_cursor?

> @@ -1188,6 +1195,15 @@ static bool assign_work(struct work_struct *work, struct worker *worker,
>  
>  	lockdep_assert_held(&pool->lock);
>  
> +	/* The positional work should not be processed */
> +	if (unlikely(work->func == mayday_pos_func)) {
> +		/* only worker_thread() can possibly take this branch */
> +		if (WARN_ON_ONCE(nextp))
> +			*nextp = list_next_entry(work, entry);

I find it confusing to conditionalize the check on @nextp as the fact that
@nextp is only not NULL for worker_thread() is rather incidental. Maybe just
do this in the caller instead?

> +static bool assign_rescue_work(struct pool_workqueue *pwq, struct worker *rescuer)
> +{
> +	struct worker_pool *pool = pwq->pool;
> +	struct work_struct *work, *n;
> +
> +	/* from where to search */
> +	if (list_empty(&pwq->mayday_pos_work.entry))
> +		work = list_first_entry(&pool->worklist, struct work_struct, entry);

Should be fully winged - if () {} else {}. Also, I wonder whether the cursor
handling can be contained on this side. ie. Why does send_mayday() need to
check whether the cursor is on the list?

> +	else {
> +		work = list_next_entry(&pwq->mayday_pos_work, entry);
> +		/* It might be at a new position or not need position anymore */
> +		list_del_init(&pwq->mayday_pos_work.entry);
> +	}
> +
> +	/* need rescue? */
> +	if (!need_to_create_worker(pool))
> +		return false;
> +
> +	/* try to assign a work to rescue */
> +	list_for_each_entry_safe_from(work, n, &pool->worklist, entry) {
> +		if (get_work_pwq(work) == pwq && assign_work(work, rescuer, &n)) {
> +			pwq->stats[PWQ_STAT_RESCUED]++;
> +			/* mark the position for next search */
> +			list_add_tail(&pwq->mayday_pos_work.entry, &n->entry);
> +			return true;
> +		}
> +	}

Would splitting it into two patches make it easier to follow? ie. First
patch to factor out assign_rescuer_work(), the second one to implement
one-at-a-time operation.

>  /* sync @pwq with the current state of its associated wq and link it */
> @@ -6300,6 +6328,8 @@ static void show_pwq(struct pool_workqueue *pwq)
>  
>  	list_for_each_entry(work, &pool->worklist, entry) {
>  		if (get_work_pwq(work) == pwq) {
> +			if (work->func == mayday_pos_func)
> +				continue;

Do we need to skip these? These are debug dumps anyway. Can't we just show
them?

>  			has_pending = true;
>  			break;
>  		}
> @@ -6311,6 +6341,8 @@ static void show_pwq(struct pool_workqueue *pwq)
>  		list_for_each_entry(work, &pool->worklist, entry) {
>  			if (get_work_pwq(work) != pwq)
>  				continue;
> +			if (work->func == mayday_pos_func)
> +				continue;

Ditto.

Thanks.

-- 
tejun