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

[mingming cao <mmc@linux.ibm.com>]

On 10/12/26 2:54 AM, 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 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:
On 10/12/26 2:54 AM, 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 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:
I walked through the reset path and the conversion looks fine.

All three system_long_wq users move together and there are no
remaining users in the driver. The work items don't rely on per-CPU
state, so system_dfl_long_wq seems appropriate here.

Minor commit message nit: ibmvnic doesn't use mod_delayed_work(), and
one of the conversions is queue_work() rather than delayed work.

Acked-by: Mingming Cao <mmc@linux.ibm.com>