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

[Dragos Tatulea <dtatulea@nvidia.com>]

On 12.11.26 10:40, netdev-bot+sashiko@kernel.org wrote:
> 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.
>
This will be improved in v3.

> [...]
> 
> [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.
> 
log_max_klm_list_size is not <= 15

> [ ... ]
> 
>> 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.
>
Devices that support data-direct also support fixed_buffer_size.

> 
> [ ... ]
> 
>> @@ -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:
> 
That's how crossed mkeys work for data-direct.

> 
> [ ... ]
> 
>> @@ -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.
>
This is the design for all ethernet string RQs. Not an issue.

Thanks,
Dragos