Re: [dpdk-dev] [RFC] net/ionic: update MTU calculations

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

On 12/15/2020 12:26 PM, Ferruh Yigit wrote:
> On 12/10/2020 2:46 AM, Andrew Boyer wrote:
>> This RFC is in response to the threads about testpmd mtu settings
>> and the deprecation of max-rx-pkt-len.
>>
>> It took us a while to figure out what we were supposed to be doing
>> in this part of the API. It is not clear if max_rx_pkt_len should be
>> an input to or an output from the PMD.
> 
> 'max_rx_pkt_len' is input to the PMD, but it needs to be in sync with MTU value, 
> that is why PMDs update this value when MTU is updated to keep the sync.
> 
>>
>> The code below represents what we believe should happen today, and also
>> happens to pass the DTS tests which were failing prior to this change
>> (checksum and jumbo_frame at least).
>>

Hi Andrew,

I am updating the status of the patch as "change requested", what is the status 
of this patch, will there be a new version?
Is DTS still failing?

Please see a few comments below if there will be new version.

<...>

>> diff --git a/drivers/net/ionic/ionic_ethdev.c b/drivers/net/ionic/ionic_ethdev.c
>> index 925da3e29..7000de3f9 100644
>> --- a/drivers/net/ionic/ionic_ethdev.c
>> +++ b/drivers/net/ionic/ionic_ethdev.c
>> @@ -357,25 +357,19 @@ ionic_dev_mtu_set(struct rte_eth_dev *eth_dev, uint16_t 
>> mtu)
>>       int err;
>>       IONIC_PRINT_CALL();
>> +    IONIC_PRINT(INFO, "Setting mtu %u\n", mtu);
>> -    /*
>> -     * Note: mtu check against IONIC_MIN_MTU, IONIC_MAX_MTU
>> -     * is done by the the API.
>> -     */
>> -
>> -    /*
>> -     * Max frame size is MTU + Ethernet header + VLAN + QinQ
>> -     * (plus ETHER_CRC_LEN if the adapter is able to keep CRC)
>> -     */
>> -    max_frame_size = mtu + RTE_ETHER_HDR_LEN + 4 + 4;
>> -
>> -    if (eth_dev->data->dev_conf.rxmode.max_rx_pkt_len < max_frame_size)
>> -        return -EINVAL;
> 
> The max frame size calculation depends on what HW support, but if VLAN header is 
> supported above calculation and check is correct.
> 

Removing above check seems correct thing to do, instead should check against 
'dev_info.max_mtu' which is already done in ethdev layer, so nothing needed here.

>> -
>> +    /* Note: mtu check against min/max is done by the API */
>>       err = ionic_lif_change_mtu(lif, mtu);
>>       if (err)
>>           return err;
>> +    /* Update max frame size */
>> +    max_frame_size = mtu + RTE_ETHER_HDR_LEN;
> 
> I guess you are removing the CRC because your HW strips the CRC in the Rx 
> buffer, but this limit is not for Rx buffer, it is for the frame size HW 
> accepts, and since recevied frame will have the CRC it should be included into 
> the calculation.
> 
>> +    eth_dev->data->dev_conf.rxmode.max_rx_pkt_len = max_frame_size;
>> +

Although 'rxmode.max_rx_pkt_len' is input to driver, it is related with MTU, 
which is also input from application in this function, so OK to update 
'rxmode.max_rx_pkt_len' here.

>> +    ionic_lif_set_rx_buf_size(lif);
>> +

This seems to keep the local copy of 'rxmode.max_rx_pkt_len' and use local copy 
instead, is this just refactoring or is there any other reason for local copy?