[PATCH net-next v4 0/6] net: Move system_long_wq to system_dfl_long_wq

[PATCH net-next v4 0/6] net: Move system_long_wq to system_dfl_long_wq

[Marco Crivellari]

Hello,

This is only rebased as it seems the patches didn't land in the next
release because are marked as Rejected (likely because of initial feedback
on 5/6),despite the general positive overall feedbacks (also on Patch 5/6,
later):

  https://lore.kernel.org/all/20260906080818.GC6356@carbon.k.g/

---

Currently the code uses the per-cpu workqueue system_long_wq to schedule
long running works.

Unbound works could benefit from scheduler task placement, to optimize
performance and power consumption. Another good reason to have this unbound,
is the "queue_delayed_work()" function, used to enqueue the work item.
More details on this will follow in the next section.

Recently, a new unbound workqueue specific for long running work has been
added:

    c116737e972e ("workqueue: Add system_dfl_long_wq for long unbound works")

~~~ Details about queue_delayed_work ~~~

system_long_wq is a per-cpu workqueue and it is used as a parameter of
queue_delayed_work(). This function schedule an item that it will later
be enqueued (once the timer will fire). __queue_delayed_work() does the job
receiving as "cpu" WORK_CPU_UNBOUND:

    if (housekeeping_enabled(HK_TYPE_TIMER)) {
    //      [....]
    } else {
            if (likely(cpu == WORK_CPU_UNBOUND))
                    add_timer_global(timer);
            else
                    add_timer_on(timer, cpu);
    }

The timer is global, so can fire everywhere, and the work item will be
enqueued where the timer fired.

Since the workqueue work doesn't rely on per-cpu variables, there is no
obvious reason that justify the use of a per-cpu workqueue. So change the
workqueue with the new system_dfl_long_wq, so that the used workqueue is
now unbound and can benefit from scheduler task placement.

Thanks!

---
Changes in v4:
- rebased on v7.3-rc7

- added review-by Sebastian on 5/6

Link to v3: https://lore.kernel.org/all/20260720100902.155605-1-marco.crivellari@suse.com/

Changes in v3:
- rebased on v7.2-rc4

- added "net: ti: icssg-prueth: Move long delayed work on system_dfl_long_wq"

- changed also the workqueue in ibmvnic_reset().

Link to v2: https://lore.kernel.org/all/20260706134033.244295-1-marco.crivellari@suse.com/

Changes in v2:
- rebased on v7.2-rc2

- dropped the RFC prefix, kept Ack and review tags

Link to v1: https://lore.kernel.org/all/20260511092846.120141-1-marco.crivellari@suse.com/

Marco Crivellari (6):
  ibmvnic: Move long delayed work on system_dfl_long_wq
  net: ti: icssg-stats: Move long delayed work on system_dfl_long_wq
  net: ti: icssg-prueth: Move long delayed work on system_dfl_long_wq
  net: thunderbolt: Move long delayed work on system_dfl_long_wq
  net: usb: pegasus: Move long delayed work on system_dfl_long_wq
  net: usb: r8152: Move long delayed work on system_dfl_long_wq

 drivers/net/ethernet/ibm/ibmvnic.c           | 6 +++---
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 2 +-
 drivers/net/ethernet/ti/icssg/icssg_stats.c  | 2 +-
 drivers/net/thunderbolt/main.c               | 7 ++++---
 drivers/net/usb/pegasus.c                    | 9 +++++----
 drivers/net/usb/r8152.c                      | 7 ++++---
 6 files changed, 18 insertions(+), 15 deletions(-)

-- 
2.55.0

[PATCH net-next v4 1/6] ibmvnic: Move long delayed work on system_dfl_long_wq

[Marco Crivellari]

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 and it is a per-cpu workqueue.

The function(s) end up calling __queue_delayed_work(), which set a global
timer that could fire anywhere, enqueuing the work where the timer fired.

Unbound works could benefit from scheduler task placement, to optimize
performance and power consumption. Long work shouldn't stick to a single
CPU.

Recently, a new unbound workqueue specific for long running work has
been added:

    c116737e972e ("workqueue: Add system_dfl_long_wq for long unbound works")

Since the workqueue work doesn't rely on per-cpu variables, there is no
obvious reason that justify the use of a per-cpu workqueue. So change
system_long_wq with system_dfl_long_wq so that the work may benefit from
scheduler task placement.

