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 <dek...@mellanox.com>
Few comments below, otherwise
Reviewed-by: Andrew Rybchenko <arybche...@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]
