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

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

[Niklas Cassel]

On Thu, Apr 30, 2026 at 11:29:47AM +0200, Marco Crivellari wrote:
> Currently the code enqueue work items using {queue|mod}_delayed_work(),
> using system_long_wq. This workqueue should be used when long works are
> expected, but it is a per-cpu workqueue.
> 
> This is important because queue_delayed_work() queue the work using:
> 
>    queue_delayed_work_on(WORK_CPU_UNBOUND, ...);
> 
> Note that WORK_CPU_UNBOUND = NR_CPUS.
> 
> This would end up calling __queue_delayed_work() that does:
> 
>     if (housekeeping_enabled(HK_TYPE_TIMER)) {
>     //      [....]
>     } else {
>             if (likely(cpu == WORK_CPU_UNBOUND))
>                     add_timer_global(timer);
>             else
>                     add_timer_on(timer, cpu);
>     }
> 
> So when cpu == WORK_CPU_UNBOUND the timer is global and is
> not using a specific CPU. Later, when __queue_work() is called:
> 
>     if (req_cpu == WORK_CPU_UNBOUND) {
>             if (wq->flags & WQ_UNBOUND)
>                     cpu = wq_select_unbound_cpu(raw_smp_processor_id());
>             else
>                     cpu = raw_smp_processor_id();
>     }
> 
> Because the wq is not unbound, it takes the CPU where the timer
> fired and enqueue the work on that CPU.
> The consequence of all of this is that the work can run anywhere,
> depending on where the timer fired.
> 
> Recently, a new unbound workqueue specific for long running work has
> been added:
> 
>     c116737e972e ("workqueue: Add system_dfl_long_wq for long unbound works")
> 
> So change system_long_wq with system_dfl_long_wq so that the work may
> benefit from scheduler task placement.
> 
> Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
> ---
>  drivers/ata/libata-scsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index f44612e269a4..6733f2b14521 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -4753,7 +4753,7 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
>  			     "WARNING: synchronous SCSI scan failed without making any progress, switching to async\n");
>  	}
>  
> -	queue_delayed_work(system_long_wq, &ap->hotplug_task,
> +	queue_delayed_work(system_dfl_long_wq, &ap->hotplug_task,
>  			   round_jiffies_relative(HZ));
>  }
>  
> -- 
> 2.53.0
> 

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.

Do you want us to pick this one up?


Kind regards,
Niklas

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

[Marco Crivellari]

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.

> Do you want us to pick this one up?

Yes please.

Many thanks!

-- 

Marco Crivellari

SUSE Labs

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

[Niklas Cassel]

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.


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?

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.


Kind regards,
Niklas

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

[Frederic Weisbecker]

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