Re: [dpdk-dev] [PATCH v2 01/22] ethdev: fix MTU size exceeds max rx packet length

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

On 12/28/2020 2:51 PM, Andrew Rybchenko wrote:
> On 12/17/20 12:22 PM, Steve Yang wrote:
>> If max rx packet length is smaller then MTU + Ether overhead, that will
>> drop all MTU size packets.
>>
>> Update the MTU size according to the max rx packet and Ether overhead.
>>
>> Fixes: 59d0ecdbf0e1 ("ethdev: MTU accessors")
>>
>> Signed-off-by: Steve Yang <stevex.yang@intel.com>
>> ---
>>   lib/librte_ethdev/rte_ethdev.c | 21 ++++++++++++++++++---
>>   1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 17ddacc78d..ff6a1e675f 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -1292,6 +1292,7 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>   	struct rte_eth_dev *dev;
>>   	struct rte_eth_dev_info dev_info;
>>   	struct rte_eth_conf orig_conf;
>> +	uint16_t overhead_len;
>>   	int diag;
>>   	int ret;
>>   
>> @@ -1323,6 +1324,15 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>   	if (ret != 0)
>>   		goto rollback;
>>   
>> +	/* Get the real Ethernet overhead length */
>> +	if (dev_info.max_mtu &&
> 
> First of all I'm not sure that we need to handle 0 value
> separately. Or it should be checked separately and trigger
> an error since it is a driver mis-behaviour.
> 

Agree. Most probably we can drop it, "dev_info.max_mtu != UINT16_MAX" covers the 
case driver doesn't provide any value.

> If kept, it should be compared vs 0 explicitly in accordance
> with DPDK coding style.
> 
>> +	    dev_info.max_mtu != UINT16_MAX &&
>> +	    dev_info.max_rx_pktlen &&
> 
> It should be compared vs 0 explicitly in accordance
> with DPDK coding style.
> 
>> +	    dev_info.max_rx_pktlen > dev_info.max_mtu)
>> +		overhead_len = dev_info.max_rx_pktlen - dev_info.max_mtu;
>> +	else
>> +		overhead_len = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
>> +
>>   	/* If number of queues specified by application for both Rx and Tx is
>>   	 * zero, use driver preferred values. This cannot be done individually
>>   	 * as it is valid for either Tx or Rx (but not both) to be zero.
>> @@ -1410,13 +1420,18 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>   			goto rollback;
>>   		}
>>   	} else {
>> -		if (dev_conf->rxmode.max_rx_pkt_len < RTE_ETHER_MIN_LEN ||
>> -			dev_conf->rxmode.max_rx_pkt_len > RTE_ETHER_MAX_LEN)
>> +		uint16_t pktlen = dev_conf->rxmode.max_rx_pkt_len;
>> +		if (pktlen < RTE_ETHER_MIN_MTU + overhead_len ||
>> +			pktlen > RTE_ETHER_MTU + overhead_len)
> 
> Alignment looks misleading. Either two tabs or just 4 spaces.
> 
>>   			/* Use default value */
>>   			dev->data->dev_conf.rxmode.max_rx_pkt_len =
>> -							RTE_ETHER_MAX_LEN;
>> +						RTE_ETHER_MTU + overhead_len;
>>   	}
>>   
>> +	/* Scale the MTU size to adapt max_rx_pkt_len */
>> +	dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
>> +				overhead_len;
>> +
> 
> Is it expected side effect that re-configure always resets
> previously set MTU. I.e.:
>     configure -> set_mtu -> start -> stop -> re-configure
> and set MTU is lost.
> 

This is the problem of two APIs updating same/related values, when device 
re-configure with a given 'max_rx_pkt_len', can we know if the intentions is 
update to the value to new provided 'max_rx_pkt_len' or not?

For this case if user want to keep the MTU value, can read the MTU from device 
first and set 'max_rx_pkt_len' according it.

And we can reduce to updating the MTU in the configure() only when JUMBO frame 
offload is requested, that should be when the 'max_rx_pkt_len' is valid only.

>>   	/*
>>   	 * If LRO is enabled, check that the maximum aggregated packet
>>   	 * size is supported by the configured device.
>>
>