Re: [dpdk-dev] [PATCH v6] ethdev: add max LRO packet size

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> 
> From: Dekel Peled <dekelp@mellanox.com>
> 
> The maximum supported aggregated packet size for LRO
> is advertised in rte_eth_dev_info.
> For some devices, max_lro_pktlen may be different of
> the basic max_rx_pktlen property.
> 
> Various PMDs supporting LRO are updated.
> 
> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> 
> v6: This is half of v5 1/3. Only the agreed part is here.
> Hope it represents the consensus, so we make a step forward.
> The field max_lro_pkt_size is renamed to max_lro_pktlen
> in order to look like max_rx_pktlen.
> 
> ---
>  doc/guides/nics/features.rst           | 1 +
>  doc/guides/rel_notes/deprecation.rst   | 4 ----
>  doc/guides/rel_notes/release_19_11.rst | 3 +++
>  drivers/net/bnxt/bnxt_ethdev.c         | 1 +
>  drivers/net/hinic/hinic_pmd_ethdev.c   | 1 +
>  drivers/net/ixgbe/ixgbe_ethdev.c       | 1 +
>  drivers/net/mlx5/mlx5.h                | 3 +++
>  drivers/net/mlx5/mlx5_ethdev.c         | 1 +
>  drivers/net/mlx5/mlx5_rxq.c            | 1 -
>  drivers/net/qede/qede_ethdev.c         | 1 +
>  drivers/net/virtio/virtio_ethdev.c     | 1 +
>  drivers/net/vmxnet3/vmxnet3_ethdev.c   | 1 +
>  lib/librte_ethdev/rte_ethdev.h         | 1 +
>  13 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> index d96696801a..1b2e120a9d 100644
> --- a/doc/guides/nics/features.rst
> +++ b/doc/guides/nics/features.rst
> @@ -197,6 +197,7 @@ Supports Large Receive Offload.
>  * **[implements] rte_eth_dev_data**: ``lro``.
>  * **[provides]   mbuf**: ``mbuf.ol_flags:PKT_RX_LRO``, ``mbuf.tso_segsz``.
>  * **[provides]   rte_eth_dev_info**: ``rx_offload_capa,rx_queue_offload_capa:DEV_RX_OFFLOAD_TCP_LRO``.
> +* **[provides]   rte_eth_dev_info**: ``max_lro_pktlen``.
> 
> 
>  .. _nic_features_tso:
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index b0b992dcb5..d4fcf9975b 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -88,10 +88,6 @@ Deprecation Notices
>    This scheme will allow PMDs to avoid lookup to internal ptype table on Rx and
>    thereby improve Rx performance if application wishes do so.
> 
> -* ethdev: New 32-bit fields may be added for maximum LRO session size, in
> -  struct ``rte_eth_dev_info`` for the port capability and in struct
> -  ``rte_eth_rxmode`` for the port configuration.
> -
>  * cryptodev: support for using IV with all sizes is added, J0 still can
>    be used but only when IV length in following structs ``rte_crypto_auth_xform``,
>    ``rte_crypto_aead_xform`` is set to zero. When IV length is greater or equal
> diff --git a/doc/guides/rel_notes/release_19_11.rst b/doc/guides/rel_notes/release_19_11.rst
> index 795c7601c0..473af44374 100644
> --- a/doc/guides/rel_notes/release_19_11.rst
> +++ b/doc/guides/rel_notes/release_19_11.rst
> @@ -403,6 +403,9 @@ ABI Changes
>    align the Ethernet header on receive and all known encapsulations
>    preserve the alignment of the header.
> 
> +* ethdev: Added 32-bit field for maximum LRO aggregated packet size,
> +  as port capability in the struct ``rte_eth_dev_info``.
> +
>  * security: The field ``replay_win_sz`` has been moved from ipsec library
>    based ``rte_ipsec_sa_prm`` structure to security library based structure
>    ``rte_security_ipsec_xform``, which specify the Anti replay window size
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index 58a4f98c9f..95c60a3757 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -535,6 +535,7 @@ static int bnxt_dev_info_get_op(struct rte_eth_dev *eth_dev,
>  	/* Fast path specifics */
>  	dev_info->min_rx_bufsize = 1;
>  	dev_info->max_rx_pktlen = BNXT_MAX_PKT_LEN;
> +	dev_info->max_lro_pktlen = BNXT_MAX_PKT_LEN;
> 
>  	dev_info->rx_offload_capa = BNXT_DEV_RX_OFFLOAD_SUPPORT;
>  	if (bp->flags & BNXT_FLAG_PTP_SUPPORTED)
> diff --git a/drivers/net/hinic/hinic_pmd_ethdev.c b/drivers/net/hinic/hinic_pmd_ethdev.c
> index 9f37a404be..cbd2d032f9 100644
> --- a/drivers/net/hinic/hinic_pmd_ethdev.c
> +++ b/drivers/net/hinic/hinic_pmd_ethdev.c
> @@ -727,6 +727,7 @@ hinic_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
>  	info->max_tx_queues  = nic_dev->nic_cap.max_sqs;
>  	info->min_rx_bufsize = HINIC_MIN_RX_BUF_SIZE;
>  	info->max_rx_pktlen  = HINIC_MAX_JUMBO_FRAME_SIZE;
> +	info->max_lro_pktlen = HINIC_MAX_JUMBO_FRAME_SIZE;
>  	info->max_mac_addrs  = HINIC_MAX_UC_MAC_ADDRS;
>  	info->min_mtu = HINIC_MIN_MTU_SIZE;
>  	info->max_mtu = HINIC_MAX_MTU_SIZE;
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index dbce7a80e9..a01b8bbf10 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -3804,6 +3804,7 @@ ixgbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>  	}
>  	dev_info->min_rx_bufsize = 1024; /* cf BSIZEPACKET in SRRCTL register */
>  	dev_info->max_rx_pktlen = 15872; /* includes CRC, cf MAXFRS register */
> +	dev_info->max_lro_pktlen = RTE_IPV4_MAX_PKT_LEN;
>  	dev_info->max_mac_addrs = hw->mac.num_rar_entries;
>  	dev_info->max_hash_mac_addrs = IXGBE_VMDQ_NUM_UC_MAC;
>  	dev_info->max_vfs = pci_dev->max_vfs;

