Re: [dpdk-dev] [PATCH v5 1/6] ethdev: fix max Rx packet length

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/8/2021 4:57 PM, Ananyev, Konstantin wrote:
> 
> 
>> There is a confusion on setting max Rx packet length, this patch aims to
>> clarify it.
>>
>> 'rte_eth_dev_configure()' API accepts max Rx packet size via
>> 'uint32_t max_rx_pkt_len' field of the config struct 'struct
>> rte_eth_conf'.
>>
>> Also 'rte_eth_dev_set_mtu()' API can be used to set the MTU, and result
>> stored into '(struct rte_eth_dev)->data->mtu'.
>>
>> These two APIs are related but they work in a disconnected way, they
>> store the set values in different variables which makes hard to figure
>> out which one to use, also having two different method for a related
>> functionality is confusing for the users.
>>
>> Other issues causing confusion is:
>> * maximum transmission unit (MTU) is payload of the Ethernet frame. And
>>    'max_rx_pkt_len' is the size of the Ethernet frame. Difference is
>>    Ethernet frame overhead, and this overhead may be different from
>>    device to device based on what device supports, like VLAN and QinQ.
>> * 'max_rx_pkt_len' is only valid when application requested jumbo frame,
>>    which adds additional confusion and some APIs and PMDs already
>>    discards this documented behavior.
>> * For the jumbo frame enabled case, 'max_rx_pkt_len' is an mandatory
>>    field, this adds configuration complexity for application.
>>
>> As solution, both APIs gets MTU as parameter, and both saves the result
>> in same variable '(struct rte_eth_dev)->data->mtu'. For this
>> 'max_rx_pkt_len' updated as 'mtu', and it is always valid independent
>> from jumbo frame.
>>
>> For 'rte_eth_dev_configure()', 'dev->data->dev_conf.rxmode.mtu' is user
>> request and it should be used only within configure function and result
>> should be stored to '(struct rte_eth_dev)->data->mtu'. After that point
>> both application and PMD uses MTU from this variable.
>>
>> When application doesn't provide an MTU during 'rte_eth_dev_configure()'
>> default 'RTE_ETHER_MTU' value is used.
>>
>> Additional clarification done on scattered Rx configuration, in
>> relation to MTU and Rx buffer size.
>> MTU is used to configure the device for physical Rx/Tx size limitation,
>> Rx buffer is where to store Rx packets, many PMDs use mbuf data buffer
>> size as Rx buffer size.
>> PMDs compare MTU against Rx buffer size to decide enabling scattered Rx
>> or not. If scattered Rx is not supported by device, MTU bigger than Rx
>> buffer size should fail.
> 
> LGTM in general, one question below.
> 
> ...
> 
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index daf5ca924221..4d0584af52e3 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -1324,6 +1324,19 @@ eth_dev_validate_offloads(uint16_t port_id, uint64_t req_offloads,
>>   	return ret;
>>   }
>>
>> +static uint16_t
>> +eth_dev_get_overhead_len(uint32_t max_rx_pktlen, uint16_t max_mtu)
>> +{
>> +	uint16_t overhead_len;
>> +
>> +	if (max_mtu != UINT16_MAX && max_rx_pktlen > max_mtu)
>> +		overhead_len = max_rx_pktlen - max_mtu;
> 
> In theory it could be overflow here, though I do realize that in practise it is unlikely situation.
> Anyway why uint16_t, why not uint32_t for all variables here?
> Just no to worry about such things.
> 

That is related to the practically expected values, it should work
fine to use 'uint32_t', so I will switch to it in next version.