netdev - Re: [PATCH net-next v4 6/7] bnxt: use new queue try_stop/try

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]

Date:   Fri, 7 Apr 2023 11:25:23 +0200
From:   Eric Dumazet <edumazet@...gle.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, pabeni@...hat.com,
        herbert@...dor.apana.org.au, alexander.duyck@...il.com,
        hkallweit1@...il.com, andrew@...n.ch, willemb@...gle.com,
        Michael Chan <michael.chan@...adcom.com>
Subject: Re: [PATCH net-next v4 6/7] bnxt: use new queue try_stop/try_wake macros

On Fri, Apr 7, 2023 at 3:25 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> Convert bnxt to use new macros rather than open code the logic.
> Two differences:
> (1) bnxt_tx_int() will now only issue a memory barrier if it sees
>     enough space on the ring to wake the queue. This should be fine,
>     the mb() is between the writes to the ring pointers and checking
>     queue state.
> (2) we'll start the queue instead of waking on race, this should
>     be safe inside the xmit handler.

...

>                                    "bnxt: ring busy w/ flush pending!\n");
> -               if (bnxt_txr_netif_try_stop_queue(bp, txr, txq))
> +               if (!netif_txq_try_stop(txq, bnxt_tx_avail(bp, txr),
> +                                       bp->tx_wake_thresh))
>                         return NETDEV_TX_BUSY;
>         }
>

BTW, we should make sure the @desc argument of netif_txq_try_stop() is
correctly implemented.
I often see drivers with either buggy or not efficient implementation.

For instance, bnxt could perhaps be slightly described more accurately with:

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 18cac98ba58e86c53eb87ed592424ed719b2f892..ec4a2d4dea3c58de08e51c1d65b3d2fd75d71e6a
100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -2231,13 +2231,12 @@ struct bnxt {
 #define SFF_MODULE_ID_QSFP28                   0x11
 #define BNXT_MAX_PHY_I2C_RESP_SIZE             64

-static inline u32 bnxt_tx_avail(struct bnxt *bp, struct bnxt_tx_ring_info *txr)
+static inline u32 bnxt_tx_avail(const struct bnxt *bp,
+                               const struct bnxt_tx_ring_info *txr)
 {
-       /* Tell compiler to fetch tx indices from memory. */
-       barrier();
+       u32 used = READ_ONCE(txr->tx_prod) - READ_ONCE(txr->tx_cons);

-       return bp->tx_ring_size -
-               ((txr->tx_prod - txr->tx_cons) & bp->tx_ring_mask);
+       return bp->tx_ring_size - (used & bp->tx_ring_mask);
 }

 static inline void bnxt_writeq(struct bnxt *bp, u64 val,


In the same vein, mlx4 would probably need something like:

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index 2f79378fbf6ec106bf78d0fd12e911ff200ba8d7..13557701e254e7aeff057abd26a88d19c3a5cd69
100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
@@ -226,9 +226,11 @@ void mlx4_en_deactivate_tx_ring(struct mlx4_en_priv *priv,
                       MLX4_QP_STATE_RST, NULL, 0, 0, &ring->sp_qp);
 }

-static inline bool mlx4_en_is_tx_ring_full(struct mlx4_en_tx_ring *ring)
+static inline bool mlx4_en_is_tx_ring_full(const struct mlx4_en_tx_ring *ring)
 {
-       return ring->prod - ring->cons > ring->full_size;
+       u32 used = READ_ONCE(ring->prod) - READ_ONCE(ring->cons);
+
+       return used > ring->full_size;
 }

 static void mlx4_en_stamp_wqe(struct mlx4_en_priv *priv,