Re: [PATCH v5 1/3] ethdev: introduce maximum Rx buffer size

[Ivan Malov <ivan.malov@arknetworks.am>]

[-- Attachment #1: Type: text/plain, Size: 4858 bytes --]

Hi,

(minor notes below)

On Fri, 3 Nov 2023, Huisong Li wrote:

> The "min_rx_bufsize" in struct rte_eth_dev_info stands for the minimum
> Rx buffer size supported by hardware. Actually, some engines also have
> the maximum Rx buffer specification, like, hns3, i40e and so on. If mbuf
> data room size in mempool is greater then the maximum Rx buffer size
> per descriptor supported by HW, the data size application used in each
> mbuf is just as much as the maximum Rx buffer size instead of the whole
> data room size.
>
> So introduce maximum Rx buffer size which is not enforced just to
> report user to avoid memory waste. In addition, fix the comment for
> the "min_rx_bufsize" to make it be more specific.

Consider:
So introduce maximum Rx buffer size. It does not enforce anything;
its sole purpose is to inform the user of possible memory waste.

>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> doc/guides/rel_notes/release_23_11.rst |  7 +++++++
> lib/ethdev/rte_ethdev.c                |  8 ++++++++
> lib/ethdev/rte_ethdev.h                | 10 +++++++++-
> 3 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/doc/guides/rel_notes/release_23_11.rst b/doc/guides/rel_notes/release_23_11.rst
> index 95db98d098..d4f7d5b266 100644
> --- a/doc/guides/rel_notes/release_23_11.rst
> +++ b/doc/guides/rel_notes/release_23_11.rst
> @@ -122,6 +122,13 @@ New Features
>   a group's miss actions, which are the actions to be performed on packets
>   that didn't match any of the flow rules in the group.
>
> +* **Added maximum Rx buffer size to report.**

Consider: Added maximum Rx buffer size reporting.

> +
> +  Introduced the ``max_rx_bufsize`` field representing the maximum Rx
> +  buffer size per descriptor supported by HW in structure ``rte_eth_dev_info``
> +  to report user and to avoid wasting space of mempool.
> +  Its value is UINT32_MAX if driver doesn't report it.
> +
> * **Updated Amazon ena (Elastic Network Adapter) net driver.**
>
>   * Upgraded ENA HAL to latest version.
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index af23ac0ad0..24b708f772 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -2112,6 +2112,7 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> 	struct rte_eth_dev *dev;
> 	struct rte_eth_dev_info dev_info;
> 	struct rte_eth_rxconf local_conf;
> +	uint32_t buf_data_size;
>
> 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> 	dev = &rte_eth_devices[port_id];
> @@ -2158,6 +2159,12 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> 			return ret;
>
> 		mbp_buf_size = rte_pktmbuf_data_room_size(mp);
> +		buf_data_size = mbp_buf_size - RTE_PKTMBUF_HEADROOM;
> +		if (buf_data_size > dev_info.max_rx_bufsize)
> +			RTE_ETHDEV_LOG(DEBUG,
> +				"For port_id=%u, the mbuf data buffer size (%u) is bigger than "

Consider: is larger than

> +				"max buffer size (%u) device can utilize, so mbuf size can be reduced.\n",
> +				port_id, buf_data_size, dev_info.max_rx_bufsize);
> 	} else if (rx_conf != NULL && rx_conf->rx_nseg > 0) {
> 		const struct rte_eth_rxseg_split *rx_seg;
> 		uint16_t n_seg;
> @@ -3757,6 +3764,7 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
> 	dev_info->min_mtu = RTE_ETHER_MIN_LEN - RTE_ETHER_HDR_LEN -
> 		RTE_ETHER_CRC_LEN;
> 	dev_info->max_mtu = UINT16_MAX;
> +	dev_info->max_rx_bufsize = UINT32_MAX;
>
> 	if (*dev->dev_ops->dev_infos_get == NULL)
> 		return -ENOTSUP;
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index a53dd5a1ef..7133b73d26 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -1723,7 +1723,15 @@ struct rte_eth_dev_info {
> 	uint16_t min_mtu;	/**< Minimum MTU allowed */
> 	uint16_t max_mtu;	/**< Maximum MTU allowed */
> 	const uint32_t *dev_flags; /**< Device flags */
> -	uint32_t min_rx_bufsize; /**< Minimum size of Rx buffer. */
> +	/** Minimum Rx buffer size per descriptor supported by HW. */
> +	uint32_t min_rx_bufsize;
> +	/**
> +	 * Maximum Rx buffer size per descriptor supported by HW.
> +	 * The value is not enforced, information only to application to
> +	 * optimize mbuf size. Its value is UINT32_MAX when not specified
> +	 * by the driver.

Consider: The value is not enforced. It's a hint for the application
to optimise mbuf size on mempool allocation. If the driver does not
restrict the maximum buffer size, reported value will be UINT32_MAX.

> +	 */
> +	uint32_t max_rx_bufsize;
> 	uint32_t max_rx_pktlen; /**< Maximum configurable length of Rx pkt. */
> 	/** Maximum configurable size of LRO aggregated packet. */
> 	uint32_t max_lro_pkt_size;
> -- 
> 2.33.0
>
>
Other than that,

Acked-by: Ivan Malov <ivan.malov@arknetworks.am>

Thank you.