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

[Andrew Boyer <aboyer@pensando.io>]

> On Apr 23, 2021, at 7:42 AM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> 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.
> 

Thank you for your thorough review!

I am working on a new feature at present so upstreaming has been delayed.

We were blocked from posting the next set of our patches by some arm64 build stuff, but thanks to Juraj & co. I think that is cleared up. It’s just a matter of when I will get to posting them.

-Andrew

> <...>
> 
>>> 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?