Re: [PATCH] qed: avoid spin loops in _qed_mcp_cmd_and_union()

[Eric Dumazet <eric.dumazet@gmail.com>]

On 10/27/21 2:45 PM, Caleb Sander wrote:
> By default, qed_mcp_cmd_and_union() sets max_retries to 500K and
> usecs to 10, so these loops can together delay up to 5s.
> We observed thread scheduling delays of over 700ms in production,
> with stacktraces pointing to this code as the culprit.
> 
> Add calls to cond_resched() in both loops to yield the CPU if necessary.
> 
> Signed-off-by: Caleb Sander <csander@purestorage.com>
> Reviewed-by: Joern Engel <joern@purestorage.com>
> ---
>  drivers/net/ethernet/qlogic/qed/qed_mcp.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
> index 24cd41567..d6944f020 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
> @@ -485,10 +485,12 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
>  
>  		spin_unlock_bh(&p_hwfn->mcp_info->cmd_lock);
>  
> -		if (QED_MB_FLAGS_IS_SET(p_mb_params, CAN_SLEEP))
> +		if (QED_MB_FLAGS_IS_SET(p_mb_params, CAN_SLEEP)) {

I do not know this driver, but apparently, there is this CAN_SLEEP test
hinting about being able to sleep.

>  			msleep(msecs);
> -		else
> +		} else {
> +			cond_resched();

Here you might sleep/schedule, while CAN_SLEEP was not set ?

>  			udelay(usecs);


I would suggest using usleep_range() instead, because cond_resched()
can be a NOP under some circumstances.

> +		}
>  	} while (++cnt < max_retries);

Then perhaps not count against max_retries, but based on total elapsed time ?

>  
>  	if (cnt >= max_retries) {
> @@ -517,10 +519,12 @@ _qed_mcp_cmd_and_union(struct qed_hwfn *p_hwfn,
>  		 * The spinlock stays locked until the list element is removed.
>  		 */
>  
> -		if (QED_MB_FLAGS_IS_SET(p_mb_params, CAN_SLEEP))
> +		if (QED_MB_FLAGS_IS_SET(p_mb_params, CAN_SLEEP)) {
>  			msleep(msecs);
> -		else
> +		} else {
> +			cond_resched();
>  			udelay(usecs);
> +		}
>  
>  		spin_lock_bh(&p_hwfn->mcp_info->cmd_lock);
>  
>