netdev - Re: [PATCH] qed: avoid spin loops in _qed_mcp_cmd_and

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d9d4b6d1-d64d-4bfb-17d9-b28153e02b9e@gmail.com>
Date:   Wed, 27 Oct 2021 15:25:14 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Caleb Sander <csander@...estorage.com>, netdev@...r.kernel.org
Cc:     Ariel Elior <aelior@...vell.com>, GR-everest-linux-l2@...vell.com,
        Joern Engel <joern@...estorage.com>
Subject: Re: [PATCH] qed: avoid spin loops in _qed_mcp_cmd_and_union()



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@...estorage.com>
> Reviewed-by: Joern Engel <joern@...estorage.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);
>  
>