Re: [RFC PATCH] ata: libata-scsi: Move long delayed work on system_dfl_long_wq

[Frederic Weisbecker <frederic@kernel.org>]

Le Mon, May 11, 2026 at 06:39:37PM +0200, Niklas Cassel a écrit :
> On Mon, May 11, 2026 at 02:54:26PM +0200, Marco Crivellari wrote:
> > On Mon, May 11, 2026 at 2:48 PM Niklas Cassel <cassel@kernel.org> wrote:
> > > [...]
> > > Looks good to me.
> > >
> > > Any particular reason that you sent this as an RFC?
> > >
> > > I can see similar patches queued up in linux-next already.
> > 
> > I just wanted to be sure I didn't miss any other reason for being
> > per-cpu, and in case
> > receive comments on it.
> 
> Hmm... I can see that:
> drivers/ata/libata-eh.c:ata_scsi_port_error_handler()
> does:
> 
> schedule_delayed_work(&ap->hotplug_task, 0);
> 
> schedule_delayed_work() does:
> queue_delayed_work(system_percpu_wq, dwork, delay);
> 
> So this will schedule the work on a per-cpu workqueue.

Hmm, yes by accident because the delay is 0 so it will queue
to the current CPU.

 
> It seems that we are already queueing the same work (&ap->hotplug_task)
> on different workqueues, so I guess that is fine.
> 
> Right now, both workqueues are per-cpu. Is it fine to change one of them
> to be not be bound to a specific CPU?

Well, is there a reason why it is scheduled to the long work pool on one
hand and to the default pool on the other end? Should the behaviour be
consolidated to always use the unbound long work pool?

> From looking at the work, ata_scsi_hotplug(), I can't think of a reason
> why this would have to run on the same CPU as the CPU that queued the
> work.
> 
> From looking at workqueue.h:
> 
>  * system_dfl_wq is unbound workqueue.  Workers are not bound to
>  * any specific CPU, not concurrency managed, and all queued works are
>  * executed immediately as long as max_active limit is not reached and
>  * resources are available.
> 
>  [...]
> 
>  * system_dfl_long_wq is similar to system_dfl_wq but it may host long running
>  * works.
> 
> "not concurrency managed"
> 
> That sounds like a big change, since the per-cpu workqueues do seem to be
> concurrency managed (unlike the _dfl_ ones).
> 
> However, considering that the work (&ap->hotplug_task / ata_scsi_hotplug())
> does:
> mutex_lock(&ap->scsi_scan_mutex);
> 
> I also don't see a problem with the workqueue not being concurrency managed,
> since the work is taking a mutex anyway.
> 
> 
> 
> If anyone sees a problem, please say something, otherwise intend to queue
> this up in a few days.

Thanks!

-- 
Frederic Weisbecker
SUSE Labs