Re: [Intel-wired-lan] [PATCH net v2 1/2] ixgbe: do not busy wait in ixgbe_devlink_reload_empr_finish()

[Paul Menzel]

Dear Linkui,


Thank you for your patch.

Am 17.09.26 um 08:54 schrieb Linkui Xiao:
> From: Linkui Xiao <xiaolinkui@xxxxxxxxxx>
>
> ixgbe_devlink_reload_empr_finish() is the .reload_up devlink operation,
> so it always runs in process context with the devlink instance lock held.
> Its polling loop delays with mdelay(500), i.e. it spins the CPU for half
> a second per iteration and, because the loop bound is 20 iterations, for
> up to ten seconds. That keeps a CPU fully occupied while the firmware
> performs the EMP reset, and on CONFIG_PREEMPT_NONE it also makes the
> loop non-preemptible for that whole window. The loop does not need to be
> atomic and holds no spinlock.
>
> Use msleep() instead.

Should you resend, please document the commands how to test this, for 
example, how to trigger .reload.

> Fixes: c9e563cae19e ("ixgbe: add support for devlink reload")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@xxxxxxxxx>
> Signed-off-by: Linkui Xiao <xiaolinkui@xxxxxxxxxx>
> ---
> v1:https://lore.kernel.org/all/20260914092626.263886-1-xiaolinkui@xxxxxxx/
>
> v2:
>    - Reworded the commit message: mdelay() does not mask interrupts or
>      disable preemption, and the ~10 s window is below the soft lockup /
>      RCU stall thresholds.  State the actual rationale instead.
>    - Split the macro rename into a separate patch.
>    - The mdelay() -> msleep() change itself is unchanged, so the
>      Reviewed-by tag is kept.
>
>   drivers/net/ethernet/intel/ixgbe/devlink/devlink.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> index cf8908b82f8a..781f13240a0d 100644
> --- a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> +++ b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> @@ -460,7 +460,7 @@ static int ixgbe_devlink_reload_empr_finish(struct devlink *devlink,
>   		 * may be not cleared yet, so begin the loop with the delay
>   		 * in order to not check the not updated register.
>   		 */
> -		mdelay(500);
> +		msleep(500);
>   
>   		fwsm = IXGBE_READ_REG(hw, IXGBE_FWSM(hw));


Reviewed-by: Paul Menzel <pmenzel@xxxxxxxxxxxxx>


Kind regards,

Paul