[PATCH net-next V2] net/mlx5: Use dma_wmb() for completion queue doorbell updates

[PATCH net-next V2] net/mlx5: Use dma_wmb() for completion queue doorbell updates

[Tariq Toukan]

From: Li RongQing <lirongqing@baidu.com>

dma_wmb() barriers are specifically for ordering writes to DMA
coherent memory that is accessible to both the CPU and DMA capable
devices.

The dma_wmb() barrier is lighter than wmb() on some architectures
because it only ensures ordering for DMA writes, not for all writes
including MMIO accesses.

In the MLX5 driver, completion queue (CQ) doorbell records are
allocated as DMA coherent memory via mlx5_dma_zalloc_coherent_node().
The CQ update pattern is:
  1. Update CQ space (device reads via DMA)
  2. Update doorbell record (device reads via DMA)
  3. Memory barrier
  4. Enable more CQEs

Since only DMA coherent memory accesses are involved (no MMIO accesses
follow), can safely use dma_wmb() instead of wmb().

This change improves performance slightly on architectures where
dma_wmb() is lighter than wmb().

Signed-off-by: Li RongQing <lirongqing@baidu.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---

V2: rebased.

V1:
https://patchwork.kernel.org/project/netdevbpf/patch/20260402055206.2311-1-lirongqing@baidu.com/

 drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c    | 2 +-
 drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c    | 2 +-
 drivers/net/ethernet/mellanox/mlx5/core/en_rx.c     | 2 +-
 drivers/net/ethernet/mellanox/mlx5/core/en_tx.c     | 2 +-
 drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c | 2 +-
 drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c   | 2 +-
 drivers/net/ethernet/mellanox/mlx5/core/wc.c        | 2 +-
 7 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
index 723f66a6bd63..17084d76209e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
@@ -259,7 +259,7 @@ static bool mlx5e_ptp_poll_ts_cq(struct mlx5e_cq *cq, int napi_budget)
 	mlx5_cqwq_update_db_record(cqwq);
 
 	/* ensure cq space is freed before enabling more cqes */
-	wmb();
+	dma_wmb();
 
 	while (metadata_buff_sz > 0)
 		mlx5e_ptp_metadata_fifo_push(&ptpsq->metadata_freelist,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
index 77ea51bfbaae..ee0986bd5c18 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
@@ -817,7 +817,7 @@ bool mlx5e_poll_xdpsq_cq(struct mlx5e_cq *cq)
 	mlx5_cqwq_update_db_record(&cq->wq);
 
 	/* ensure cq space is freed before enabling more cqes */
-	wmb();
+	dma_wmb();
 
 	sq->cc = sqcc;
 	return (i == MLX5E_TX_CQ_POLL_BUDGET);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
index fb7110b1b683..442a8d7d6a58 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
@@ -2501,7 +2501,7 @@ int mlx5e_poll_rx_cq(struct mlx5e_cq *cq, int budget)
 	mlx5_cqwq_update_db_record(cqwq);
 
 	/* ensure cq space is freed before enabling more cqes */
-	wmb();
+	dma_wmb();
 
 	return work_done;
 }
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
index 0b5e600e4a6a..14479da98f42 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
@@ -849,7 +849,7 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
 	mlx5_cqwq_update_db_record(&cq->wq);
 
 	/* ensure cq space is freed before enabling more cqes */
-	wmb();
+	dma_wmb();
 
 	sq->dma_fifo_cc = dma_fifo_cc;
 	sq->cc = sqcc;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c b/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
index 1f6bde5d7626..1341874ee58a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
@@ -384,7 +384,7 @@ static inline void mlx5_fpga_conn_cqes(struct mlx5_fpga_conn *conn,
 
 	mlx5_fpga_dbg(conn->fdev, "Re-arming CQ with cc# %u\n", conn->cq.wq.cc);
 	/* ensure cq space is freed before enabling more cqes */
-	wmb();
+	dma_wmb();
 	mlx5_fpga_conn_arm_cq(conn);
 }
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c
index 614cd57a40d7..8f7a89ae4ad8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c
@@ -421,7 +421,7 @@ int mlx5_aso_poll_cq(struct mlx5_aso *aso, bool with_data)
 	mlx5_cqwq_update_db_record(&cq->wq);
 
 	/* ensure cq space is freed before enabling more cqes */
-	wmb();
+	dma_wmb();
 
 	if (with_data)
 		aso->cc += MLX5_ASO_WQEBBS_DATA;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/wc.c b/drivers/net/ethernet/mellanox/mlx5/core/wc.c
