Re: [PATCH net-next 05/10] 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: 1 · Medium: 2 · 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).

- [High] In data-direct mode the ICOSQ is too small for the UMR WQEs
  posted to it, so the ring overruns.
- [Medium] In data-direct mode, mlx5e_create_rq_umr_mkey() now creates
  the KSM UMR mkey in mdev->data_direct->pdn.
- [Medium] The data-direct RX path dereferences the per-channel
  mdev->data_direct without a NULL check: ->pdn in…
- [Low] The commit message describes the mkey chain incorrectly.
- [Low] Data-direct RQs now always use the UNALIGNED (KSM) UMR mode, but
  nothing checks that mode's requirements before the channel is opened.

--

net/mlx5e: Add data direct RX infrastructure

This commit adds the RX-side infrastructure for data direct mode. It adds
an RQ state flag and creates a crossing mkey that the RQ WQEs use as their
lkey. It also switches data direct RQs to a KSM UMR mkey in the data
direct PD, whose entries point to the data direct mkey.

> For data direct to work on the RX path, the RQ will use a KSM mkey whose
> entries will point to the crossed mkey. This is necessary to enable DMA
> from the NIC to the other device.

[ ... ]

> - Create KSM mkey and in data direct PD and make it point to the
>   crossed mkey.

[Severity: Low]
This isn't a bug, but does this description match the code?

In mlx5e_rq_alloc_data_direct_mkey(), the KSM UMR mkey itself is passed
as the crossed mkey:

	umr_mkey = be32_to_cpu(rq->mpwqe.umr_mkey_be);
	...
	return mlx5e_create_crossing_vhca_mkey(mdev, umr_mkey,

The KSM entries written in mlx5e_alloc_rx_mpwqe() point to
mdev->data_direct->mkey or ->mkey_ro (ksm_child_mkey_be), not to the
crossed mkey.

So the chain in the code looks like:

  RQ lkey -> crossing mkey (hw_objs.pdn)
          -> crossed KSM UMR mkey (data_direct->pdn)
          -> data direct mkey

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/params.c b/drivers/net/ethernet/mellanox/mlx5/core/en/params.c
> index 5caf7820a136b..88b4cd5497d88 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: Low]
Data direct RQs now always use the UNALIGNED (KSM) mode. Is this mode
validated anywhere before the channel is opened?

mlx5e_mpwrq_validate_regular() still checks only the ALIGNED mode:

	enum mlx5e_mpwrq_umr_mode umr_mode = mlx5e_mpwrq_umr_mode(mdev, NULL);

set_pflag_data_direct(), added later in the series, doesn't check
fixed_buffer_size either. The first check happens during
mlx5e_alloc_rq(), in mlx5e_create_umr_mkey():

	mlx5_core_warn(mdev, "Unaligned AF_XDP requires fixed_buffer_size capability\n");
	return -EINVAL;

On a device without fixed_buffer_size, a data direct RQ with no AF_XDP in
use would fail channel open with this AF_XDP-specific message.

The KSM limit derived from log_max_klm_list_size isn't checked up front
either. mlx5e_create_rq_umr_mkey() only warns when num_entries is larger
than max_num_entries, and then continues.

[ ... ]

