Re: [PATCH net-next V2 06/13] net/mlx5e: Add data direct RX infrastructure

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] A data-direct RQ's KSM UMR mkey has child entries from two
  different PDs and DMA domains.
- [Medium] mlx5e_alloc_rq() calls mlx5e_rq_alloc_data_direct_mkey() right
  after mlx5e_create_rq_umr_mkey().
- [Medium] net/mlx5e: SHAMPO header buffer double-sized in
  mlx5e_shampo_hd_per_wqe()
- [Low] Data-direct RQs are forced into MLX5E_MPWRQ_UMR_MODE_UNALIGNED
  (KSM) by mlx5e_mpwrq_umr_mode().
- [Low] mlx5e_create_umr_mkey() rejects MLX5E_MPWRQ_UMR_MODE_UNALIGNED
  when MLX5_CAP_GEN(mdev, fixed_buffer_size) is missing and logs…

--

net/mlx5e: Add data direct RX infrastructure

This commit adds the RX side infrastructure for data direct in mlx5e. It
adds a new RQ state flag and a crossing mkey over the RQ's UMR mkey. It
also creates the KSM UMR mkey in the data direct PD and sizes the ICOSQ for
the KSM UMR mode. Data direct itself is enabled in a later commit.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/params.c b/drivers/net/ethernet/mellanox/mlx5/core/en/params.c
> index 5caf7820a136b..61a77ee77244a 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/params.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/params.c
> @@ -62,6 +62,9 @@ mlx5e_mpwrq_umr_mode(struct mlx5_core_dev *mdev,
>  	bool unaligned = xsk ? xsk->unaligned : false;
>  	bool oversized = false;
>  
> +	if (rqo && rqo->data_direct)
> +		return MLX5E_MPWRQ_UMR_MODE_UNALIGNED;
> +

[Severity: Medium]
mlx5e_open_channel() only enables data direct together with SHAMPO,
so every data-direct RQ also allocates a SHAMPO header buffer.
Switching those RQs to UNALIGNED here changes the WQE geometry, but
the header buffer sizing doesn't seem to follow.

mlx5_rq_shampo_alloc() sizes the buffer through
mlx5e_shampo_hd_per_wq(), which calls mlx5e_shampo_hd_per_wqe(). That
helper still computes the stride geometry without rqo:

	u16 num_strides = BIT(mlx5e_mpwqe_get_log_num_strides(mdev, params, NULL));
	u8 log_stride_sz = mlx5e_mpwqe_get_log_stride_size(mdev, params, NULL);

With rqo == NULL this uses the ALIGNED (MTT) WQE size. With 4K pages
and 16-byte KSM entries, mlx5e_mpwrq_log_wqe_sz() is one smaller for
the data-direct RQ. Each WQE therefore holds half as many strides and
half as many header reservations.

The log_wq_sz in rq_param->rqc, however, was computed with rqo through
mlx5e_mpwqe_get_log_rq_size(), so the ring has twice as many WQEs.
mlx5e_shampo_hd_per_wq() then multiplies the unhalved hd_per_wqe by
the doubled wq_size, and hd_per_wq comes out twice as large as needed.

Apart from allocating twice the header pages and DMA mappings, this
looks like it can overflow the hardware field. Take an 8192 entry RX
ring with sw_mtu <= 1024. hd_per_wq would go from 32768 to 65536, and
mlx5e_create_rq() would write order_base_2(hd_per_wq) = 16 into the
4-bit log_headers_buffer_entry_num field, which truncates to 0. The
device would then be told the header buffer has one entry while the
driver posts headers across 65536 of them.

Should mlx5e_shampo_hd_per_wqe() take the RQ's rqo
(cparam->rq_opt), so it sees the same UMR mode as the RQ?
mlx5e_shampo_get_log_cq_size() already passes rqo to the same helpers.

[Severity: Low]

This puts data-direct RQs under the KSM limits. Is there anything that
checks the ring size against those limits, the way mlx5e_mpwrq_validate_xsk()
does for XSK?

mlx5e_ethtool_get_ringparam() still computes rx_max_pending from the
MLX5E_MPWRQ_UMR_MODE_ALIGNED geometry. Its comment says only XSK RQs may
clamp the queue length. mlx5e_mpwqe_get_log_rq_size() makes the same
assumption:

	if (WARN_ON_ONCE(params->log_rq_mtu_frames > log_pkts_per_wqe + max_log_rq_size))
		return max_log_rq_size;

KSM entries are twice the size of MTT entries, and max_num_entries is
bounded by 1 << log_max_klm_list_size. Take a device with
log_max_klm_list_size <= 15, an MTU around 9000 and an RX ring of 8192.
log_pkts_per_wqe would be 3 and max_log_rq_size would be 9, so 3 + 9 < 13.

Would that trigger the WARN_ON_ONCE() and quietly shrink the RX ring below
the size ethtool reports? Neither mlx5e_open_channel() nor the later
set_pflag_data_direct() appears to check for this. The comments in both
places would also no longer match the code.

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index 96b521d8bed35..cee73f3b7a997 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -425,7 +425,7 @@ static u8 mlx5e_mpwrq_access_mode(enum mlx5e_mpwrq_umr_mode umr_mode)
>  static int mlx5e_create_umr_mkey(struct mlx5_core_dev *mdev,
>  				 u32 npages, u8 page_shift, u32 *umr_mkey,
>  				 dma_addr_t filler_addr,
> -				 enum mlx5e_mpwrq_umr_mode umr_mode,
> +				 enum mlx5e_mpwrq_umr_mode umr_mode, u32 pdn,
>  				 u32 xsk_chunk_size)
>  {

[Severity: Low]

Data-direct RQs now use UNALIGNED mode, so they hit the existing
fixed_buffer_size check at the top of this function:

	if ((umr_mode == MLX5E_MPWRQ_UMR_MODE_UNALIGNED ||
	     umr_mode == MLX5E_MPWRQ_UMR_MODE_TRIPLE) &&
	    !MLX5_CAP_GEN(mdev, fixed_buffer_size)) {
		mlx5_core_warn(mdev, "Unaligned AF_XDP requires fixed_buffer_size capability\n");
		return -EINVAL;
	}

On a device without fixed_buffer_size, opening a data-direct channel would
fail with -EINVAL and a warning about AF_XDP, which the user never set up.

Should the message be updated? Alternatively, could fixed_buffer_size be
checked before data direct is enabled?

[ ... ]

> @@ -546,13 +548,80 @@ static int mlx5e_create_rq_umr_mkey(struct mlx5_core_dev *mdev, struct mlx5e_rq
>  			      __func__, wq_size, rq->mpwqe.mtts_per_wqe,
>  			      max_num_entries);
>  
> +	pdn = dd ? mdev->data_direct->pdn : mdev->mlx5e_res.hw_objs.pdn;
>  	err = mlx5e_create_umr_mkey(mdev, num_entries, rq->mpwqe.page_shift,
>  				    &umr_mkey, rq->wqe_overflow.addr,
> -				    rq->mpwqe.umr_mode, xsk_chunk_size);
> +				    rq->mpwqe.umr_mode, pdn,
> +				    xsk_chunk_size);

[Severity: Medium]

With dd set, the parent KSM mkey is now created in
mdev->data_direct->pdn. However, mlx5e_create_umr_mkey() still fills every
UNALIGNED entry with the regular NIC mkey and the filler page:

mlx5e_create_umr_mkey() {
    ...
	case MLX5E_MPWRQ_UMR_MODE_UNALIGNED:
		ksm = MLX5_ADDR_OF(create_mkey_in, in, klm_pas_mtt);
		for (i = 0; i < npages; i++)
			ksm[i] = (struct mlx5_ksm) {
				.key = cpu_to_be32(mdev->mlx5e_res.hw_objs.mkey),
				.va = cpu_to_be64(filler_addr),
			};
    ...
}

hw_objs.mkey belongs to hw_objs.pdn. filler_addr is rq->wqe_overflow.addr,
which is mapped with dma_map_page(rq->pdev, ...) against the NIC function.

At runtime, mlx5e_alloc_rx_mpwqe() only rewrites inline_ksms[0] through
inline_ksms[pages_per_wqe - 1] with ksm_child_mkey_be.
mlx5e_mpwrq_mtts_per_wqe() returns ALIGN(pages_per_wqe + 1, ...), so each
WQE has at least one overflow entry that is never rewritten. That entry
still points at hw_objs.mkey.

Does this leave the data-direct KSM mkey with child mkeys from two
different PDs? If the device checks that an indirect mkey and its child
mkeys share a PD, could a packet larger than MTU that writes into the
overflow slot cause a local protection error and put the RQ into error
recovery? The filler page is supposed to absorb such writes.

For comparison, mlx5_ib creates its data-direct KSM mkey in
data_direct->pdn. _mlx5r_umr_update_mr_pas() fills its entries only with
data_direct->mkey or mkey_ro, or with key=0/va=0 on ZAP.

This code looks unchanged at the end of the series.

[ ... ]

> @@ -975,10 +1050,14 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
>  		if (err)
>  			goto err_rq_drop_page;
>  
> -		err = mlx5e_rq_alloc_mpwqe_info(rq, node);
> +		err = mlx5e_rq_alloc_data_direct_mkey(rq);
>  		if (err)
>  			goto err_rq_mkey;

[Severity: Medium]

This creates the crossing mkey right after mlx5e_create_rq_umr_mkey().
At that point the crossed UMR mkey is still in the free state, because
mlx5e_create_umr_mkey() sets:

	MLX5_SET(mkc, mkc, free, 1);

The crossed mkey only becomes valid after the first UMR WQE completes on
the ICOSQ, once the channel is activated.

The other in-tree crossing user is reg_user_mr_dmabuf_by_data_direct() in
drivers/infiniband/hw/mlx5/mr.c, and it uses the opposite order. First,
reg_user_mr_dmabuf() enables the crossed KSM mkey through UMR. Only then is
reg_create_crossing_vhca_mr() called.

Will CREATE_MKEY with MLX5E_MKC_ACCESS_MODE_CROSSING always accept a
crossed mkey that is still free? If firmware checks the crossed mkey's
state at creation time, mlx5e_create_crossing_vhca_mkey() would fail here
for every data-direct RQ.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110183826.2927274-1-tariqt%40nvidia.com