index 7d3d4d739e2c..1afbdd19f8de 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/wc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/wc.c
@@ -314,7 +314,7 @@ static void mlx5_wc_post_nop(struct mlx5_wc_sq *sq, unsigned int *offset,
 	/* ensure doorbell record is visible to device before ringing the
 	 * doorbell
 	 */
-	wmb();
+	dma_wmb();
 
 	mlx5_iowrite64_copy(sq, mmio_wqe, sizeof(mmio_wqe), *offset);
 

base-commit: d67e5dbda22604d0fcde32fce58c65f88676e676
-- 
2.44.0

Re: [PATCH net-next V2] net/mlx5: Use dma_wmb() for completion queue doorbell updates

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/mlx5: Use dma_wmb() for completion queue doorbell updates

This converts the barrier that follows mlx5_cqwq_update_db_record() from
wmb() to dma_wmb() in six CQ polling paths, on the grounds that only DMA
coherent memory is involved.  It also converts the barrier in
mlx5_wc_post_nop() in wc.c.

V1: https://patchwork.kernel.org/project/netdevbpf/patch/20260402055206.2311-1-lirongqing@baidu.com/

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index fb7110b1b6834..442a8d7d6a58f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -2501,7 +2501,7 @@ int mlx5e_poll_rx_cq(struct mlx5e_cq *cq, int budget)
>  	mlx5_cqwq_update_db_record(cqwq);
>  
>  	/* ensure cq space is freed before enabling more cqes */
> -	wmb();
> +	dma_wmb();
>  
>  	return work_done;
>  }

[ ... hunks for en/ptp.c, en/xdp.c, en_tx.c, fpga/conn.c and lib/aso.c
snipped, they follow the same shape as the en_rx.c hunk above ... ]

[Severity: Low]
This isn't a bug, but the same "ensure cq space is freed before enabling
more cqes" pattern in mlx5_wc_poll_cq(), in wc.c, is left as wmb():

drivers/net/ethernet/mellanox/mlx5/core/wc.c:mlx5_wc_poll_cq() {
	...
	mlx5_cqwq_update_db_record(&cq->wq);

	/* ensure cq space is freed before enabling more cqes */
	wmb();

	sq->cc++;
	...
}

Was leaving that one out intentional?  After this patch the same commented
rule has two different implementations inside the one file the patch
modifies.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/wc.c b/drivers/net/ethernet/mellanox/mlx5/core/wc.c
> index 7d3d4d739e2cd..1afbdd19f8de2 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/wc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/wc.c
> @@ -314,7 +314,7 @@ static void mlx5_wc_post_nop(struct mlx5_wc_sq *sq, unsigned int *offset,
>  	/* ensure doorbell record is visible to device before ringing the
>  	 * doorbell
>  	 */
> -	wmb();
> +	dma_wmb();
>  
>  	mlx5_iowrite64_copy(sq, mmio_wqe, sizeof(mmio_wqe), *offset);

[Severity: High]
Is this site really covered by the commit message rationale?  The commit
message says:

  "Since only DMA coherent memory accesses are involved (no MMIO accesses
  follow), can safely use dma_wmb() instead of wmb()."

In mlx5_wc_post_nop() an MMIO access does follow immediately.  The barrier
separates the host-memory stores (the NOP WQE and the send doorbell record)
from the BlueFlame store:

	sq->pc++;
	sq->wq.db[MLX5_SND_DBR] = cpu_to_be32(sq->pc);

	/* ensure doorbell record is visible to device before ringing the
	 * doorbell
	 */
	dma_wmb();

	mlx5_iowrite64_copy(sq, mmio_wqe, sizeof(mmio_wqe), *offset);

mlx5_iowrite64_copy() writes into sq->bfreg.map, which is a
write-combining __iomem mapping.  mlx5_core_test_wc() asks for it with
map_wc = true:

	err = mlx5_alloc_bfreg(mdev, &sq->bfreg, true, false);

Does dma_wmb() order the write-back doorbell-record store against that
write-combining MMIO store?  On x86-64, __dma_wmb() is a plain barrier()
while __wmb() is sfence, so this hunk appears to drop the fence that
ordered the two.

Neither branch of mlx5_iowrite64_copy() looks like it restores the
ordering, since both use raw stores: the arm64 path issues a bare NEON
"st1" from inline asm and the fallback uses __iowrite64_copy().

The rest of the driver keeps the two steps distinct, dma_wmb() for
WQE -> doorbell record and wmb() for doorbell record -> MMIO, for example
mlx5e_notify_hw() in en/txrx.h:

	/* ensure wqe is visible to device before updating doorbell record */
	dma_wmb();

	*wq->db = cpu_to_be32(pc);

	/* ensure doorbell record is visible to device before ringing the
	 * doorbell
	 */
	wmb();

	mlx5_write64((__be32 *)ctrl, uar_map);

and mlx5_cq_arm() in include/linux/mlx5/cq.h states the same rule:

	/* Make sure that the doorbell record in host memory is
	 * written before ringing the doorbell via PCI MMIO.
	 */
	wmb();

Can the device then read a stale sq->wq.db[MLX5_SND_DBR] or a stale WQE
after the BlueFlame doorbell arrives?  mlx5_wc_poll_cq() derives
mdev->wc_state from cqe->wqe_counter, so a misordered post would either
time out or mis-classify write-combining support for that device.

Also, the retained comment still says "ensure doorbell record is visible to
device before ringing the doorbell" while the barrier no longer covers the
MMIO side.  Should this hunk be dropped from the patch so wc.c keeps wmb()
here?
-- 
pw-bot: cr