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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAJhGHyBD2Kbv27ahkbJqMzdHJotCdozmFuGceN+TbW5+rTRyPg@mail.gmail.com>
Date: Fri, 14 Nov 2025 10:41:04 +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] workqueue: Process rescuer work items one-by-one using a
 positional marker

On Fri, Nov 14, 2025 at 1:15 AM Tejun Heo <tj@...nel.org> wrote:
>
> Hello, Lai.
>
> On Fri, Nov 14, 2025 at 12:34:26AM +0800, Lai Jiangshan wrote:
> > +static void insert_mayday_pos(struct pool_workqueue *pwq, struct work_struct *next)
> > +{
> > +     unsigned int work_flags;
> > +     unsigned int work_color;
> > +
> > +     __set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&pwq->mayday_pos_work));
>
> Maybe use test_and_set_bit() here like normal work item?

This code is copied from insert_wq_barrier().
I will change it to use test_and_set_bit().

>
> > +static void remove_mayday_pos(struct pool_workqueue *pwq)
> > +{
> > +     list_del_init(&pwq->mayday_pos_work.entry);
> > +     pwq_dec_nr_in_flight(pwq, *work_data_bits(&pwq->mayday_pos_work));
> > +     INIT_WORK(&pwq->mayday_pos_work, mayday_pos_func);
>
> and maybe we can init the work item once and

It has to clear the pending bit, I think INIT_WORK() is quite convenient to
do so. I will use set_work_pool_and_keep_pending(&pwq->mayday_pos_work,
WORK_OFFQ_POOL_NONE, 0) instead.

remove_mayday_pos() works as the owner rescuer processes/cancels a positional
work without releasing the pool lock.  Maybe the function should be renamed.

>
> >  static void send_mayday(struct work_struct *work)
> >  {
> >       struct pool_workqueue *pwq = get_work_pwq(work);
> > @@ -2992,6 +3037,9 @@ static void send_mayday(struct work_struct *work)
> >       if (!wq->rescuer)
> >               return;
> >
> > +     if (!work_pending(&pwq->mayday_pos_work))
>
> drop this conditional too?
>
> Also, I wonder whether it'd be simpler to think about if we just exclude the
> work item from flush color management. e.g. we can just flag the work item
> and then make work scanning skip them, so that they really are just markers;
> then, we don't have to worry about colors or othre states at all. We just
> insert the marker and use it purely as iteration marker.

I think it should be processable by the normal workers, so that we don't
have to add special code in the fast path in worker_thread(), and it should work
like a normal work item or the wq_barrier.

The ability of processable by the normal workers has an additional benefit:
if it is processed by a normal worker rather than the rescuer, this
indicates that the pool has made forward progress by normal workers
and  the pwq does not need to be rescued until the next mayday event.

Like the wq_barrier, the positional work should not contribute to nr_active,
e.g. it is INACTIVE.

The commit d812796eb390 ("workqueue: Assign a color to barrier work items")
explains why we need a color for the INACTIVE, I don't think it is a good idea
not to assign a color to it.

>
> > +                             /* reset the position and handle the assigned work */
> > +                             if (list_next_entry(&pwq->mayday_pos_work, entry) != n) {
> > +                                     remove_mayday_pos(pwq);
> > +                                     insert_mayday_pos(pwq, n);
>
> and this would become simple list_move_tail().
>

It intended to update the positional work color to avoid back-to-back
work items to
stall the flush_workqueue().

But it did it wrong. Maybe this is better:

if (get_work_color(*work_data_bits(&pwq->mayday_pos_work)) != pwq->work_color) {
{
  remove_mayday_pos(pwq);
  insert_mayday_pos(pwq, n);
} else {
  list_move_tail(&pwq->mayday_pos_work.entry, &n->entry);
}

Thanks
Lai