Message-ID: <CAJhGHyAa2Nexh9y12ARP5NOCYXRCgfvPonjfCuoz_pSJXvPmYA@mail.gmail.com>
Date: Fri, 21 Nov 2025 11:18:29 +0800
From: Lai Jiangshan <jiangshanlai@...il.com>
To: Tejun Heo <tj@...nel.org>
Cc: linux-kernel@...r.kernel.org, Lai Jiangshan <jiangshan.ljs@...group.com>, 
	ying chen <yc1082463@...il.com>
Subject: Re: [PATCH V2] workqueue: Process rescuer work items one-by-one using
 a positional marker

Hello Tejun,

On Fri, Nov 21, 2025 at 10:14 AM Tejun Heo <tj@...nel.org> wrote:
>
> 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?

I will just remove the WARN_ON_ONCE() and the comments.

The check of mayday_pos_func here intends to make it clear that
either the worker is regular or rescuer, it must not move the faked
mayday_pos_func work to its ->scheduled since it is not inserted
by insert_work().

worker_thread() needs this check and removes the cursor.

rescuer_thread() only searches after the cursor, so it will never try
to assign itself with the pwq->mayday_cursor work. But having the
checking is harmless and clearer.

Or may:
    /* only worker_thread() can possibly take this branch */
    WARN_ON_ONCE(worker->rescue_wq);
    if (nextp)
        *nextp = list_next_entry(work, entry);

>
> > +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?


send_mayday() has already traversed the worklist, so it seems reasonable
to insert a cursor to avoid repeating the work. The worklist may be long,
and the first work of a pwq may be near the end.

If the cursor is on the list, it means the rescuer is handling the pwq,
not queuing mayday_node can reduce the work of an extra pool
attach/detach.

Since it is a rescuer, I will remove the check/insertion in send_mayday().

>
> > +     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.

will do.

>
> >  /* 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?

will do.

>
> >                       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.

will do.

Thanks
Lai

Powered by blists - more mailing lists