> @@ -1296,12 +1299,14 @@ void mlx5e_build_xdpsq_param(struct mlx5_core_dev *mdev,
>  int mlx5e_build_channel_param(struct mlx5_core_dev *mdev,
>  			      struct mlx5e_params *params,
>  			      struct netdev_queue_config *qcfg,
> +			      bool data_direct,
>  			      struct mlx5e_channel_param *cparam)
>  {
>  	u8 icosq_log_wq_sz, async_icosq_log_wq_sz;
>  	int err;
>  
>  	cparam->rq_opt.qcfg = qcfg;
> +	cparam->rq_opt.data_direct = data_direct;
>  
>  	err = mlx5e_build_rq_param(mdev, params, &cparam->rq_opt, &cparam->rq);
>  	if (err)

[Severity: High]
The RQ parameters are now built with rq_opt.data_direct set, so the RQ
geometry is computed for KSM. Is the ICOSQ still sized for the MTT layout?

The next call, mlx5e_build_icosq_log_wq_sz(), does:

	/* UMR WQEs for the regular RQ. */
	wqebbs = mlx5e_mpwrq_total_umr_wqebbs(mdev, params, NULL);

With rqo == NULL, mlx5e_mpwrq_umr_mode() returns the ALIGNED mode, so the
budget assumes 8-byte MTT entries. KSM entries are 16 bytes. That halves
the maximum pages per WQE in mlx5e_mpwrq_log_wqe_sz(), which doubles the
RQ WQE count from mlx5e_mpwqe_get_log_rq_size(). Where the log WQE size
is capped at 17 (ECPF), the per-UMR WQEBB count grows instead.

With defaults (MTU 1500, log_rq_mtu_frames=10, 64-byte cachelines), the
MTT budget is 8 WQEs * 10 WQEBBs = 80. That rounds up to a 128-WQEBB
ICOSQ. The KSM RQ has 16 WQEs, so the first fill does:

mlx5e_post_rx_mpwqes()
  mlx5e_alloc_rx_mpwqe()   /* 15 times, 10 WQEBBs each */

That posts about 150 WQEBBs plus NOP padding in one batch before the
doorbell. MTU 9000 has the same problem: about 1270 WQEBBs are needed
for a 1024-WQEBB ICOSQ.

Neither mlx5e_post_rx_mpwqes() nor mlx5e_icosq_get_next_pi() checks for
free space. Can the producer then wrap onto UMR WQEs and sq->db.wqe_info[]
entries that the hardware hasn't completed yet?

This still seems to be the case at the end of the series. There,
mlx5e_build_icosq_log_wq_sz() still passes NULL, and the commit
"net/mlx5e: Enable the data direct netdev feature" makes this path
reachable.

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index b86c7b52774a0..e0d46e067c720 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c

[ ... ]

> @@ -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]
The KSM UMR mkey is now created in mdev->data_direct->pdn. However,
mlx5e_create_umr_mkey() still fills every UNALIGNED entry with a child
key from the regular PD:

	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),
			};

mlx5e_alloc_rx_mpwqe() rewrites only the first pages_per_wqe entries of
each WQE with ksm_child_mkey_be.

mlx5e_mpwrq_mtts_per_wqe() reserves extra entries per WQE
(ALIGN(pages_per_wqe + 1, 4)) to absorb hardware write overflow. These
are never rewritten, so they keep hw_objs.mkey.

That seems to be the reason for moving the UMR mkey into the data direct
PD in the first place. If the hardware requires KSM child mkeys to be in
the same PD as the parent, would an oversized frame that reaches these
overflow entries cause a protection error? That would mean an RQ error
CQE instead of a harmless write to the filler page.

[ ... ]

> @@ -2873,7 +2958,11 @@ static int mlx5e_open_channel(struct mlx5e_priv *priv, int ix,
>  		goto err_free;
>  	}
>  
> -	err = mlx5e_build_channel_param(mdev, params, qcfg, cparam);
> +	data_direct = MLX5E_GET_PFLAG(params, MLX5E_PFLAG_DATA_DIRECT) &&
> +		      netif_rxq_has_unreadable_mp(netdev, ix);
> +
> +	err = mlx5e_build_channel_param(mdev, params, qcfg, data_direct,
> +					cparam);

[Severity: Medium]
data_direct is decided here from the netdev-wide pflag and
netif_rxq_has_unreadable_mp(). However, mdev comes from
mlx5_sd_ch_ix_get_dev(priv->mdev, ix), which can be a Socket-Direct
secondary PF. Can mdev->data_direct be NULL for this channel?

The RQ allocation path dereferences it without a check:

mlx5e_alloc_rq()
  mlx5e_create_rq_umr_mkey()
    pdn = dd ? mdev->data_direct->pdn : ...
  mlx5e_rq_alloc_data_direct_mkey()
    mdev->data_direct->mkey_ro_valid ? ...

mlx5_data_direct_init() leaves mdev->data_direct NULL when
mlx5_data_direct_supported() is false. set_pflag_data_direct(), added
later in the series, only checks priv->mdev.

mlx5e_queue_get_dma_dev() already handles a NULL ch->mdev->data_direct
through mlx5_data_direct_get_dev(), so that case seems to be expected.

Suppose the primary PF supports data direct and a secondary PF does not.
Would binding devmem to a queue owned by the secondary oops in
mlx5e_create_rq_umr_mkey()?

This can't be reached in this commit. It becomes reachable with the
commit "net/mlx5e: Enable the data direct netdev feature".

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