Re: [dpdk-dev] [PATCH v5 1/3] ethdev: support API to set max LRO packet size

[Dekel Peled <dekelp@mellanox.com>]

Thnaks, PSB.

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Monday, November 11, 2019 1:08 AM
> To: Dekel Peled <dekelp@mellanox.com>; Mcnamara, John
> <john.mcnamara@intel.com>; Kovacevic, Marko
> <marko.kovacevic@intel.com>; nhorman@tuxdriver.com;
> ajit.khaparde@broadcom.com; somnath.kotur@broadcom.com; Burakov,
> Anatoly <anatoly.burakov@intel.com>; xuanziyang2@huawei.com;
> cloud.wangxiaoyun@huawei.com; zhouguoyang@huawei.com; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Matan Azrad <matan@mellanox.com>; Shahaf
> Shuler <shahafs@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; rmody@marvell.com;
> shshaikh@marvell.com; maxime.coquelin@redhat.com; Bie, Tiwei
> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>;
> yongwang@vmware.com; Thomas Monjalon <thomas@monjalon.net>; Yigit,
> Ferruh <ferruh.yigit@intel.com>; arybchenko@solarflare.com; Wu, Jingjing
> <jingjing.wu@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v5 1/3] ethdev: support API to set max LRO packet size
> 
> 
> 
> > This patch implements [1], to support API for configuration and
> > validation of max size for LRO aggregated packet.
> > API change notice [2] is removed, and release notes for 19.11 are
> > updated accordingly.
> >
> > Various PMDs using LRO offload are updated, the new data members are
> > initialized to ensure they don't fail validation.
> >
> > [1]
> >
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> >
> es.dpdk.org%2Fpatch%2F58217%2F&amp;data=02%7C01%7Cdekelp%40mell
> anox.co
> >
> m%7Cc12f78c6bd3d48bc223008d76632cd0f%7Ca652971c7d2e4d9ba6a4d1492
> 56f461
> >
> b%7C0%7C0%7C637090240692948792&amp;sdata=pU6LJBYDmlzPFzHc%2FQlF
> UVXpGuv
> > ulTcl%2F29GsXdvBF8%3D&amp;reserved=0
> > [2]
> >
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> >
> es.dpdk.org%2Fpatch%2F57492%2F&amp;data=02%7C01%7Cdekelp%40mell
> anox.co
> >
> m%7Cc12f78c6bd3d48bc223008d76632cd0f%7Ca652971c7d2e4d9ba6a4d1492
> 56f461
> >
> b%7C0%7C0%7C637090240692958790&amp;sdata=VegV5HcYhkabDgcOF29u
> %2FFLq25I
> > %2BEDZTaEn20A2t2Wo%3D&amp;reserved=0
> 
> Actually if the requirement is just to allow user to limit max lro size, then why
> not to add just new function for that:
> 
> int rte_eth_dev_set_max_lro(uint16_t port_id, uint32_t lro); ?
> 
> And make it optional for the drivers to support it.
> That way PMD/devices that allow LRO max size to be configurable, can
> support it others can fail.

Current implementation is consistent with the existing max_rx_pkt_len use, in case LRO is used.
When using jumbo frames the packet len must be specified.
Using LRO the max session size should be specified.