Cc: Haren Myneni <haren@linux.ibm.com>
Cc: Rick Lindsley <ricklind@linux.ibm.com>
Cc: Nick Child <nnac123@linux.ibm.com>
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 5a510eed335e..d4c284c8ef43 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -3229,7 +3229,7 @@ static void __ibmvnic_reset(struct work_struct *work)
 	if (adapter->state == VNIC_PROBING &&
 	    !wait_for_completion_timeout(&adapter->probe_done, timeout)) {
 		dev_err(dev, "Reset thread timed out on probe");
-		queue_delayed_work(system_long_wq,
+		queue_delayed_work(system_dfl_long_wq,
 				   &adapter->ibmvnic_delayed_reset,
 				   IBMVNIC_RESET_DELAY);
 		return;
@@ -3267,7 +3267,7 @@ static void __ibmvnic_reset(struct work_struct *work)
 	spin_lock(&adapter->rwi_lock);
 	if (!list_empty(&adapter->rwi_list)) {
 		if (test_and_set_bit_lock(0, &adapter->resetting)) {
-			queue_delayed_work(system_long_wq,
+			queue_delayed_work(system_dfl_long_wq,
 					   &adapter->ibmvnic_delayed_reset,
 					   IBMVNIC_RESET_DELAY);
 		} else {
@@ -3454,7 +3454,7 @@ static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
 	list_add_tail(&rwi->list, &adapter->rwi_list);
 	netdev_dbg(adapter->netdev, "Scheduling reset (reason %s)\n",
 		   reset_reason_to_string(reason));
-	queue_work(system_long_wq, &adapter->ibmvnic_reset);
+	queue_work(system_dfl_long_wq, &adapter->ibmvnic_reset);
 
 	ret = 0;
 err:
-- 
2.55.0

Re: [PATCH net-next v4 1/6] ibmvnic: Move long delayed work on system_dfl_long_wq

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message does not match all of the diff.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  edits a call inside the affected critical section.
- [High] This problem wasn't introduced by this patch, but
  ibmvnic_reset() can call ibmvnic_close(), which may sleep, from atomic…
- [Medium] This problem wasn't introduced by this patch, but device
  removal can leak queued reset requests.

--

ibmvnic: Move long delayed work on system_dfl_long_wq

This commit moves the ibmvnic reset work items from the per-cpu
system_long_wq to the unbound system_dfl_long_wq. It converts the two
queue_delayed_work() calls in __ibmvnic_reset() and the queue_work() call
in ibmvnic_reset().

> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index 5a510eed335ea..d4c284c8ef437 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c

[ ... ]

> @@ -3267,7 +3267,7 @@ static void __ibmvnic_reset(struct work_struct *work)
>  	spin_lock(&adapter->rwi_lock);
>  	if (!list_empty(&adapter->rwi_list)) {
>  		if (test_and_set_bit_lock(0, &adapter->resetting)) {
> -			queue_delayed_work(system_long_wq,
> +			queue_delayed_work(system_dfl_long_wq,
>  					   &adapter->ibmvnic_delayed_reset,
>  					   IBMVNIC_RESET_DELAY);
>  		} else {

[Severity: High]
This isn't a bug introduced by this patch, but can this plain spin_lock()
on adapter->rwi_lock deadlock against softirq users of the same lock?

__ibmvnic_reset() runs as a work item in process context with BH enabled.
ibmvnic_reset() takes the same lock from softirq context:

ibmvnic_tasklet()
  spin_lock_irqsave(&queue->lock, flags);
  ibmvnic_handle_crq()
    ibmvnic_reset(adapter, VNIC_RESET_MOBILITY);
      spin_lock_irqsave(&adapter->rwi_lock, flags);

The CRQ_INIT and fatal XPORT events in ibmvnic_handle_crq() reach
ibmvnic_reset() the same way. So does the tx watchdog path,
dev_watchdog()->ibmvnic_tx_timeout()->ibmvnic_reset().

Suppose a CRQ interrupt arrives on the worker's CPU while this section
holds rwi_lock. The tasklet can then run on irq_exit and spin forever in
ibmvnic_reset(), with IRQs disabled and crq.lock held.

Would this need spin_lock_bh(), or spin_lock_irqsave() like the other
users? Lockdep should report this as inconsistent {IN-SOFTIRQ-W} ->
{SOFTIRQ-ON-W} usage.

The plain spin_lock() in ibmvnic_remove() looks fine, because it is nested
inside spin_lock_irqsave(&adapter->state_lock, flags).

The plain spin_lock() here appears to date back to commit fd98693cb072
("ibmvnic: Allow queueing resets during probe").

[ ... ]

> @@ -3454,7 +3454,7 @@ static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
>  	list_add_tail(&rwi->list, &adapter->rwi_list);

[Severity: Medium]
This is a pre-existing issue, but can the struct ibmvnic_rwi entries added
to adapter->rwi_list here leak when the device is removed?

ibmvnic_remove() sets adapter->state = VNIC_REMOVING and then does:

	flush_work(&adapter->ibmvnic_reset);
	flush_delayed_work(&adapter->ibmvnic_delayed_reset);
	...
	free_netdev(netdev);

__ibmvnic_reset() returns early without touching the list:

	if (adapter->state == VNIC_REMOVING)
		return;

If the worker is already inside its loop, it frees only the current rwi
and breaks out.

ibmvnic_remove() never calls flush_reset_queue(). Are the remaining
entries lost when free_netdev() frees the adapter? The probe failure path
does call flush_reset_queue() before free_netdev().

>  	netdev_dbg(adapter->netdev, "Scheduling reset (reason %s)\n",
>  		   reset_reason_to_string(reason));
> -	queue_work(system_long_wq, &adapter->ibmvnic_reset);
> +	queue_work(system_dfl_long_wq, &adapter->ibmvnic_reset);

[Severity: Low]
This isn't a bug, but the commit message doesn't seem to cover this hunk:

  ibmvnic: Move long delayed work on system_dfl_long_wq

  Currently the code enqueue work items using {queue|mod}_delayed_work(),
  using system_long_wq.

  The function(s) end up calling __queue_delayed_work(), which set a global
  timer that could fire anywhere, enqueuing the work where the timer fired.

ibmvnic doesn't call mod_delayed_work(). Also, this call site is a plain
queue_work() of ibmvnic_reset, not delayed work, so the timer argument
doesn't apply to it.

Every reset request (CRQ events, tx timeout, change-param, sysfs failover)
goes through this call. Before, it ran on the CPU that submitted it. Now
it runs on an unbound pool.

Could the commit message also mention this conversion?

>  
>  	ret = 0;
>  err:

[Severity: High]
This is a pre-existing issue, but can ibmvnic_reset() sleep in atomic
context on the ENOMEM path just below?

	rwi = kzalloc_obj(*rwi, GFP_ATOMIC);
	if (!rwi) {
		ret = ENOMEM;
		goto err;
	}
	...
err:
	/* ibmvnic_close() below can block, so drop the lock first */
	spin_unlock_irqrestore(&adapter->rwi_lock, flags);

	if (ret == ENOMEM)
		ibmvnic_close(netdev);

For most callers, dropping rwi_lock does not leave atomic context.
ibmvnic_tasklet() holds adapter->crq.lock with spin_lock_irqsave() around
ibmvnic_handle_crq(). ibmvnic_tx_timeout() runs from the dev_watchdog
timer.

When failover_pending is false, the close path can block:

ibmvnic_close()
  __ibmvnic_close()
    set_link_state()
      wait_for_completion_timeout(&adapter->init_done, timeout);
  ibmvnic_cleanup()
    synchronize_rcu();

Wouldn't this trigger "scheduling while atomic"? Also, init_done is
completed by CRQ processing in that same tasklet, which is the context
blocked here.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012095452.135828-1-marco.crivellari%40suse.com

[PATCH net-next v4 2/6] net: ti: icssg-stats: Move long delayed work on system_dfl_long_wq

[Marco Crivellari]

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 and it is a per-cpu workqueue.

The function(s) end up calling __queue_delayed_work(), which set a global
timer that could fire anywhere, enqueuing the work where the timer fired.

Unbound works could benefit from scheduler task placement, to optimize
performance and power consumption. Long work shouldn't stick to a single
CPU.

Recently, a new unbound workqueue specific for long running work has
been added:

    c116737e972e ("workqueue: Add system_dfl_long_wq for long unbound works")

Since the workqueue work doesn't rely on per-cpu variables, there is no
obvious reason that justify the use of a per-cpu workqueue. So change
system_long_wq with system_dfl_long_wq so that the work may benefit from
scheduler task placement.

Cc: MD Danish Anwar <danishanwar@ti.com>
Cc: Roger Quadros <rogerq@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
Reviewed-by: Richard Cheng <icheng@nvidia.com>
---
 drivers/net/ethernet/ti/icssg/icssg_stats.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.c b/drivers/net/ethernet/ti/icssg/icssg_stats.c
index 7159baa0155c..7d6d6692d819 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_stats.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_stats.c
@@ -69,7 +69,7 @@ void icssg_stats_work_handler(struct work_struct *work)
 						stats_work.work);
 	emac_update_hardware_stats(emac);
 
-	queue_delayed_work(system_long_wq, &emac->stats_work,
+	queue_delayed_work(system_dfl_long_wq, &emac->stats_work,
 			   msecs_to_jiffies((STATS_TIME_LIMIT_1G_MS * 1000) / emac->speed));
 }
 EXPORT_SYMBOL_GPL(icssg_stats_work_handler);
-- 
2.55.0

Re: [PATCH net-next v4 2/6] net: ti: icssg-stats: Move long delayed work on system_dfl_long_wq

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The conversion is incomplete, so one delayed_work is queued on
  two different workqueues.

--

net: ti: icssg-stats: Move long delayed work on system_dfl_long_wq

This commit changes the self-requeue in icssg_stats_work_handler() from
the per-cpu system_long_wq to the unbound system_dfl_long_wq. The goal is
to let the periodic stats work benefit from scheduler task placement.

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.c b/drivers/net/ethernet/ti/icssg/icssg_stats.c
> index 7159baa0155cf..7d6d6692d8192 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_stats.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_stats.c
> @@ -69,7 +69,7 @@ void icssg_stats_work_handler(struct work_struct *work)
>  						stats_work.work);
>  	emac_update_hardware_stats(emac);
>  
> -	queue_delayed_work(system_long_wq, &emac->stats_work,
> +	queue_delayed_work(system_dfl_long_wq, &emac->stats_work,
>  			   msecs_to_jiffies((STATS_TIME_LIMIT_1G_MS * 1000) / emac->speed));
>  }

[Severity: Medium]
Is this conversion complete?

Both ICSSG drivers use icssg_stats_work_handler() for the same
emac->stats_work. Both prueth_netdev_init() functions, in icssg_prueth.c
and icssg_prueth_sr1.c, do this:

    INIT_DELAYED_WORK(&emac->stats_work, icssg_stats_work_handler);

However, the first run is still queued on the per-cpu workqueue from
the ndo_open path:

drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c:emac_ndo_open() {
    ...
	queue_work(system_long_wq, &emac->stats_work.work);
    ...
}

The follow-up patch "net: ti: icssg-prueth: Move long delayed work on
system_dfl_long_wq" converts the matching call in icssg_prueth.c. No
patch in the series seems to change icssg_prueth_sr1.c.

On SR1.0 hardware, the first stats run after each open would still go to
a per-cpu worker. Every later run would go to system_dfl_long_wq.

Could mixing the two workqueues also bypass the non-reentrancy check in
__queue_work()?

kernel/workqueue.c:__queue_work() {
    ...
		if (worker && worker->current_pwq->wq == wq) {
    ...
}

The instance running on system_long_wq re-arms onto system_dfl_long_wq,
so this check fails. If the timer expires before the first worker
returns, a second instance could start at the same time on another
worker.

Today this looks harmless. The handler does nothing after
queue_delayed_work(), and emac_update_hardware_stats() takes
prueth->stats_lock.

Should emac_ndo_open() in icssg_prueth_sr1.c also switch to
system_dfl_long_wq, so stats_work is only ever queued on one workqueue?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012095452.135828-1-marco.crivellari%40suse.com

[PATCH net-next v4 3/6] net: ti: icssg-prueth: Move long delayed work on system_dfl_long_wq

[Marco Crivellari]

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 and it is a per-cpu workqueue.

The function(s) end up calling __queue_delayed_work(), which set a global
timer that could fire anywhere, enqueuing the work where the timer fired.

Unbound works could benefit from scheduler task placement, to optimize
performance and power consumption. Long work shouldn't stick to a single
CPU.

Recently, a new unbound workqueue specific for long running work has
been added:

    c116737e972e ("workqueue: Add system_dfl_long_wq for long unbound works")

Since the workqueue work doesn't rely on per-cpu variables, there is no
obvious reason that justify the use of a per-cpu workqueue. So change
system_long_wq with system_dfl_long_wq so that the work may benefit from
scheduler task placement.

Cc: MD Danish Anwar <danishanwar@ti.com>
Cc: Roger Quadros <rogerq@kernel.org>
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
---
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 591be5c8056b..0fba22c1046e 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -1100,7 +1100,7 @@ static int emac_ndo_open(struct net_device *ndev)
 
 	prueth->emacs_initialized++;
 
-	queue_work(system_long_wq, &emac->stats_work.work);
+	queue_work(system_dfl_long_wq, &emac->stats_work.work);
 
 	return 0;
 
-- 
2.55.0

Re: [PATCH net-next v4 3/6] net: ti: icssg-prueth: Move long delayed work on system_dfl_long_wq

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The stats_work conversion is only half done because the SR1
  driver variant was missed.
- [Low] The commit message is boilerplate copied from the delayed-work
  conversions and does not describe this diff.

--

net: ti: icssg-prueth: Move long delayed work on system_dfl_long_wq

This patch changes the first queueing of emac->stats_work in
emac_ndo_open() from the per-CPU system_long_wq to the unbound
system_dfl_long_wq. The goal is to let the scheduler place the work.

> 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 and it is a per-cpu workqueue.
>
> The function(s) end up calling __queue_delayed_work(), which set a global
> timer that could fire anywhere, enqueuing the work where the timer fired.

[Severity: Low]
Does this description match the diff?

The only line changed is in emac_ndo_open() in icssg_prueth.c, and it
uses queue_work(), not {queue|mod}_delayed_work():

    queue_work(system_dfl_long_wq, &emac->stats_work.work);

queue_work() is queue_work_on(WORK_CPU_UNBOUND, wq, work). It queues the
embedded work_struct right away. It arms no timer and does not go through
__queue_delayed_work().

The delayed re-arm that the message describes lives in
icssg_stats_work_handler() in icssg_stats.c. The companion patch
"net: ti: icssg-stats: Move long delayed work on system_dfl_long_wq"
converts that one.

Could the message describe the immediate first kick of stats_work instead,
and mention the companion patch?

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 591be5c8056b4..0fba22c1046e9 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -1100,7 +1100,7 @@ static int emac_ndo_open(struct net_device *ndev)
>
>  	prueth->emacs_initialized++;
>
> -	queue_work(system_long_wq, &emac->stats_work.work);
> +	queue_work(system_dfl_long_wq, &emac->stats_work.work);

[Severity: Low]
Should the SR1 variant of this driver get the same change?

emac_ndo_open() in drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c still
does this:

    queue_work(system_long_wq, &emac->stats_work.work);

Both drivers set up stats_work with the same shared handler:

    INIT_DELAYED_WORK(&emac->stats_work, icssg_stats_work_handler);

After the companion icssg-stats patch, icssg_stats_work_handler() re-arms
itself on the unbound workqueue:

    queue_delayed_work(system_dfl_long_wq, &emac->stats_work,
                       msecs_to_jiffies((STATS_TIME_LIMIT_1G_MS * 1000) / emac->speed));

So on SR1 hardware, the first run after ndo_open goes to the per-CPU
system_long_wq. Every later run goes to system_dfl_long_wq.

The commit message reasoning ("the workqueue work doesn't rely on per-cpu
variables") applies to the SR1 call site too. Nothing else in the series
seems to convert it.

>
>  	return 0;
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012095452.135828-1-marco.crivellari%40suse.com

[PATCH net-next v4 4/6] net: thunderbolt: Move long delayed work on system_dfl_long_wq

[Marco Crivellari]

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 and it is a per-cpu workqueue.

The function(s) end up calling __queue_delayed_work(), which set a global
timer that could fire anywhere, enqueuing the work where the timer fired.

Unbound works could benefit from scheduler task placement, to optimize
performance and power consumption. Long work shouldn't stick to a single
CPU.

Recently, a new unbound workqueue specific for long running work has
been added:

    c116737e972e ("workqueue: Add system_dfl_long_wq for long unbound works")

Since the workqueue work doesn't rely on per-cpu variables, there is no
obvious reason that justify the use of a per-cpu workqueue. So change
system_long_wq with system_dfl_long_wq so that the work may benefit from
scheduler task placement.

Cc: Mika Westerberg <westeri@kernel.org>
Cc: Yehezkel Bernat <YehezkelShB@gmail.com>
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
Acked-by: Mika Westerberg <westeri@kernel.org>
---
 drivers/net/thunderbolt/main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
index d9fb587a62c5..59a4dcf719b8 100644
--- a/drivers/net/thunderbolt/main.c
+++ b/drivers/net/thunderbolt/main.c
@@ -316,7 +316,7 @@ static void start_login(struct tbnet *net)
 	net->login_received = false;
 	mutex_unlock(&net->connection_lock);
 
-	queue_delayed_work(system_long_wq, &net->login_work,
+	queue_delayed_work(system_dfl_long_wq, &net->login_work,
 			   msecs_to_jiffies(1000));
 }
 
@@ -470,7 +470,7 @@ static int tbnet_handle_packet(const void *buf, size_t size, void *data)
 			if (net->login_retries >= TBNET_LOGIN_RETRIES ||
 			    !net->login_sent) {
 				net->login_retries = 0;
-				queue_delayed_work(system_long_wq,
+				queue_delayed_work(system_dfl_long_wq,
 						   &net->login_work, 0);
 			}
 			mutex_unlock(&net->connection_lock);
@@ -722,7 +722,8 @@ static void tbnet_login_work(struct work_struct *work)
 		netdev_dbg(net->dev, "sending login request failed, ret=%d\n",
 			   ret);
 		if (net->login_retries++ < TBNET_LOGIN_RETRIES) {
-			queue_delayed_work(system_long_wq, &net->login_work,
+			queue_delayed_work(system_dfl_long_wq,
+					   &net->login_work,
 					   delay);
 		} else {
 			netdev_info(net->dev, "ThunderboltIP login timed out\n");
-- 
2.55.0

Re: [PATCH net-next v4 4/6] net: thunderbolt: Move long delayed work on system_dfl_long_wq

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Part of the commit message's justification does not match this
  driver.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a
  use-after-free when the driver is torn down.
- [Medium] This problem wasn't introduced by this patch, but
  net->login_retries is accessed without consistent locking.

--

net: thunderbolt: Move long delayed work on system_dfl_long_wq

This commit moves the three queue_delayed_work() calls for login_work in
the thunderbolt networking driver from the per-cpu system_long_wq to the
unbound system_dfl_long_wq. connected_work and disconnect_work stay on
system_long_wq.

> 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 and it is a per-cpu workqueue.
>
> The function(s) end up calling __queue_delayed_work(), which set a global
> timer that could fire anywhere, enqueuing the work where the timer fired.

[Severity: Low]
Does this description match this driver? drivers/net/thunderbolt/main.c
never calls mod_delayed_work().

Also, the call in tbnet_handle_packet() passes a delay of 0. With delay 0,
__queue_delayed_work() never arms a timer:

kernel/workqueue.c:__queue_delayed_work() {
    ...
	if (!delay) {
		__queue_work(cpu, wq, &dwork->work);
		return;
	}
    ...
}

__queue_work() with WORK_CPU_UNBOUND on a per-cpu workqueue then picks
raw_smp_processor_id(). So before this patch, that instance of login_work
ran on the CPU that handled the TBIP_LOGIN packet, not where a timer fired.

The change itself looks fine, because tbnet_login_work() has no per-cpu
dependency. Could the commit message describe the old behaviour more
precisely? The current wording seems to come from a template shared
across the series.

> diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
> index d9fb587a62c5e..59a4dcf719b82 100644
> --- a/drivers/net/thunderbolt/main.c
> +++ b/drivers/net/thunderbolt/main.c

[ ... ]

> @@ -470,7 +470,7 @@ static int tbnet_handle_packet(const void *buf, size_t size, void *data)
>  			if (net->login_retries >= TBNET_LOGIN_RETRIES ||
>  			    !net->login_sent) {
>  				net->login_retries = 0;
> -				queue_delayed_work(system_long_wq,
> +				queue_delayed_work(system_dfl_long_wq,
>  						   &net->login_work, 0);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Can
login_work, connected_work or disconnect_work still be pending when
tbnet_remove() frees the netdev?

drivers/net/thunderbolt/main.c:tbnet_remove() {
	...
	unregister_netdev(net->dev);
	tb_unregister_protocol_handler(&net->handler);
	free_netdev(net->dev);
}

tbnet_stop() cancels disconnect_work, and tbnet_tear_down()->stop_login()
cancels login_work and connected_work. The protocol handler is still
registered at that point, though, so tbnet_handle_packet() can run
afterwards and queue the works again.

tb_unregister_protocol_handler() also does not wait for a callback that is
already running, because tb_xdomain_handle_request() drops xdomain_lock
around the call:

drivers/thunderbolt/xdomain.c:tb_xdomain_handle_request() {
    ...
		mutex_unlock(&xdomain_lock);
		ret = handler->callback(buf, size, handler->data);
		mutex_lock(&xdomain_lock);
    ...
}

The TBIP_LOGOUT case has no netif_running() check:

	queue_work(system_long_wq, &net->disconnect_work);

A TBIP_LOGIN that passed netif_running() just before close can also queue
login_work here, and connected_work right after it.

If the interface is down at remove time, ndo_stop never runs. In that case
a disconnect_work queued by a LOGOUT packet is never cancelled.

After free_netdev(), tbnet_disconnect_work()->tbnet_tear_down(),
tbnet_login_work() or tbnet_connected_work() would use the freed struct
tbnet (connection_lock, net->dev, the rings).

tbnet_suspend() has the same ordering: stop_login() runs before
tb_unregister_protocol_handler().

One option is to unregister the protocol handler and wait for in-flight
callbacks before the final cancel_*_sync() calls. Another is to cancel all
three works again after unregistering and before free_netdev(). Would
either of those work here?

No later patch in the series touches drivers/net/thunderbolt/.

>  			}
>  			mutex_unlock(&net->connection_lock);

[ ... ]

> @@ -722,7 +722,8 @@ static void tbnet_login_work(struct work_struct *work)
>  		netdev_dbg(net->dev, "sending login request failed, ret=%d\n",
>  			   ret);
>  		if (net->login_retries++ < TBNET_LOGIN_RETRIES) {

[Severity: Medium]
This isn't a bug introduced by this patch. Is net->login_retries meant to
be protected by connection_lock?

tbnet_handle_packet() checks and resets it while holding the lock:

	mutex_lock(&net->connection_lock);
	...
	if (net->login_retries >= TBNET_LOGIN_RETRIES ||
	    !net->login_sent) {
		net->login_retries = 0;
		queue_delayed_work(system_dfl_long_wq,
				   &net->login_work, 0);
	}

tbnet_login_work() reads it, increments it here and sets it to 0 on
success, all without connection_lock.

Here is one sequence:

- The worker is doing its final increment (60 -> 61).
- At the same time, the handler resets the counter to 0 and queues
  login_work again. This succeeds because the work is running, not
  pending.
- The worker's store can then overwrite the reset.

The re-queued run then makes a single attempt and prints "ThunderboltIP
login timed out" instead of starting a new retry round.

It recovers when the peer sends another TBIP_LOGIN. login_work already ran
concurrently with the RX handler on system_long_wq, so the workqueue change
neither adds nor removes this race.

> -			queue_delayed_work(system_long_wq, &net->login_work,
> +			queue_delayed_work(system_dfl_long_wq,
> +					   &net->login_work,
>  					   delay);
>  		} else {
>  			netdev_info(net->dev, "ThunderboltIP login timed out\n");

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012095452.135828-1-marco.crivellari%40suse.com

[PATCH net-next v4 5/6] net: usb: pegasus: Move long delayed work on system_dfl_long_wq

[Marco Crivellari]

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 and it is a per-cpu workqueue.

The function(s) end up calling __queue_delayed_work(), which set a global
timer that could fire anywhere, enqueuing the work where the timer fired.

Unbound works could benefit from scheduler task placement, to optimize
performance and power consumption. Long work shouldn't stick to a single
CPU.

Recently, a new unbound workqueue specific for long running work has
been added:

    c116737e972e ("workqueue: Add system_dfl_long_wq for long unbound works")

Since the workqueue work doesn't rely on per-cpu variables, there is no
obvious reason that justify the use of a per-cpu workqueue. So change
system_long_wq with system_dfl_long_wq so that the work may benefit from
scheduler task placement.

Cc: Petko Manolov <petkan@nucleusys.com>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/usb/pegasus.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index aba1a640fc26..ed3f8d555c2a 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -1118,8 +1118,9 @@ static void check_carrier(struct work_struct *work)
 	pegasus_t *pegasus = container_of(work, pegasus_t, carrier_check.work);
 	set_carrier(pegasus->net);
 	if (!(pegasus->flags & PEGASUS_UNPLUG)) {
-		queue_delayed_work(system_long_wq, &pegasus->carrier_check,
-			CARRIER_CHECK_DELAY);
+		queue_delayed_work(system_dfl_long_wq,
+				   &pegasus->carrier_check,
+				   CARRIER_CHECK_DELAY);
 	}
 }
 
@@ -1223,7 +1224,7 @@ static int pegasus_probe(struct usb_interface *intf,
 	res = register_netdev(net);
 	if (res)
 		goto out3;
-	queue_delayed_work(system_long_wq, &pegasus->carrier_check,
+	queue_delayed_work(system_dfl_long_wq, &pegasus->carrier_check,
 			   CARRIER_CHECK_DELAY);
 	dev_info(&intf->dev, "%s, %s, %pM\n", net->name,
 		 info ? info->name : "(unknown)", net->dev_addr);
@@ -1288,7 +1289,7 @@ static int pegasus_resume(struct usb_interface *intf)
 		pegasus->intr_urb->actual_length = 0;
 		intr_callback(pegasus->intr_urb);
 	}
-	queue_delayed_work(system_long_wq, &pegasus->carrier_check,
+	queue_delayed_work(system_dfl_long_wq, &pegasus->carrier_check,
 				CARRIER_CHECK_DELAY);
 	return 0;
 }
-- 
2.55.0

[PATCH net-next v4 6/6] net: usb: r8152: Move long delayed work on system_dfl_long_wq

[Marco Crivellari]

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 and it is a per-cpu workqueue.

The function(s) end up calling __queue_delayed_work(), which set a global
timer that could fire anywhere, enqueuing the work where the timer fired.

Unbound works could benefit from scheduler task placement, to optimize
performance and power consumption. Long work shouldn't stick to a single
CPU.

Recently, a new unbound workqueue specific for long running work has
been added:

    c116737e972e ("workqueue: Add system_dfl_long_wq for long unbound works")

Since the workqueue work doesn't rely on per-cpu variables, there is no
obvious reason that justify the use of a per-cpu workqueue. So change
system_long_wq with system_dfl_long_wq so that the work may benefit from
scheduler task placement.

Cc: Ethan Nelson-Moore <enelsonmoore@gmail.com>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
---
 drivers/net/usb/r8152.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f61686433031..f6af66f294db 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -7072,7 +7072,8 @@ static void rtl_hw_phy_work_func_t(struct work_struct *work)
 
 		/* Delay execution in case request_firmware() is not ready yet.
 		 */
-		queue_delayed_work(system_long_wq, &tp->hw_phy_work, HZ * 10);
+		queue_delayed_work(system_dfl_long_wq, &tp->hw_phy_work,
+				   HZ * 10);
 		goto ignore_once;
 	}
 
@@ -8840,7 +8841,7 @@ static int rtl8152_reset_resume(struct usb_interface *intf)
 	clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 	rtl_reset_ocp_base(tp);
 	tp->rtl_ops.init(tp);
-	queue_delayed_work(system_long_wq, &tp->hw_phy_work, 0);
+	queue_delayed_work(system_dfl_long_wq, &tp->hw_phy_work, 0);
 	set_ethernet_addr(tp, true);
 	return rtl8152_resume(intf);
 }
@@ -10295,7 +10296,7 @@ static int rtl8152_probe_once(struct usb_interface *intf,
 	/* Retry in case request_firmware() is not ready yet. */
 	tp->rtl_fw.retry = true;
 #endif
-	queue_delayed_work(system_long_wq, &tp->hw_phy_work, 0);
+	queue_delayed_work(system_dfl_long_wq, &tp->hw_phy_work, 0);
 	set_ethernet_addr(tp, false);
 
 	usb_set_intfdata(intf, tp);
-- 
2.55.0