Re: [PATCH net-next v3 7/9] net/mlx5e: lift the netmem TX frag size limit

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch sets `netdev->netmem_tx_max_frag_size = U32_MAX` in
  `mlx5e_build_nic_netdev()`…

--

net/mlx5e: lift the netmem TX frag size limit

This commit sets netmem_tx_max_frag_size to U32_MAX for mlx5e NIC netdevs.
The commit message says mlx5 TX data segments have a 32-bit byte count,
so devmem TX frags don't need a size limit.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index c3c2c5720c6f3..28ba04bd5f4a4 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -5967,6 +5967,7 @@ static void mlx5e_build_nic_netdev(struct net_device *netdev)
>  	netdev->priv_flags       |= IFF_UNICAST_FLT;
>
>  	netdev->netmem_tx = NETMEM_TX_DMA;
> +	netdev->netmem_tx_max_frag_size = U32_MAX;

[Severity: Low]
Is U32_MAX really the largest length an mlx5 data segment can describe?
The commit message says:

  TX data segments have a 32-bit byte count, so devmem TX frags don't
  need a size limit.

Bit 31 of byte_count looks like it is the inline segment marker rather
than part of the length:

include/linux/mlx5/device.h:
	MLX5_INLINE_SEG = 0x80000000,

The mlx5 IB ODP WQE parser reads it the same way:

drivers/infiniband/hw/mlx5/odp.c:
	inline_segment = !!(byte_count &  MLX5_INLINE_SEG);
	bcnt	       = byte_count & ~MLX5_INLINE_SEG;

The mlx5e XDP, IPsec and PSP TX paths also OR this bit in to mark inline
segments. That leaves 31 bits for the length of a pointer segment.

mlx5e_txwqe_build_dsegs() stores the frag size in a signed int and writes
it to byte_count without masking:

drivers/net/ethernet/mellanox/mlx5/core/en_tx.c:mlx5e_txwqe_build_dsegs() {
    ...
		int fsz = skb_frag_size(frag);
    ...
		dseg->byte_count = cpu_to_be32(fsz);
    ...
}

If a frag of 2^31 bytes or more ever got here, fsz would be negative.
The pointer segment would also have the inline bit set, so the WQE would
be malformed.

Today nothing can trigger this. net_devmem_bind_dmabuf() is called with
niov_shift=0 for TX, so the net_iov size is at most SZ_2M. Each frag in
zerocopy_fill_skb_from_devmem() is limited by that size.

The documented contract in Documentation/networking/netmem.rst still
says:

   Drivers which can describe larger TX buffers
   may raise it, up to the largest buffer a single TX descriptor can
   describe.

zerocopy_fill_skb_from_devmem() also relies on this value as the
per-device bound:

	size = min_t(size_t, size, binding->tx_max_frag_size);

Would a value that matches the hardware, such as (1U << 31) - 1 or SZ_1G,
work better here? The commit message would need updating to match.

>
>  	netif_set_tso_max_size(netdev, GSO_MAX_SIZE);
>  	mlx5e_set_xdp_feature(priv);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105222058.944293-1-sdf%40fomichev.me