> 
> Konstantin
> 
> >
> > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > Acked-by: Thomas Monjalon <thomas@monjalon.net>
> > Acked-by: Matan Azrad <matan@mellanox.com>
> > ---
> >  doc/guides/nics/features.rst           |  2 ++
> >  doc/guides/rel_notes/deprecation.rst   |  4 ----
> >  doc/guides/rel_notes/release_19_11.rst |  8 +++++++
> >  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.c         | 44
> ++++++++++++++++++++++++++++++++++
> >  lib/librte_ethdev/rte_ethdev.h         |  4 ++++
> >  14 files changed, 68 insertions(+), 5 deletions(-)
> >
> > diff --git a/doc/guides/nics/features.rst
> > b/doc/guides/nics/features.rst index 7a31cf7..2138ce3 100644
> > --- a/doc/guides/nics/features.rst
> > +++ b/doc/guides/nics/features.rst
> > @@ -193,10 +193,12 @@ LRO
> >  Supports Large Receive Offload.
> >
> >  * **[uses]       rte_eth_rxconf,rte_eth_rxmode**:
> ``offloads:DEV_RX_OFFLOAD_TCP_LRO``.
> > +  ``dev_conf.rxmode.max_lro_pkt_size``.
> >  * **[implements] datapath**: ``LRO functionality``.
> >  * **[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_pkt_size``.
> >
> >
> >  .. _nic_features_tso:
> > diff --git a/doc/guides/rel_notes/deprecation.rst
> > b/doc/guides/rel_notes/deprecation.rst
> > index c10dc30..fdec33d 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -87,10 +87,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 87b7bd0..a3fc023 100644
> > --- a/doc/guides/rel_notes/release_19_11.rst
> > +++ b/doc/guides/rel_notes/release_19_11.rst
> > @@ -418,6 +418,14 @@ ABI Changes
> >    align the Ethernet header on receive and all known encapsulations
> >    preserve the alignment of the header.
> >
> > +* ethdev: Added 32-bit fields for maximum LRO aggregated packet size,
> > +in
> > +  struct ``rte_eth_dev_info`` for the port capability and in struct
> > +  ``rte_eth_rxmode`` for the port configuration.
> > +  Application should use the new field in struct ``rte_eth_rxmode``
> > +to configure
> > +  the requested size.
> 
> That part I am not happy with: * application should use*.
> Many apps I suppose are ok with default LRO size selected by PMD/HW.
> Why to force changes in all of them?

Again this is to keep consistent with max_rx_pkt_len use.

> 
> > +  PMD should use the new field in struct ``rte_eth_dev_info`` to
> > + report the  supported port capability.
> > +
> >
> >  Shared Library Versions
> >  -----------------------
> > diff --git a/drivers/net/bnxt/bnxt_ethdev.c
> > b/drivers/net/bnxt/bnxt_ethdev.c index b9b055e..741b897 100644
> > --- a/drivers/net/bnxt/bnxt_ethdev.c
> > +++ b/drivers/net/bnxt/bnxt_ethdev.c
> > @@ -519,6 +519,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_pkt_size = 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 9f37a40..b33b2cf 100644
> > --- a/drivers/net/hinic/hinic_pmd_ethdev.c
> > +++ b/drivers/net/hinic/hinic_pmd_ethdev.c
> > @@ -727,6 +727,7 @@ static void hinic_get_speed_capa(struct
> rte_eth_dev *dev, uint32_t *speed_capa)
> >  	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_pkt_size = 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 30c0379..5719552 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -3814,6 +3814,7 @@ static int
> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> >  	}
> >  	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_pkt_size = 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; diff --git
> > a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> > fab58c9..4783b5c 100644
> > --- a/drivers/net/mlx5/mlx5.h
> > +++ b/drivers/net/mlx5/mlx5.h
> > @@ -206,6 +206,9 @@ struct mlx5_hca_attr {  #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 2b7c867..3adc824 100644
> > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > @@ -606,6 +606,7 @@ struct ethtool_link_settings {
> >  	/* FIXME: we should ask the device for these values. */
> >  	info->min_rx_bufsize = 32;
> >  	info->max_rx_pktlen = 65536;
> > +	info->max_lro_pkt_size = 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 24d0eaa..9423e7b 100644
> > --- a/drivers/net/mlx5/mlx5_rxq.c
> > +++ b/drivers/net/mlx5/mlx5_rxq.c
> > @@ -1701,7 +1701,6 @@ struct mlx5_rxq_obj *
> >  	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 575982f..ccbb8a4 100644
> > --- a/drivers/net/qede/qede_ethdev.c
> > +++ b/drivers/net/qede/qede_ethdev.c
> > @@ -1277,6 +1277,7 @@ static int qede_dev_configure(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_pkt_size = (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 044eb10..22ce5a2 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -2435,6 +2435,7 @@ static void virtio_dev_free_mbufs(struct
> rte_eth_dev *dev)
> >  		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_pkt_size = 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 d1faeaa..d18e8bc 100644
> > --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > @@ -1161,6 +1161,7 @@ static int eth_vmxnet3_pci_remove(struct
> rte_pci_device *pci_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_pkt_size = 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.c
> > b/lib/librte_ethdev/rte_ethdev.c index 652c369..c642ba5 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -1136,6 +1136,26 @@ struct rte_eth_dev *
> >  	return name;
> >  }
> >
> > +static inline int
> > +check_lro_pkt_size(uint16_t port_id, uint32_t config_size,
> > +		   uint32_t dev_info_size)
> > +{
> > +	int ret = 0;
> > +
> > +	if (config_size > dev_info_size) {
> > +		RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%d
> max_lro_pkt_size %u "
> > +			       "> max allowed value %u\n", port_id, config_size,
> > +			       dev_info_size);
> > +		ret = -EINVAL;
> > +	} else if (config_size < RTE_ETHER_MIN_LEN) {
> > +		RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%d
> max_lro_pkt_size %u "
> > +			       "< min allowed value %u\n", port_id, config_size,
> > +			       (unsigned int)RTE_ETHER_MIN_LEN);
> > +		ret = -EINVAL;
> > +	}
> > +	return ret;
> > +}
> > +
> >  int
> >  rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t
> nb_tx_q,
> >  		      const struct rte_eth_conf *dev_conf) @@ -1266,6
> +1286,18 @@
> > struct rte_eth_dev *
> >
> 	RTE_ETHER_MAX_LEN;
> >  	}
> >
> > +	/*
> > +	 * If LRO is enabled, check that the maximum aggregated packet
> > +	 * size is supported by the configured device.
> > +	 */
> > +	if (dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_TCP_LRO) {
> > +		ret = check_lro_pkt_size(
> > +				port_id, dev_conf-
> >rxmode.max_lro_pkt_size,
> > +				dev_info.max_lro_pkt_size);
> > +		if (ret != 0)
> > +			goto rollback;
> > +	}
> > +
> >  	/* Any requested offloading must be within its device capabilities */
> >  	if ((dev_conf->rxmode.offloads & dev_info.rx_offload_capa) !=
> >  	     dev_conf->rxmode.offloads) {
> > @@ -1770,6 +1802,18 @@ struct rte_eth_dev *
> >  		return -EINVAL;
> >  	}
> >
> > +	/*
> > +	 * If LRO is enabled, check that the maximum aggregated packet
> > +	 * size is supported by the configured device.
> > +	 */
> > +	if (local_conf.offloads & DEV_RX_OFFLOAD_TCP_LRO) {
> > +		int ret = check_lro_pkt_size(port_id,
> > +				dev->data-
> >dev_conf.rxmode.max_lro_pkt_size,
> > +				dev_info.max_lro_pkt_size);
> > +		if (ret != 0)
> > +			return ret;
> > +	}
> > +
> >  	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id,
> nb_rx_desc,
> >  					      socket_id, &local_conf, mp);
> >  	if (!ret) {
> > diff --git a/lib/librte_ethdev/rte_ethdev.h
> > b/lib/librte_ethdev/rte_ethdev.h index 44d77b3..1b76df5 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -395,6 +395,8 @@ struct rte_eth_rxmode {
> >  	/** The multi-queue packet distribution mode to be used, e.g. RSS.
> */
> >  	enum rte_eth_rx_mq_mode mq_mode;
> >  	uint32_t max_rx_pkt_len;  /**< Only used if JUMBO_FRAME
> enabled. */
> > +	/** Maximum allowed size of LRO aggregated packet. */
> > +	uint32_t max_lro_pkt_size;
> >  	uint16_t split_hdr_size;  /**< hdr buf size (header_split enabled).*/
> >  	/**
> >  	 * Per-port Rx offloads to be set using DEV_RX_OFFLOAD_* flags.
> > @@ -1218,6 +1220,8 @@ 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.
> > */
> > +	/** Maximum configurable size of LRO aggregated packet. */
> > +	uint32_t max_lro_pkt_size;
> >  	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. */
> > --
> > 1.8.3.1