netdev - Re: [PATCH net v2] bnxt_en: Fix TX timeout during netpoll.

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: <05E7DE1E-38C8-465C-B4F2-B5EE2612B303@fb.com>
Date:   Wed, 26 Sep 2018 16:11:25 +0000
From:   Song Liu <songliubraving@...com>
To:     Michael Chan <michael.chan@...adcom.com>
CC:     "edumazet@...gle.com" <edumazet@...gle.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net v2] bnxt_en: Fix TX timeout during netpoll.



> On Sep 25, 2018, at 9:41 PM, Michael Chan <michael.chan@...adcom.com> wrote:
> 
> The current netpoll implementation in the bnxt_en driver has problems
> that may miss TX completion events.  bnxt_poll_work() in effect is
> only handling at most 1 TX packet before exiting.  In addition,
> there may be in flight TX completions that ->poll() may miss even
> after we fix bnxt_poll_work() to handle all visible TX completions.
> netpoll may not call ->poll() again and HW may not generate IRQ
> because the driver does not ARM the IRQ when the budget (0 for netpoll)
> is reached.
> 
> We fix it by handling all TX completions and to always ARM the IRQ
> when we exit ->poll() with 0 budget.
> 
> Also, the logic to ACK the completion ring in case it is almost filled
> with TX completions need to be adjusted to take care of the 0 budget
> case, as discussed with Eric Dumazet <edumazet@...gle.com>
> 
> Reported-by: Song Liu <songliubraving@...com>
> Signed-off-by: Michael Chan <michael.chan@...adcom.com>

Reviewed-and-tested-by: Song Liu <songliubraving@...com>


> ---
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 61957b0..0478e56 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -1884,8 +1884,11 @@ static int bnxt_poll_work(struct bnxt *bp, struct bnxt_napi *bnapi, int budget)
> 		if (TX_CMP_TYPE(txcmp) == CMP_TYPE_TX_L2_CMP) {
> 			tx_pkts++;
> 			/* return full budget so NAPI will complete. */
> -			if (unlikely(tx_pkts > bp->tx_wake_thresh))
> +			if (unlikely(tx_pkts > bp->tx_wake_thresh)) {
> 				rx_pkts = budget;
> +				raw_cons = NEXT_RAW_CMP(raw_cons);
> +				break;
> +			}
> 		} else if ((TX_CMP_TYPE(txcmp) & 0x30) == 0x10) {
> 			if (likely(budget))
> 				rc = bnxt_rx_pkt(bp, bnapi, &raw_cons, &event);
> @@ -1913,7 +1916,7 @@ static int bnxt_poll_work(struct bnxt *bp, struct bnxt_napi *bnapi, int budget)
> 		}
> 		raw_cons = NEXT_RAW_CMP(raw_cons);
> 
> -		if (rx_pkts == budget)
> +		if (rx_pkts && rx_pkts == budget)
> 			break;
> 	}
> 
> @@ -2027,8 +2030,12 @@ static int bnxt_poll(struct napi_struct *napi, int budget)
> 	while (1) {
> 		work_done += bnxt_poll_work(bp, bnapi, budget - work_done);
> 
> -		if (work_done >= budget)
> +		if (work_done >= budget) {
> +			if (!budget)
> +				BNXT_CP_DB_REARM(cpr->cp_doorbell,
> +						 cpr->cp_raw_cons);
> 			break;
> +		}
> 
> 		if (!bnxt_has_work(bp, cpr)) {
> 			if (napi_complete_done(napi, work_done))
> -- 
> 2.5.1
>