Re: [PATCH v2 1/5] ibmvnic: Move long delayed work on system_dfl_long_wq

Re: [PATCH v2 1/5] ibmvnic: Move long delayed work on system_dfl_long_wq

[Simon Horman]

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?

> 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
[ ... ]
> @@ -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 {