Re: [RFC] ethdev: introduce maximum Rx buffer size

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 8/8/23 07:02, Huisong Li wrote:
> The Rx buffer size stands for the size hardware supported to receive
> packets in one mbuf. The "min_rx_bufsize" is the minimum buffer hardware
> supported in Rx. Actually, some engines also have the maximum buffer
> specification, like, hns3. For these engines, the available data size
> of one mbuf in Rx also depends on the maximum buffer hardware supported.
> So introduce maximum Rx buffer size in struct rte_eth_dev_info to report
> user to avoid memory waste.

I think that the field should be defined as for informational purposes
only (highlighted in comments). I.e. if application specifies larger Rx
buffer, driver should accept it and just pass smaller value value to HW.
Also I think it would be useful to log warning from Rx queue setup
if provided Rx buffer is larger than maximum reported by the driver.

> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>   lib/ethdev/rte_ethdev.c | 1 +
>   lib/ethdev/rte_ethdev.h | 4 ++--
>   2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 0840d2b594..6d1b92e607 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -3689,6 +3689,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 04a2564f22..1f0ab9c5d8 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -1779,8 +1779,8 @@ struct rte_eth_dev_info {
>   	struct rte_eth_switch_info switch_info;
>   	/** Supported error handling mode. */
>   	enum rte_eth_err_handle_mode err_handle_mode;
> -
> -	uint64_t reserved_64s[2]; /**< Reserved for future fields */
> +	uint32_t max_rx_bufsize; /**< Maximum size of Rx buffer. */

IMHO, comment should be aligned similar to comments below.
Since the next release is ABI breaking, I think it should be put
nearby min_rx_bufsize to make it easier to notice it.

> +	uint32_t reserved_32s[3]; /**< Reserved for future fields */
>   	void *reserved_ptrs[2];   /**< Reserved for future fields */
>   };
>