Re: [PATCH v2 1/5] ibmvnic: Move long delayed work on system_dfl_long_wq
From: Simon Horman <horms@kernel.org>
Date: 2026-07-11 16:46:39
Also in:
linux-usb, lkml
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev Simon says: While I think that the pre-existing issue flagged below is orthogonal to this patch, I wonder if you could respond to the question regarding scope raised in the first issue flagged below. --- ibmvnic: Move long delayed work on system_dfl_long_wq This commit changes the code to enqueue delayed reset work items using system_dfl_long_wq instead of the per-cpu system_long_wq. The intent is to allow these unbound, long-running works to benefit from scheduler task placement. [ ... ]
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.
[Severity: Medium]
Will this leave the primary reset path bound to the per-cpu system_long_wq?
While this patch partially migrates delayed reset works to system_dfl_long_wq,
the initial reset work enqueued in ibmvnic_reset() is still bound to the
per-CPU system_long_wq:
drivers/net/ethernet/ibm/ibmvnic.c:ibmvnic_reset() {
...
rwi->reset_reason = reason;
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);
...
}
Should queue_work(system_long_wq, ...) also be migrated to ensure the
reset workload fully benefits from scheduler placement as intended?
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index 5a510eed335ea..a1c01c9820d20 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -3267,7 +3267,7 @@ static void __ibmvnic_reset(struct work_struct *work) spin_lock(&adapter->rwi_lock);
[Severity: High]
This is a pre-existing issue, but can this plain spin_lock() cause a
deadlock against the tasklet context?
The workqueue handler __ibmvnic_reset() acquires adapter->rwi_lock using a
plain spin_lock() without disabling softirqs.
If a hardware interrupt fires on the same CPU while this lock is held, local
softirqs are processed on return, invoking ibmvnic_tasklet().
If the tasklet handles CRQ events via ibmvnic_handle_crq() and triggers a
reset (e.g., IBMVNIC_DEVICE_FAILOVER):
ibmvnic_reset()
spin_lock_irqsave(&adapter->rwi_lock, flags);
Will the tasklet spin forever trying to acquire rwi_lock since the preempted
__ibmvnic_reset() already holds it?
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 {