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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 11/5/19 5:18 PM, Dekel Peled wrote:
> Thanks, PSB.
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>> Sent: Tuesday, November 5, 2019 2:40 PM
>> To: Dekel Peled <dekelp@mellanox.com>; john.mcnamara@intel.com;
>> marko.kovacevic@intel.com; nhorman@tuxdriver.com;
>> ajit.khaparde@broadcom.com; somnath.kotur@broadcom.com;
>> anatoly.burakov@intel.com; xuanziyang2@huawei.com;
>> cloud.wangxiaoyun@huawei.com; zhouguoyang@huawei.com;
>> wenzhuo.lu@intel.com; konstantin.ananyev@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;
>> tiwei.bie@intel.com; zhihong.wang@intel.com; yongwang@vmware.com;
>> Thomas Monjalon <thomas@monjalon.net>; ferruh.yigit@intel.com;
>> jingjing.wu@intel.com; bernard.iremonger@intel.com
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH 1/3] ethdev: support API to set max LRO packet size
>>
>> 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]
>>>
>> 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%7C751aa0cb18b94b8a447c08d761ed4051%7Ca652971c7d2e4d9ba6a4d149
>> 256f461
>>>
>> b%7C0%7C1%7C637085543948425032&amp;sdata=C2laHnaMCQZbDUneQD0
>> 2Kpi5iAcr%
>>> 2FYDAS%2BMuO8IcV9s%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%7C751aa0cb18b94b8a447c08d761ed4051%7Ca652971c7d2e4d9ba6a4d149
>> 256f461
>>>
>> b%7C0%7C1%7C637085543948435028&amp;sdata=XnexdrRYNmFyLqT9IL6ZKa
>> CLF2JKr
>>> oKPDVML7gXKceE%3D&amp;reserved=0
>>>
>>> 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.
> 
> Will remove prefix.
> 
>>
>>> +{
>>> +	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)
>>
> 
> This check is for value smaller than minimum, not just 0.

Yes, I know, but the question still remains.

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

[snip]