netdev - Re: [PATCH net 1/4] bnxt: don't lock the tx queue from napi poll

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: <CACKFLin+2hzGOGqt0Qs8HNHnBADooZN=6yt7budLEUn_BhmU5Q@mail.gmail.com>
Date:   Wed, 11 Aug 2021 13:42:40 -0700
From:   Michael Chan <michael.chan@...adcom.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     David Miller <davem@...emloft.net>,
        Jeffrey Huang <huangjw@...adcom.com>,
        Eddie Wai <eddie.wai@...adcom.com>,
        Prashant Sreedharan <prashant@...adcom.com>,
        Andrew Gospodarek <gospo@...adcom.com>,
        Netdev <netdev@...r.kernel.org>,
        Edwin Peer <edwin.peer@...adcom.com>
Subject: Re: [PATCH net 1/4] bnxt: don't lock the tx queue from napi poll

On Wed, Aug 11, 2021 at 12:32 PM Jakub Kicinski <kuba@...nel.org> wrote:

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 865fcb8cf29f..07827d6b0fec 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -730,15 +730,10 @@ static void bnxt_tx_int(struct bnxt *bp, struct bnxt_napi *bnapi, int nr_pkts)
>          */
>         smp_mb();
>
> -       if (unlikely(netif_tx_queue_stopped(txq)) &&
> -           (bnxt_tx_avail(bp, txr) > bp->tx_wake_thresh)) {
> -               __netif_tx_lock(txq, smp_processor_id());
> -               if (netif_tx_queue_stopped(txq) &&
> -                   bnxt_tx_avail(bp, txr) > bp->tx_wake_thresh &&
> -                   txr->dev_state != BNXT_DEV_STATE_CLOSING)
> -                       netif_tx_wake_queue(txq);
> -               __netif_tx_unlock(txq);
> -       }
> +       if (netif_tx_queue_stopped(txq) &&
> +           bnxt_tx_avail(bp, txr) > bp->tx_wake_thresh &&
> +           READ_ONCE(txr->dev_state) != BNXT_DEV_STATE_CLOSING)

This can race with bnxt_start_xmit().  bnxt_start_xmit() can also wake
up the queue when it sees that descriptors are available.  I think
this is the reason we added tx locking here.  The race may be ok
because in the worst case, we will wake up the TX queue when it's not
supposed to wakeup.  If that happens, bnxt_start_xmit() will return
NETDEV_TX_BUSY and stop the queue again when there are not enough TX
descriptors.

> +               netif_tx_wake_queue(txq);
>  }
>
>  static struct page *__bnxt_alloc_rx_page(struct bnxt *bp, dma_addr_t *mapping,

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4209 bytes)