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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 11/5/19 11:40 AM, Dekel Peled wrote:
> 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] http://patches.dpdk.org/patch/58217/
> [2] http://patches.dpdk.org/patch/57492/
>
> Signed-off-by: Dekel Peled <dekelp@mellanox.com>

Few comments below, otherwise

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

[snip]

> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 85ab5f0..2f52090 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1156,6 +1156,26 @@ struct rte_eth_dev *
>  	return name;
>  }
>  
> +static inline int
> +rte_eth_check_lro_pkt_size(uint16_t port_id, uint32_t config_size,
> +			   uint32_t dev_info_size)

As I understand Thomas prefers static functions without rte_eth_ prefix.
I think it is reasonable.

> +{
> +	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) {

Shouldn't config_size == 0 fallback to maximum?
(I don't know and I simply would like to get comments on it)

> +		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)
> @@ -1286,6 +1306,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 = rte_eth_check_lro_pkt_size(
> +				port_id, dev_conf->rxmode.max_lro_pkt_size,
> +				dev_info.max_lro_pkt_size);
> +		if (ret)

if (ret != 0)
https://doc.dpdk.org/guides/contributing/coding_style.html#function-calls
and the style dominates in rte_ethdev.c

> +			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) {
> @@ -1790,6 +1822,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 = rte_eth_check_lro_pkt_size(port_id,
> +				dev->data->dev_conf.rxmode.max_lro_pkt_size,
> +				dev_info.max_lro_pkt_size);
> +		if (ret)

if (ret != 0)
https://doc.dpdk.org/guides/contributing/coding_style.html#function-calls
and the style dominates in rte_ethdev.c

> +			return ret;
> +	}
> +
>  	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
>  					      socket_id, &local_conf, mp);
>  	if (!ret) {
>

[snip]