Actually after looking at ixgbe code once again -
as we set LRO capability conditionally, we probably better to set max_lro_pktlen
conditionally too. Something like:

--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -3820,6 +3820,9 @@ ixgbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
        dev_info->tx_queue_offload_capa = ixgbe_get_tx_queue_offloads(dev);
        dev_info->tx_offload_capa = ixgbe_get_tx_port_offloads(dev);

+       if (dev_info->rx_offload_capa & DEV_RX_OFFLOAD_TCP_LRO)
+               dev_info->max_lro_pktlen = RTE_IPV4_MAX_PKT_LEN;
+
        dev_info->default_rxconf = (struct rte_eth_rxconf) {
                .rx_thresh = {
                        .pthresh = IXGBE_DEFAULT_RX_PTHRESH,

Sorry for missed that previously.
Apart from that: LGTM.

> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index b6a51b2b4d..935adbbbf3 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -198,6 +198,9 @@ TAILQ_HEAD(mlx5_flows, rte_flow);
>  #define MLX5_LRO_SUPPORTED(dev) \
>  	(((struct mlx5_priv *)((dev)->data->dev_private))->config.lro.supported)
> 
> +/* Maximal size of aggregated LRO packet. */
> +#define MLX5_MAX_LRO_SIZE (UINT8_MAX * 256u)
> +
>  /* LRO configurations structure. */
>  struct mlx5_lro_config {
>  	uint32_t supported:1; /* Whether LRO is supported. */
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 2278b24c01..91de186365 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -552,6 +552,7 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
>  	/* FIXME: we should ask the device for these values. */
>  	info->min_rx_bufsize = 32;
>  	info->max_rx_pktlen = 65536;
> +	info->max_lro_pktlen = MLX5_MAX_LRO_SIZE;
>  	/*
>  	 * Since we need one CQ per QP, the limit is the minimum number
>  	 * between the two values.
> diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
> index f0ab8438d3..aca2e67e0c 100644
> --- a/drivers/net/mlx5/mlx5_rxq.c
> +++ b/drivers/net/mlx5/mlx5_rxq.c
> @@ -1524,7 +1524,6 @@ mlx5_mprq_alloc_mp(struct rte_eth_dev *dev)
>  	return 0;
>  }
> 
> -#define MLX5_MAX_LRO_SIZE (UINT8_MAX * 256u)
>  #define MLX5_MAX_TCP_HDR_OFFSET ((unsigned int)(sizeof(struct rte_ether_hdr) + \
>  					sizeof(struct rte_vlan_hdr) * 2 + \
>  					sizeof(struct rte_ipv6_hdr)))
> diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
> index 53fdfde9a8..fd05856836 100644
> --- a/drivers/net/qede/qede_ethdev.c
> +++ b/drivers/net/qede/qede_ethdev.c
> @@ -1273,6 +1273,7 @@ qede_dev_info_get(struct rte_eth_dev *eth_dev,
> 
>  	dev_info->min_rx_bufsize = (uint32_t)QEDE_MIN_RX_BUFF_SIZE;
>  	dev_info->max_rx_pktlen = (uint32_t)ETH_TX_MAX_NON_LSO_PKT_LEN;
> +	dev_info->max_lro_pktlen = (uint32_t)0x7FFF;
>  	dev_info->rx_desc_lim = qede_rx_desc_lim;
>  	dev_info->tx_desc_lim = qede_tx_desc_lim;
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 646de9945c..d97f3c6645 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -2435,6 +2435,7 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>  		RTE_MIN(hw->max_queue_pairs, VIRTIO_MAX_TX_QUEUES);
>  	dev_info->min_rx_bufsize = VIRTIO_MIN_RX_BUFSIZE;
>  	dev_info->max_rx_pktlen = VIRTIO_MAX_RX_PKTLEN;
> +	dev_info->max_lro_pktlen = VIRTIO_MAX_RX_PKTLEN;
>  	dev_info->max_mac_addrs = VIRTIO_MAX_MAC_ADDRS;
> 
>  	host_features = VTPCI_OPS(hw)->get_features(hw);
> diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> index d1faeaa81b..6c99a2a8e0 100644
> --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> @@ -1161,6 +1161,7 @@ vmxnet3_dev_info_get(struct rte_eth_dev *dev,
>  	dev_info->max_tx_queues = VMXNET3_MAX_TX_QUEUES;
>  	dev_info->min_rx_bufsize = 1518 + RTE_PKTMBUF_HEADROOM;
>  	dev_info->max_rx_pktlen = 16384; /* includes CRC, cf MAXFRS register */
> +	dev_info->max_lro_pktlen = 16384;
>  	dev_info->speed_capa = ETH_LINK_SPEED_10G;
>  	dev_info->max_mac_addrs = VMXNET3_MAX_MAC_ADDRS;
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index c36c1b631f..b47eea60d9 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1183,6 +1183,7 @@ struct rte_eth_dev_info {
>  	const uint32_t *dev_flags; /**< Device flags */
>  	uint32_t min_rx_bufsize; /**< Minimum size of RX buffer. */
>  	uint32_t max_rx_pktlen; /**< Maximum configurable length of RX pkt. */
> +	uint32_t max_lro_pktlen; /**< Maximum size of LRO aggregated packet. */
>  	uint16_t max_rx_queues; /**< Maximum number of RX queues. */
>  	uint16_t max_tx_queues; /**< Maximum number of TX queues. */
>  	uint32_t max_mac_addrs; /**< Maximum number of MAC addresses. */
> --
> 2.23.0