diff mbox series

[v8,1/2] app/testpmd: fix max rx packet length for VLAN packets

Message ID 20201102085234.72779-2-stevex.yang@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers show
Series fix default max mtu size when device configured | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Steve Yang Nov. 2, 2020, 8:52 a.m. UTC
When the max rx packet length is smaller than the sum of mtu size and
ether overhead size, it should be enlarged, otherwise the VLAN packets
will be dropped.

Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")

Signed-off-by: SteveX Yang <stevex.yang@intel.com>
---
 app/test-pmd/testpmd.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Ferruh Yigit Nov. 2, 2020, 11:48 a.m. UTC | #1
On 11/2/2020 8:52 AM, SteveX Yang wrote:
> When the max rx packet length is smaller than the sum of mtu size and
> ether overhead size, it should be enlarged, otherwise the VLAN packets
> will be dropped.
> 
> Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
> 
> Signed-off-by: SteveX Yang <stevex.yang@intel.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Ferruh Yigit Nov. 3, 2020, 1:29 p.m. UTC | #2
On 11/2/2020 11:48 AM, Ferruh Yigit wrote:
> On 11/2/2020 8:52 AM, SteveX Yang wrote:
>> When the max rx packet length is smaller than the sum of mtu size and
>> ether overhead size, it should be enlarged, otherwise the VLAN packets
>> will be dropped.
>>
>> Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
>>
>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 

Applied to dpdk-next-net/main, thanks.

only 1/2 applied since discussion is going on for 2/2.
Thomas Monjalon Nov. 4, 2020, 4:51 p.m. UTC | #3
03/11/2020 14:29, Ferruh Yigit:
> On 11/2/2020 11:48 AM, Ferruh Yigit wrote:
> > On 11/2/2020 8:52 AM, SteveX Yang wrote:
> >> When the max rx packet length is smaller than the sum of mtu size and
> >> ether overhead size, it should be enlarged, otherwise the VLAN packets
> >> will be dropped.
> >>
> >> Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
> >>
> >> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
> > 
> > Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Applied to dpdk-next-net/main, thanks.
> 
> only 1/2 applied since discussion is going on for 2/2.

I'm not sure this testpmd change is good.

Reminder: testpmd is for testing the PMDs.
Don't we want to see VLAN packets dropped in the case described above?
Ferruh Yigit Nov. 4, 2020, 5:07 p.m. UTC | #4
On 11/4/2020 4:51 PM, Thomas Monjalon wrote:
> 03/11/2020 14:29, Ferruh Yigit:
>> On 11/2/2020 11:48 AM, Ferruh Yigit wrote:
>>> On 11/2/2020 8:52 AM, SteveX Yang wrote:
>>>> When the max rx packet length is smaller than the sum of mtu size and
>>>> ether overhead size, it should be enlarged, otherwise the VLAN packets
>>>> will be dropped.
>>>>
>>>> Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
>>>>
>>>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
>>>
>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>
>> Applied to dpdk-next-net/main, thanks.
>>
>> only 1/2 applied since discussion is going on for 2/2.
> 
> I'm not sure this testpmd change is good.
> 
> Reminder: testpmd is for testing the PMDs.
> Don't we want to see VLAN packets dropped in the case described above?
> 

The patch set 'max_rx_pkt_len' in a way to make MTU 1500 for all PMDs,
otherwise testpmd set hard-coded 'RTE_ETHER_MAX_LEN' value, which makes MTU 
between 1492-1500 depending on PMD.

It is application responsibility to provide correct 'max_rx_pkt_len'.
I guess the original intention was to set MTU as 1500 but was not correct for 
all PMDs and this patch is fixing it.

The same problem in the ethdev, (assuming 'RTE_ETHER_MAX_LEN' will give MTU 
1500), the other patch in the set is to fix it later.
Thomas Monjalon Nov. 4, 2020, 5:55 p.m. UTC | #5
04/11/2020 18:07, Ferruh Yigit:
> On 11/4/2020 4:51 PM, Thomas Monjalon wrote:
> > 03/11/2020 14:29, Ferruh Yigit:
> >> On 11/2/2020 11:48 AM, Ferruh Yigit wrote:
> >>> On 11/2/2020 8:52 AM, SteveX Yang wrote:
> >>>> When the max rx packet length is smaller than the sum of mtu size and
> >>>> ether overhead size, it should be enlarged, otherwise the VLAN packets
> >>>> will be dropped.
> >>>>
> >>>> Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
> >>>>
> >>>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
> >>>
> >>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >>
> >> Applied to dpdk-next-net/main, thanks.
> >>
> >> only 1/2 applied since discussion is going on for 2/2.
> > 
> > I'm not sure this testpmd change is good.
> > 
> > Reminder: testpmd is for testing the PMDs.
> > Don't we want to see VLAN packets dropped in the case described above?
> > 
> 
> The patch set 'max_rx_pkt_len' in a way to make MTU 1500 for all PMDs,
> otherwise testpmd set hard-coded 'RTE_ETHER_MAX_LEN' value, which makes MTU 
> between 1492-1500 depending on PMD.
> 
> It is application responsibility to provide correct 'max_rx_pkt_len'.
> I guess the original intention was to set MTU as 1500 but was not correct for 
> all PMDs and this patch is fixing it.
> 
> The same problem in the ethdev, (assuming 'RTE_ETHER_MAX_LEN' will give MTU 
> 1500), the other patch in the set is to fix it later.

OK but the testpmd patch is just hiding the issue, isn't it?
Ferruh Yigit Nov. 4, 2020, 8:19 p.m. UTC | #6
On 11/4/2020 5:55 PM, Thomas Monjalon wrote:
> 04/11/2020 18:07, Ferruh Yigit:
>> On 11/4/2020 4:51 PM, Thomas Monjalon wrote:
>>> 03/11/2020 14:29, Ferruh Yigit:
>>>> On 11/2/2020 11:48 AM, Ferruh Yigit wrote:
>>>>> On 11/2/2020 8:52 AM, SteveX Yang wrote:
>>>>>> When the max rx packet length is smaller than the sum of mtu size and
>>>>>> ether overhead size, it should be enlarged, otherwise the VLAN packets
>>>>>> will be dropped.
>>>>>>
>>>>>> Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
>>>>>>
>>>>>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
>>>>>
>>>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>
>>>> Applied to dpdk-next-net/main, thanks.
>>>>
>>>> only 1/2 applied since discussion is going on for 2/2.
>>>
>>> I'm not sure this testpmd change is good.
>>>
>>> Reminder: testpmd is for testing the PMDs.
>>> Don't we want to see VLAN packets dropped in the case described above?
>>>
>>
>> The patch set 'max_rx_pkt_len' in a way to make MTU 1500 for all PMDs,
>> otherwise testpmd set hard-coded 'RTE_ETHER_MAX_LEN' value, which makes MTU
>> between 1492-1500 depending on PMD.
>>
>> It is application responsibility to provide correct 'max_rx_pkt_len'.
>> I guess the original intention was to set MTU as 1500 but was not correct for
>> all PMDs and this patch is fixing it.
>>
>> The same problem in the ethdev, (assuming 'RTE_ETHER_MAX_LEN' will give MTU
>> 1500), the other patch in the set is to fix it later.
> 
> OK but the testpmd patch is just hiding the issue, isn't it?
> 

I don't think so, issue was application (testpmd) setting the 'max_rx_pkt_len' 
wrong.

What is hidden?
Thomas Monjalon Nov. 4, 2020, 8:39 p.m. UTC | #7
04/11/2020 21:19, Ferruh Yigit:
> On 11/4/2020 5:55 PM, Thomas Monjalon wrote:
> > 04/11/2020 18:07, Ferruh Yigit:
> >> On 11/4/2020 4:51 PM, Thomas Monjalon wrote:
> >>> 03/11/2020 14:29, Ferruh Yigit:
> >>>> On 11/2/2020 11:48 AM, Ferruh Yigit wrote:
> >>>>> On 11/2/2020 8:52 AM, SteveX Yang wrote:
> >>>>>> When the max rx packet length is smaller than the sum of mtu size and
> >>>>>> ether overhead size, it should be enlarged, otherwise the VLAN packets
> >>>>>> will be dropped.
> >>>>>>
> >>>>>> Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
> >>>>>>
> >>>>>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
> >>>>>
> >>>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>>
> >>>> Applied to dpdk-next-net/main, thanks.
> >>>>
> >>>> only 1/2 applied since discussion is going on for 2/2.
> >>>
> >>> I'm not sure this testpmd change is good.
> >>>
> >>> Reminder: testpmd is for testing the PMDs.
> >>> Don't we want to see VLAN packets dropped in the case described above?
> >>>
> >>
> >> The patch set 'max_rx_pkt_len' in a way to make MTU 1500 for all PMDs,
> >> otherwise testpmd set hard-coded 'RTE_ETHER_MAX_LEN' value, which makes MTU
> >> between 1492-1500 depending on PMD.
> >>
> >> It is application responsibility to provide correct 'max_rx_pkt_len'.
> >> I guess the original intention was to set MTU as 1500 but was not correct for
> >> all PMDs and this patch is fixing it.
> >>
> >> The same problem in the ethdev, (assuming 'RTE_ETHER_MAX_LEN' will give MTU
> >> 1500), the other patch in the set is to fix it later.
> > 
> > OK but the testpmd patch is just hiding the issue, isn't it?
> > 
> 
> I don't think so, issue was application (testpmd) setting the 'max_rx_pkt_len' 
> wrong.
> 
> What is hidden?

I was looking for adding a helper in ethdev API.
But I think I can agree with your way of thinking.
Andrew Rybchenko Nov. 5, 2020, 8:54 a.m. UTC | #8
On 11/4/20 11:39 PM, Thomas Monjalon wrote:
> 04/11/2020 21:19, Ferruh Yigit:
>> On 11/4/2020 5:55 PM, Thomas Monjalon wrote:
>>> 04/11/2020 18:07, Ferruh Yigit:
>>>> On 11/4/2020 4:51 PM, Thomas Monjalon wrote:
>>>>> 03/11/2020 14:29, Ferruh Yigit:
>>>>>> On 11/2/2020 11:48 AM, Ferruh Yigit wrote:
>>>>>>> On 11/2/2020 8:52 AM, SteveX Yang wrote:
>>>>>>>> When the max rx packet length is smaller than the sum of mtu size and
>>>>>>>> ether overhead size, it should be enlarged, otherwise the VLAN packets
>>>>>>>> will be dropped.
>>>>>>>>
>>>>>>>> Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
>>>>>>>>
>>>>>>>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
>>>>>>>
>>>>>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>
>>>>>> Applied to dpdk-next-net/main, thanks.
>>>>>>
>>>>>> only 1/2 applied since discussion is going on for 2/2.
>>>>>
>>>>> I'm not sure this testpmd change is good.
>>>>>
>>>>> Reminder: testpmd is for testing the PMDs.
>>>>> Don't we want to see VLAN packets dropped in the case described above?
>>>>>
>>>>
>>>> The patch set 'max_rx_pkt_len' in a way to make MTU 1500 for all PMDs,
>>>> otherwise testpmd set hard-coded 'RTE_ETHER_MAX_LEN' value, which makes MTU
>>>> between 1492-1500 depending on PMD.
>>>>
>>>> It is application responsibility to provide correct 'max_rx_pkt_len'.
>>>> I guess the original intention was to set MTU as 1500 but was not correct for
>>>> all PMDs and this patch is fixing it.
>>>>
>>>> The same problem in the ethdev, (assuming 'RTE_ETHER_MAX_LEN' will give MTU
>>>> 1500), the other patch in the set is to fix it later.
>>>
>>> OK but the testpmd patch is just hiding the issue, isn't it?
>>>
>>
>> I don't think so, issue was application (testpmd) setting the 'max_rx_pkt_len' 
>> wrong.
>>
>> What is hidden?
> 
> I was looking for adding a helper in ethdev API.
> But I think I can agree with your way of thinking.
> 

The patch breaks running testpmd on Virtio-Net because the
driver populates dev_info.max_rx_pktlen but keeps
dev_info.max_mtu equal to UINT16_MAX as it was filled in by
ethdev. As the result:

Ethdev port_id=0 max_rx_pkt_len 11229 > max valid value 9728
Fail to configure port 0
Ferruh Yigit Nov. 5, 2020, 10:37 a.m. UTC | #9
On 11/5/2020 9:33 AM, Yang, SteveX wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Thursday, November 5, 2020 4:54 PM
>> To: Thomas Monjalon <thomas@monjalon.net>; Yang, SteveX
>> <stevex.yang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
>> Xing, Beilei <beilei.xing@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
>> Iremonger, Bernard <bernard.iremonger@intel.com>; Yang, Qiming
>> <qiming.yang@intel.com>; mdr@ashroe.eu; nhorman@tuxdriver.com;
>> david.marchand@redhat.com
>> Subject: Re: [dpdk-dev] [PATCH v8 1/2] app/testpmd: fix max rx packet
>> length for VLAN packets
>>
>> On 11/4/20 11:39 PM, Thomas Monjalon wrote:
>>> 04/11/2020 21:19, Ferruh Yigit:
>>>> On 11/4/2020 5:55 PM, Thomas Monjalon wrote:
>>>>> 04/11/2020 18:07, Ferruh Yigit:
>>>>>> On 11/4/2020 4:51 PM, Thomas Monjalon wrote:
>>>>>>> 03/11/2020 14:29, Ferruh Yigit:
>>>>>>>> On 11/2/2020 11:48 AM, Ferruh Yigit wrote:
>>>>>>>>> On 11/2/2020 8:52 AM, SteveX Yang wrote:
>>>>>>>>>> When the max rx packet length is smaller than the sum of mtu
>>>>>>>>>> size and ether overhead size, it should be enlarged, otherwise
>>>>>>>>>> the VLAN packets will be dropped.
>>>>>>>>>>
>>>>>>>>>> Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
>>>>>>>>>
>>>>>>>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>>>
>>>>>>>> Applied to dpdk-next-net/main, thanks.
>>>>>>>>
>>>>>>>> only 1/2 applied since discussion is going on for 2/2.
>>>>>>>
>>>>>>> I'm not sure this testpmd change is good.
>>>>>>>
>>>>>>> Reminder: testpmd is for testing the PMDs.
>>>>>>> Don't we want to see VLAN packets dropped in the case described
>> above?
>>>>>>>
>>>>>>
>>>>>> The patch set 'max_rx_pkt_len' in a way to make MTU 1500 for all
>>>>>> PMDs, otherwise testpmd set hard-coded 'RTE_ETHER_MAX_LEN'
>> value,
>>>>>> which makes MTU between 1492-1500 depending on PMD.
>>>>>>
>>>>>> It is application responsibility to provide correct 'max_rx_pkt_len'.
>>>>>> I guess the original intention was to set MTU as 1500 but was not
>>>>>> correct for all PMDs and this patch is fixing it.
>>>>>>
>>>>>> The same problem in the ethdev, (assuming 'RTE_ETHER_MAX_LEN'
>> will
>>>>>> give MTU 1500), the other patch in the set is to fix it later.
>>>>>
>>>>> OK but the testpmd patch is just hiding the issue, isn't it?
>>>>>
>>>>
>>>> I don't think so, issue was application (testpmd) setting the
>> 'max_rx_pkt_len'
>>>> wrong.
>>>>
>>>> What is hidden?
>>>
>>> I was looking for adding a helper in ethdev API.
>>> But I think I can agree with your way of thinking.
>>>
>>
>> The patch breaks running testpmd on Virtio-Net because the driver
>> populates dev_info.max_rx_pktlen but keeps dev_info.max_mtu equal to
>> UINT16_MAX as it was filled in by ethdev. As the result:
>>
>> Ethdev port_id=0 max_rx_pkt_len 11229 > max valid value 9728 Fail to
>> configure port 0
> 
> Similar issue occurred for other net PMD drivers which use default max_mtu (UINT16_MAX).
> More strict checking condition will be added within new patch sooner.
> 

:(

For drivers not providing 'max_mtu' information explicitly, the default 
'UINT16_MAX' is set in ethdev layer.
This prevents calculating PMD specific 'overhead' and the logic in the patch is 
broken.

Indeed this makes inconsistency in the driver too, for example for virtio, it 
claims 'max_rx_pktlen' as "VIRTIO_MAX_RX_PKTLEN (9728)" and 'max_mtu' as 
UINT16_MAX. From 'virtio_mtu_set()' we can see the real limit is 
'VIRTIO_MAX_RX_PKTLEN'.

When PMDs fixed, the logic in this patch can work but not sure if post -rc2 is 
good time to start fixing the PMDs.
Thomas Monjalon Nov. 5, 2020, 10:44 a.m. UTC | #10
05/11/2020 11:37, Ferruh Yigit:
> On 11/5/2020 9:33 AM, Yang, SteveX wrote:
> > 
> > 
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Thursday, November 5, 2020 4:54 PM
> >> To: Thomas Monjalon <thomas@monjalon.net>; Yang, SteveX
> >> <stevex.yang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> >> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> >> Xing, Beilei <beilei.xing@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> >> Iremonger, Bernard <bernard.iremonger@intel.com>; Yang, Qiming
> >> <qiming.yang@intel.com>; mdr@ashroe.eu; nhorman@tuxdriver.com;
> >> david.marchand@redhat.com
> >> Subject: Re: [dpdk-dev] [PATCH v8 1/2] app/testpmd: fix max rx packet
> >> length for VLAN packets
> >>
> >> On 11/4/20 11:39 PM, Thomas Monjalon wrote:
> >>> 04/11/2020 21:19, Ferruh Yigit:
> >>>> On 11/4/2020 5:55 PM, Thomas Monjalon wrote:
> >>>>> 04/11/2020 18:07, Ferruh Yigit:
> >>>>>> On 11/4/2020 4:51 PM, Thomas Monjalon wrote:
> >>>>>>> 03/11/2020 14:29, Ferruh Yigit:
> >>>>>>>> On 11/2/2020 11:48 AM, Ferruh Yigit wrote:
> >>>>>>>>> On 11/2/2020 8:52 AM, SteveX Yang wrote:
> >>>>>>>>>> When the max rx packet length is smaller than the sum of mtu
> >>>>>>>>>> size and ether overhead size, it should be enlarged, otherwise
> >>>>>>>>>> the VLAN packets will be dropped.
> >>>>>>>>>>
> >>>>>>>>>> Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
> >>>>>>>>>
> >>>>>>>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>>>>>>
> >>>>>>>> Applied to dpdk-next-net/main, thanks.
> >>>>>>>>
> >>>>>>>> only 1/2 applied since discussion is going on for 2/2.
> >>>>>>>
> >>>>>>> I'm not sure this testpmd change is good.
> >>>>>>>
> >>>>>>> Reminder: testpmd is for testing the PMDs.
> >>>>>>> Don't we want to see VLAN packets dropped in the case described
> >> above?
> >>>>>>>
> >>>>>>
> >>>>>> The patch set 'max_rx_pkt_len' in a way to make MTU 1500 for all
> >>>>>> PMDs, otherwise testpmd set hard-coded 'RTE_ETHER_MAX_LEN'
> >> value,
> >>>>>> which makes MTU between 1492-1500 depending on PMD.
> >>>>>>
> >>>>>> It is application responsibility to provide correct 'max_rx_pkt_len'.
> >>>>>> I guess the original intention was to set MTU as 1500 but was not
> >>>>>> correct for all PMDs and this patch is fixing it.
> >>>>>>
> >>>>>> The same problem in the ethdev, (assuming 'RTE_ETHER_MAX_LEN'
> >> will
> >>>>>> give MTU 1500), the other patch in the set is to fix it later.
> >>>>>
> >>>>> OK but the testpmd patch is just hiding the issue, isn't it?
> >>>>>
> >>>>
> >>>> I don't think so, issue was application (testpmd) setting the
> >> 'max_rx_pkt_len'
> >>>> wrong.
> >>>>
> >>>> What is hidden?
> >>>
> >>> I was looking for adding a helper in ethdev API.
> >>> But I think I can agree with your way of thinking.
> >>>
> >>
> >> The patch breaks running testpmd on Virtio-Net because the driver
> >> populates dev_info.max_rx_pktlen but keeps dev_info.max_mtu equal to
> >> UINT16_MAX as it was filled in by ethdev. As the result:
> >>
> >> Ethdev port_id=0 max_rx_pkt_len 11229 > max valid value 9728 Fail to
> >> configure port 0
> > 
> > Similar issue occurred for other net PMD drivers which use default max_mtu (UINT16_MAX).
> > More strict checking condition will be added within new patch sooner.
> > 
> 
> :(
> 
> For drivers not providing 'max_mtu' information explicitly, the default 
> 'UINT16_MAX' is set in ethdev layer.
> This prevents calculating PMD specific 'overhead' and the logic in the patch is 
> broken.
> 
> Indeed this makes inconsistency in the driver too, for example for virtio, it 
> claims 'max_rx_pktlen' as "VIRTIO_MAX_RX_PKTLEN (9728)" and 'max_mtu' as 
> UINT16_MAX. From 'virtio_mtu_set()' we can see the real limit is 
> 'VIRTIO_MAX_RX_PKTLEN'.
> 
> When PMDs fixed, the logic in this patch can work but not sure if post -rc2 is 
> good time to start fixing the PMDs.

Do you suggest revert is the best choice here?
Thomas Monjalon Nov. 5, 2020, 10:48 a.m. UTC | #11
+ more maintainers Cc'ed

We have a critical issue with testpmd in -rc2.
It is blocking a lot of testing.
Would be good to do a -rc3 today.
Please see below.

05/11/2020 11:44, Thomas Monjalon:
> 05/11/2020 11:37, Ferruh Yigit:
> > On 11/5/2020 9:33 AM, Yang, SteveX wrote:
> > > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > >> Sent: Thursday, November 5, 2020 4:54 PM
> > >> To: Thomas Monjalon <thomas@monjalon.net>; Yang, SteveX
> > >> <stevex.yang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> > >> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > >> Xing, Beilei <beilei.xing@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> > >> Iremonger, Bernard <bernard.iremonger@intel.com>; Yang, Qiming
> > >> <qiming.yang@intel.com>; mdr@ashroe.eu; nhorman@tuxdriver.com;
> > >> david.marchand@redhat.com
> > >> Subject: Re: [dpdk-dev] [PATCH v8 1/2] app/testpmd: fix max rx packet
> > >> length for VLAN packets
> > >>
> > >> On 11/4/20 11:39 PM, Thomas Monjalon wrote:
> > >>> 04/11/2020 21:19, Ferruh Yigit:
> > >>>> On 11/4/2020 5:55 PM, Thomas Monjalon wrote:
> > >>>>> 04/11/2020 18:07, Ferruh Yigit:
> > >>>>>> On 11/4/2020 4:51 PM, Thomas Monjalon wrote:
> > >>>>>>> 03/11/2020 14:29, Ferruh Yigit:
> > >>>>>>>> On 11/2/2020 11:48 AM, Ferruh Yigit wrote:
> > >>>>>>>>> On 11/2/2020 8:52 AM, SteveX Yang wrote:
> > >>>>>>>>>> When the max rx packet length is smaller than the sum of mtu
> > >>>>>>>>>> size and ether overhead size, it should be enlarged, otherwise
> > >>>>>>>>>> the VLAN packets will be dropped.
> > >>>>>>>>>>
> > >>>>>>>>>> Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
> > >>>>>>>>>>
> > >>>>>>>>>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
> > >>>>>>>>>
> > >>>>>>>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > >>>>>>>>
> > >>>>>>>> Applied to dpdk-next-net/main, thanks.
> > >>>>>>>>
> > >>>>>>>> only 1/2 applied since discussion is going on for 2/2.
> > >>>>>>>
> > >>>>>>> I'm not sure this testpmd change is good.
> > >>>>>>>
> > >>>>>>> Reminder: testpmd is for testing the PMDs.
> > >>>>>>> Don't we want to see VLAN packets dropped in the case described
> > >> above?
> > >>>>>>>
> > >>>>>>
> > >>>>>> The patch set 'max_rx_pkt_len' in a way to make MTU 1500 for all
> > >>>>>> PMDs, otherwise testpmd set hard-coded 'RTE_ETHER_MAX_LEN'
> > >> value,
> > >>>>>> which makes MTU between 1492-1500 depending on PMD.
> > >>>>>>
> > >>>>>> It is application responsibility to provide correct 'max_rx_pkt_len'.
> > >>>>>> I guess the original intention was to set MTU as 1500 but was not
> > >>>>>> correct for all PMDs and this patch is fixing it.
> > >>>>>>
> > >>>>>> The same problem in the ethdev, (assuming 'RTE_ETHER_MAX_LEN'
> > >> will
> > >>>>>> give MTU 1500), the other patch in the set is to fix it later.
> > >>>>>
> > >>>>> OK but the testpmd patch is just hiding the issue, isn't it?
> > >>>>>
> > >>>>
> > >>>> I don't think so, issue was application (testpmd) setting the
> > >> 'max_rx_pkt_len'
> > >>>> wrong.
> > >>>>
> > >>>> What is hidden?
> > >>>
> > >>> I was looking for adding a helper in ethdev API.
> > >>> But I think I can agree with your way of thinking.
> > >>>
> > >>
> > >> The patch breaks running testpmd on Virtio-Net because the driver
> > >> populates dev_info.max_rx_pktlen but keeps dev_info.max_mtu equal to
> > >> UINT16_MAX as it was filled in by ethdev. As the result:
> > >>
> > >> Ethdev port_id=0 max_rx_pkt_len 11229 > max valid value 9728 Fail to
> > >> configure port 0
> > > 
> > > Similar issue occurred for other net PMD drivers which use default max_mtu (UINT16_MAX).
> > > More strict checking condition will be added within new patch sooner.
> > > 
> > 
> > :(
> > 
> > For drivers not providing 'max_mtu' information explicitly, the default 
> > 'UINT16_MAX' is set in ethdev layer.
> > This prevents calculating PMD specific 'overhead' and the logic in the patch is 
> > broken.
> > 
> > Indeed this makes inconsistency in the driver too, for example for virtio, it 
> > claims 'max_rx_pktlen' as "VIRTIO_MAX_RX_PKTLEN (9728)" and 'max_mtu' as 
> > UINT16_MAX. From 'virtio_mtu_set()' we can see the real limit is 
> > 'VIRTIO_MAX_RX_PKTLEN'.
> > 
> > When PMDs fixed, the logic in this patch can work but not sure if post -rc2 is 
> > good time to start fixing the PMDs.
> 
> Do you suggest revert is the best choice here?
Ferruh Yigit Nov. 5, 2020, 10:49 a.m. UTC | #12
On 11/5/2020 10:44 AM, Thomas Monjalon wrote:
> 05/11/2020 11:37, Ferruh Yigit:
>> On 11/5/2020 9:33 AM, Yang, SteveX wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Sent: Thursday, November 5, 2020 4:54 PM
>>>> To: Thomas Monjalon <thomas@monjalon.net>; Yang, SteveX
>>>> <stevex.yang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>>>> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
>>>> Xing, Beilei <beilei.xing@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
>>>> Iremonger, Bernard <bernard.iremonger@intel.com>; Yang, Qiming
>>>> <qiming.yang@intel.com>; mdr@ashroe.eu; nhorman@tuxdriver.com;
>>>> david.marchand@redhat.com
>>>> Subject: Re: [dpdk-dev] [PATCH v8 1/2] app/testpmd: fix max rx packet
>>>> length for VLAN packets
>>>>
>>>> On 11/4/20 11:39 PM, Thomas Monjalon wrote:
>>>>> 04/11/2020 21:19, Ferruh Yigit:
>>>>>> On 11/4/2020 5:55 PM, Thomas Monjalon wrote:
>>>>>>> 04/11/2020 18:07, Ferruh Yigit:
>>>>>>>> On 11/4/2020 4:51 PM, Thomas Monjalon wrote:
>>>>>>>>> 03/11/2020 14:29, Ferruh Yigit:
>>>>>>>>>> On 11/2/2020 11:48 AM, Ferruh Yigit wrote:
>>>>>>>>>>> On 11/2/2020 8:52 AM, SteveX Yang wrote:
>>>>>>>>>>>> When the max rx packet length is smaller than the sum of mtu
>>>>>>>>>>>> size and ether overhead size, it should be enlarged, otherwise
>>>>>>>>>>>> the VLAN packets will be dropped.
>>>>>>>>>>>>
>>>>>>>>>>>> Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
>>>>>>>>>>>
>>>>>>>>>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>>>>>
>>>>>>>>>> Applied to dpdk-next-net/main, thanks.
>>>>>>>>>>
>>>>>>>>>> only 1/2 applied since discussion is going on for 2/2.
>>>>>>>>>
>>>>>>>>> I'm not sure this testpmd change is good.
>>>>>>>>>
>>>>>>>>> Reminder: testpmd is for testing the PMDs.
>>>>>>>>> Don't we want to see VLAN packets dropped in the case described
>>>> above?
>>>>>>>>>
>>>>>>>>
>>>>>>>> The patch set 'max_rx_pkt_len' in a way to make MTU 1500 for all
>>>>>>>> PMDs, otherwise testpmd set hard-coded 'RTE_ETHER_MAX_LEN'
>>>> value,
>>>>>>>> which makes MTU between 1492-1500 depending on PMD.
>>>>>>>>
>>>>>>>> It is application responsibility to provide correct 'max_rx_pkt_len'.
>>>>>>>> I guess the original intention was to set MTU as 1500 but was not
>>>>>>>> correct for all PMDs and this patch is fixing it.
>>>>>>>>
>>>>>>>> The same problem in the ethdev, (assuming 'RTE_ETHER_MAX_LEN'
>>>> will
>>>>>>>> give MTU 1500), the other patch in the set is to fix it later.
>>>>>>>
>>>>>>> OK but the testpmd patch is just hiding the issue, isn't it?
>>>>>>>
>>>>>>
>>>>>> I don't think so, issue was application (testpmd) setting the
>>>> 'max_rx_pkt_len'
>>>>>> wrong.
>>>>>>
>>>>>> What is hidden?
>>>>>
>>>>> I was looking for adding a helper in ethdev API.
>>>>> But I think I can agree with your way of thinking.
>>>>>
>>>>
>>>> The patch breaks running testpmd on Virtio-Net because the driver
>>>> populates dev_info.max_rx_pktlen but keeps dev_info.max_mtu equal to
>>>> UINT16_MAX as it was filled in by ethdev. As the result:
>>>>
>>>> Ethdev port_id=0 max_rx_pkt_len 11229 > max valid value 9728 Fail to
>>>> configure port 0
>>>
>>> Similar issue occurred for other net PMD drivers which use default max_mtu (UINT16_MAX).
>>> More strict checking condition will be added within new patch sooner.
>>>
>>
>> :(
>>
>> For drivers not providing 'max_mtu' information explicitly, the default
>> 'UINT16_MAX' is set in ethdev layer.
>> This prevents calculating PMD specific 'overhead' and the logic in the patch is
>> broken.
>>
>> Indeed this makes inconsistency in the driver too, for example for virtio, it
>> claims 'max_rx_pktlen' as "VIRTIO_MAX_RX_PKTLEN (9728)" and 'max_mtu' as
>> UINT16_MAX. From 'virtio_mtu_set()' we can see the real limit is
>> 'VIRTIO_MAX_RX_PKTLEN'.
>>
>> When PMDs fixed, the logic in this patch can work but not sure if post -rc2 is
>> good time to start fixing the PMDs.
> 
> Do you suggest revert is the best choice here?
> 

One option is revert, but than the issue this patch is trying to fix still remain.

Other option is the extend the patch as Steve sent [1], the check there is more 
like workaround in application, so not nice to have them, but with extending the 
deprecation notice (other patch in this patchset) to fix PMDs too in next 
release, I would be OK to have these checks. What do you think?

[1]
https://patches.dpdk.org/patch/83717/
Ferruh Yigit Nov. 5, 2020, 10:50 a.m. UTC | #13
On 11/5/2020 10:48 AM, Thomas Monjalon wrote:
> + more maintainers Cc'ed
> 
> We have a critical issue with testpmd in -rc2.
> It is blocking a lot of testing.
> Would be good to do a -rc3 today.
> Please see below.
> 
> 05/11/2020 11:44, Thomas Monjalon:
>> 05/11/2020 11:37, Ferruh Yigit:
>>> On 11/5/2020 9:33 AM, Yang, SteveX wrote:
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>> Sent: Thursday, November 5, 2020 4:54 PM
>>>>> To: Thomas Monjalon <thomas@monjalon.net>; Yang, SteveX
>>>>> <stevex.yang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>>>>> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
>>>>> Xing, Beilei <beilei.xing@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
>>>>> Iremonger, Bernard <bernard.iremonger@intel.com>; Yang, Qiming
>>>>> <qiming.yang@intel.com>; mdr@ashroe.eu; nhorman@tuxdriver.com;
>>>>> david.marchand@redhat.com
>>>>> Subject: Re: [dpdk-dev] [PATCH v8 1/2] app/testpmd: fix max rx packet
>>>>> length for VLAN packets
>>>>>
>>>>> On 11/4/20 11:39 PM, Thomas Monjalon wrote:
>>>>>> 04/11/2020 21:19, Ferruh Yigit:
>>>>>>> On 11/4/2020 5:55 PM, Thomas Monjalon wrote:
>>>>>>>> 04/11/2020 18:07, Ferruh Yigit:
>>>>>>>>> On 11/4/2020 4:51 PM, Thomas Monjalon wrote:
>>>>>>>>>> 03/11/2020 14:29, Ferruh Yigit:
>>>>>>>>>>> On 11/2/2020 11:48 AM, Ferruh Yigit wrote:
>>>>>>>>>>>> On 11/2/2020 8:52 AM, SteveX Yang wrote:
>>>>>>>>>>>>> When the max rx packet length is smaller than the sum of mtu
>>>>>>>>>>>>> size and ether overhead size, it should be enlarged, otherwise
>>>>>>>>>>>>> the VLAN packets will be dropped.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
>>>>>>>>>>>>
>>>>>>>>>>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>>>>>>
>>>>>>>>>>> Applied to dpdk-next-net/main, thanks.
>>>>>>>>>>>
>>>>>>>>>>> only 1/2 applied since discussion is going on for 2/2.
>>>>>>>>>>
>>>>>>>>>> I'm not sure this testpmd change is good.
>>>>>>>>>>
>>>>>>>>>> Reminder: testpmd is for testing the PMDs.
>>>>>>>>>> Don't we want to see VLAN packets dropped in the case described
>>>>> above?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The patch set 'max_rx_pkt_len' in a way to make MTU 1500 for all
>>>>>>>>> PMDs, otherwise testpmd set hard-coded 'RTE_ETHER_MAX_LEN'
>>>>> value,
>>>>>>>>> which makes MTU between 1492-1500 depending on PMD.
>>>>>>>>>
>>>>>>>>> It is application responsibility to provide correct 'max_rx_pkt_len'.
>>>>>>>>> I guess the original intention was to set MTU as 1500 but was not
>>>>>>>>> correct for all PMDs and this patch is fixing it.
>>>>>>>>>
>>>>>>>>> The same problem in the ethdev, (assuming 'RTE_ETHER_MAX_LEN'
>>>>> will
>>>>>>>>> give MTU 1500), the other patch in the set is to fix it later.
>>>>>>>>
>>>>>>>> OK but the testpmd patch is just hiding the issue, isn't it?
>>>>>>>>
>>>>>>>
>>>>>>> I don't think so, issue was application (testpmd) setting the
>>>>> 'max_rx_pkt_len'
>>>>>>> wrong.
>>>>>>>
>>>>>>> What is hidden?
>>>>>>
>>>>>> I was looking for adding a helper in ethdev API.
>>>>>> But I think I can agree with your way of thinking.
>>>>>>
>>>>>
>>>>> The patch breaks running testpmd on Virtio-Net because the driver
>>>>> populates dev_info.max_rx_pktlen but keeps dev_info.max_mtu equal to
>>>>> UINT16_MAX as it was filled in by ethdev. As the result:
>>>>>
>>>>> Ethdev port_id=0 max_rx_pkt_len 11229 > max valid value 9728 Fail to
>>>>> configure port 0
>>>>
>>>> Similar issue occurred for other net PMD drivers which use default max_mtu (UINT16_MAX).
>>>> More strict checking condition will be added within new patch sooner.
>>>>
>>>
>>> :(
>>>
>>> For drivers not providing 'max_mtu' information explicitly, the default
>>> 'UINT16_MAX' is set in ethdev layer.
>>> This prevents calculating PMD specific 'overhead' and the logic in the patch is
>>> broken.
>>>
>>> Indeed this makes inconsistency in the driver too, for example for virtio, it
>>> claims 'max_rx_pktlen' as "VIRTIO_MAX_RX_PKTLEN (9728)" and 'max_mtu' as
>>> UINT16_MAX. From 'virtio_mtu_set()' we can see the real limit is
>>> 'VIRTIO_MAX_RX_PKTLEN'.
>>>
>>> When PMDs fixed, the logic in this patch can work but not sure if post -rc2 is
>>> good time to start fixing the PMDs.
>>
>> Do you suggest revert is the best choice here?
> 
> 

(copy/pasting previous reply to this eamil)

One option is revert, but than the issue this patch is trying to fix still remain.

Other option is the extend the patch as Steve sent [1], the check there is more 
like workaround in application, so not nice to have them, but with extending the 
deprecation notice (other patch in this patchset) to fix PMDs too in next 
release, I would be OK to have these checks. What do you think?

[1]
https://patches.dpdk.org/patch/83717/
Olivier Matz Nov. 5, 2020, 1:52 p.m. UTC | #14
On Thu, Nov 05, 2020 at 10:50:45AM +0000, Ferruh Yigit wrote:
> On 11/5/2020 10:48 AM, Thomas Monjalon wrote:
> > + more maintainers Cc'ed
> > 
> > We have a critical issue with testpmd in -rc2.
> > It is blocking a lot of testing.
> > Would be good to do a -rc3 today.
> > Please see below.
> > 
> > 05/11/2020 11:44, Thomas Monjalon:
> > > 05/11/2020 11:37, Ferruh Yigit:
> > > > On 11/5/2020 9:33 AM, Yang, SteveX wrote:
> > > > > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > > > > > Sent: Thursday, November 5, 2020 4:54 PM
> > > > > > To: Thomas Monjalon <thomas@monjalon.net>; Yang, SteveX
> > > > > > <stevex.yang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> > > > > > Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > > > > Xing, Beilei <beilei.xing@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> > > > > > Iremonger, Bernard <bernard.iremonger@intel.com>; Yang, Qiming
> > > > > > <qiming.yang@intel.com>; mdr@ashroe.eu; nhorman@tuxdriver.com;
> > > > > > david.marchand@redhat.com
> > > > > > Subject: Re: [dpdk-dev] [PATCH v8 1/2] app/testpmd: fix max rx packet
> > > > > > length for VLAN packets
> > > > > > 
> > > > > > On 11/4/20 11:39 PM, Thomas Monjalon wrote:
> > > > > > > 04/11/2020 21:19, Ferruh Yigit:
> > > > > > > > On 11/4/2020 5:55 PM, Thomas Monjalon wrote:
> > > > > > > > > 04/11/2020 18:07, Ferruh Yigit:
> > > > > > > > > > On 11/4/2020 4:51 PM, Thomas Monjalon wrote:
> > > > > > > > > > > 03/11/2020 14:29, Ferruh Yigit:
> > > > > > > > > > > > On 11/2/2020 11:48 AM, Ferruh Yigit wrote:
> > > > > > > > > > > > > On 11/2/2020 8:52 AM, SteveX Yang wrote:
> > > > > > > > > > > > > > When the max rx packet length is smaller than the sum of mtu
> > > > > > > > > > > > > > size and ether overhead size, it should be enlarged, otherwise
> > > > > > > > > > > > > > the VLAN packets will be dropped.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Signed-off-by: SteveX Yang <stevex.yang@intel.com>
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > > > > > > > > > > > 
> > > > > > > > > > > > Applied to dpdk-next-net/main, thanks.
> > > > > > > > > > > > 
> > > > > > > > > > > > only 1/2 applied since discussion is going on for 2/2.
> > > > > > > > > > > 
> > > > > > > > > > > I'm not sure this testpmd change is good.
> > > > > > > > > > > 
> > > > > > > > > > > Reminder: testpmd is for testing the PMDs.
> > > > > > > > > > > Don't we want to see VLAN packets dropped in the case described
> > > > > > above?
> > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > The patch set 'max_rx_pkt_len' in a way to make MTU 1500 for all
> > > > > > > > > > PMDs, otherwise testpmd set hard-coded 'RTE_ETHER_MAX_LEN'
> > > > > > value,
> > > > > > > > > > which makes MTU between 1492-1500 depending on PMD.
> > > > > > > > > > 
> > > > > > > > > > It is application responsibility to provide correct 'max_rx_pkt_len'.
> > > > > > > > > > I guess the original intention was to set MTU as 1500 but was not
> > > > > > > > > > correct for all PMDs and this patch is fixing it.
> > > > > > > > > > 
> > > > > > > > > > The same problem in the ethdev, (assuming 'RTE_ETHER_MAX_LEN'
> > > > > > will
> > > > > > > > > > give MTU 1500), the other patch in the set is to fix it later.
> > > > > > > > > 
> > > > > > > > > OK but the testpmd patch is just hiding the issue, isn't it?
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > I don't think so, issue was application (testpmd) setting the
> > > > > > 'max_rx_pkt_len'
> > > > > > > > wrong.
> > > > > > > > 
> > > > > > > > What is hidden?
> > > > > > > 
> > > > > > > I was looking for adding a helper in ethdev API.
> > > > > > > But I think I can agree with your way of thinking.
> > > > > > > 
> > > > > > 
> > > > > > The patch breaks running testpmd on Virtio-Net because the driver
> > > > > > populates dev_info.max_rx_pktlen but keeps dev_info.max_mtu equal to
> > > > > > UINT16_MAX as it was filled in by ethdev. As the result:
> > > > > > 
> > > > > > Ethdev port_id=0 max_rx_pkt_len 11229 > max valid value 9728 Fail to
> > > > > > configure port 0
> > > > > 
> > > > > Similar issue occurred for other net PMD drivers which use default max_mtu (UINT16_MAX).
> > > > > More strict checking condition will be added within new patch sooner.
> > > > > 
> > > > 
> > > > :(
> > > > 
> > > > For drivers not providing 'max_mtu' information explicitly, the default
> > > > 'UINT16_MAX' is set in ethdev layer.
> > > > This prevents calculating PMD specific 'overhead' and the logic in the patch is
> > > > broken.
> > > > 
> > > > Indeed this makes inconsistency in the driver too, for example for virtio, it
> > > > claims 'max_rx_pktlen' as "VIRTIO_MAX_RX_PKTLEN (9728)" and 'max_mtu' as
> > > > UINT16_MAX. From 'virtio_mtu_set()' we can see the real limit is
> > > > 'VIRTIO_MAX_RX_PKTLEN'.
> > > > 
> > > > When PMDs fixed, the logic in this patch can work but not sure if post -rc2 is
> > > > good time to start fixing the PMDs.
> > > 
> > > Do you suggest revert is the best choice here?
> > 
> > 
> 
> (copy/pasting previous reply to this eamil)
> 
> One option is revert, but than the issue this patch is trying to fix still remain.
> 
> Other option is the extend the patch as Steve sent [1], the check there is
> more like workaround in application, so not nice to have them, but with
> extending the deprecation notice (other patch in this patchset) to fix PMDs
> too in next release, I would be OK to have these checks. What do you think?

+1 for this second option.

I think it is ok to have a workaround to fix an issue. Clarifying and
uniformizing the ethdev/drivers behavior in that area can come in a
second time.

> [1]
> https://patches.dpdk.org/patch/83717/
Lance Richardson Nov. 5, 2020, 3:11 p.m. UTC | #15
With this change, the bnxt driver fails to initialize under testpmd:

Configuring Port 0 (socket 0)
Port 0 failed to enable Rx offload JUMBO_FRAME
Fail to configure port 0
EAL: Error - exiting with code: 1

It appears that the cause is this bit of code in bnxt_ethdev.c:

        if (bp->eth_dev->data->mtu > RTE_ETHER_MTU) {
                bp->eth_dev->data->dev_conf.rxmode.offloads |=
                        DEV_RX_OFFLOAD_JUMBO_FRAME;
                bp->flags |= BNXT_FLAG_JUMBO;
        } else {
                bp->eth_dev->data->dev_conf.rxmode.offloads &=
                        ~DEV_RX_OFFLOAD_JUMBO_FRAME;
                bp->flags &= ~BNXT_FLAG_JUMBO;
        }

Should a PMD be overriding this offload on dev_start()? Or should this
test be changed to be based on max_rx_pkt_len instead of mtu?

Thanks,
    Lance

On Thu, Nov 5, 2020 at 8:52 AM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> On Thu, Nov 05, 2020 at 10:50:45AM +0000, Ferruh Yigit wrote:
> > On 11/5/2020 10:48 AM, Thomas Monjalon wrote:
> > > + more maintainers Cc'ed
> > >
> > > We have a critical issue with testpmd in -rc2.
> > > It is blocking a lot of testing.
> > > Would be good to do a -rc3 today.
> > > Please see below.
> > >
> > > 05/11/2020 11:44, Thomas Monjalon:
> > > > 05/11/2020 11:37, Ferruh Yigit:
> > > > > On 11/5/2020 9:33 AM, Yang, SteveX wrote:
> > > > > > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > > > > > > Sent: Thursday, November 5, 2020 4:54 PM
> > > > > > > To: Thomas Monjalon <thomas@monjalon.net>; Yang, SteveX
> > > > > > > <stevex.yang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> > > > > > > Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > > > > > Xing, Beilei <beilei.xing@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> > > > > > > Iremonger, Bernard <bernard.iremonger@intel.com>; Yang, Qiming
> > > > > > > <qiming.yang@intel.com>; mdr@ashroe.eu; nhorman@tuxdriver.com;
> > > > > > > david.marchand@redhat.com
> > > > > > > Subject: Re: [dpdk-dev] [PATCH v8 1/2] app/testpmd: fix max rx packet
> > > > > > > length for VLAN packets
> > > > > > >
> > > > > > > On 11/4/20 11:39 PM, Thomas Monjalon wrote:
> > > > > > > > 04/11/2020 21:19, Ferruh Yigit:
> > > > > > > > > On 11/4/2020 5:55 PM, Thomas Monjalon wrote:
> > > > > > > > > > 04/11/2020 18:07, Ferruh Yigit:
> > > > > > > > > > > On 11/4/2020 4:51 PM, Thomas Monjalon wrote:
> > > > > > > > > > > > 03/11/2020 14:29, Ferruh Yigit:
> > > > > > > > > > > > > On 11/2/2020 11:48 AM, Ferruh Yigit wrote:
> > > > > > > > > > > > > > On 11/2/2020 8:52 AM, SteveX Yang wrote:
> > > > > > > > > > > > > > > When the max rx packet length is smaller than the sum of mtu
> > > > > > > > > > > > > > > size and ether overhead size, it should be enlarged, otherwise
> > > > > > > > > > > > > > > the VLAN packets will be dropped.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Signed-off-by: SteveX Yang <stevex.yang@intel.com>
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > > > > > > > > > > > >
> > > > > > > > > > > > > Applied to dpdk-next-net/main, thanks.
> > > > > > > > > > > > >
> > > > > > > > > > > > > only 1/2 applied since discussion is going on for 2/2.
> > > > > > > > > > > >
> > > > > > > > > > > > I'm not sure this testpmd change is good.
> > > > > > > > > > > >
> > > > > > > > > > > > Reminder: testpmd is for testing the PMDs.
> > > > > > > > > > > > Don't we want to see VLAN packets dropped in the case described
> > > > > > > above?
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > The patch set 'max_rx_pkt_len' in a way to make MTU 1500 for all
> > > > > > > > > > > PMDs, otherwise testpmd set hard-coded 'RTE_ETHER_MAX_LEN'
> > > > > > > value,
> > > > > > > > > > > which makes MTU between 1492-1500 depending on PMD.
> > > > > > > > > > >
> > > > > > > > > > > It is application responsibility to provide correct 'max_rx_pkt_len'.
> > > > > > > > > > > I guess the original intention was to set MTU as 1500 but was not
> > > > > > > > > > > correct for all PMDs and this patch is fixing it.
> > > > > > > > > > >
> > > > > > > > > > > The same problem in the ethdev, (assuming 'RTE_ETHER_MAX_LEN'
> > > > > > > will
> > > > > > > > > > > give MTU 1500), the other patch in the set is to fix it later.
> > > > > > > > > >
> > > > > > > > > > OK but the testpmd patch is just hiding the issue, isn't it?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > I don't think so, issue was application (testpmd) setting the
> > > > > > > 'max_rx_pkt_len'
> > > > > > > > > wrong.
> > > > > > > > >
> > > > > > > > > What is hidden?
> > > > > > > >
> > > > > > > > I was looking for adding a helper in ethdev API.
> > > > > > > > But I think I can agree with your way of thinking.
> > > > > > > >
> > > > > > >
> > > > > > > The patch breaks running testpmd on Virtio-Net because the driver
> > > > > > > populates dev_info.max_rx_pktlen but keeps dev_info.max_mtu equal to
> > > > > > > UINT16_MAX as it was filled in by ethdev. As the result:
> > > > > > >
> > > > > > > Ethdev port_id=0 max_rx_pkt_len 11229 > max valid value 9728 Fail to
> > > > > > > configure port 0
> > > > > >
> > > > > > Similar issue occurred for other net PMD drivers which use default max_mtu (UINT16_MAX).
> > > > > > More strict checking condition will be added within new patch sooner.
> > > > > >
> > > > >
> > > > > :(
> > > > >
> > > > > For drivers not providing 'max_mtu' information explicitly, the default
> > > > > 'UINT16_MAX' is set in ethdev layer.
> > > > > This prevents calculating PMD specific 'overhead' and the logic in the patch is
> > > > > broken.
> > > > >
> > > > > Indeed this makes inconsistency in the driver too, for example for virtio, it
> > > > > claims 'max_rx_pktlen' as "VIRTIO_MAX_RX_PKTLEN (9728)" and 'max_mtu' as
> > > > > UINT16_MAX. From 'virtio_mtu_set()' we can see the real limit is
> > > > > 'VIRTIO_MAX_RX_PKTLEN'.
> > > > >
> > > > > When PMDs fixed, the logic in this patch can work but not sure if post -rc2 is
> > > > > good time to start fixing the PMDs.
> > > >
> > > > Do you suggest revert is the best choice here?
> > >
> > >
> >
> > (copy/pasting previous reply to this eamil)
> >
> > One option is revert, but than the issue this patch is trying to fix still remain.
> >
> > Other option is the extend the patch as Steve sent [1], the check there is
> > more like workaround in application, so not nice to have them, but with
> > extending the deprecation notice (other patch in this patchset) to fix PMDs
> > too in next release, I would be OK to have these checks. What do you think?
>
> +1 for this second option.
>
> I think it is ok to have a workaround to fix an issue. Clarifying and
> uniformizing the ethdev/drivers behavior in that area can come in a
> second time.
>
> > [1]
> > https://patches.dpdk.org/patch/83717/
Ferruh Yigit Nov. 5, 2020, 3:56 p.m. UTC | #16
On 11/5/2020 3:11 PM, Lance Richardson wrote:
> With this change, the bnxt driver fails to initialize under testpmd:
> 
> Configuring Port 0 (socket 0)
> Port 0 failed to enable Rx offload JUMBO_FRAME
> Fail to configure port 0
> EAL: Error - exiting with code: 1
> 
> It appears that the cause is this bit of code in bnxt_ethdev.c:
> 
>          if (bp->eth_dev->data->mtu > RTE_ETHER_MTU) {
>                  bp->eth_dev->data->dev_conf.rxmode.offloads |=
>                          DEV_RX_OFFLOAD_JUMBO_FRAME;
>                  bp->flags |= BNXT_FLAG_JUMBO;
>          } else {
>                  bp->eth_dev->data->dev_conf.rxmode.offloads &=
>                          ~DEV_RX_OFFLOAD_JUMBO_FRAME;
>                  bp->flags &= ~BNXT_FLAG_JUMBO;
>          }
> 
> Should a PMD be overriding this offload on dev_start()? Or should this
> test be changed to be based on max_rx_pkt_len instead of mtu?
> 

I think testing 'mtu' is correct thing to do, problem looks somewhere else,

First the code cause problem in the driver looks in another place, following in 
'bnxt_mtu_set_op()':

           if (new_mtu > RTE_ETHER_MTU) {
                   bp->flags |= BNXT_FLAG_JUMBO;
                   bp->eth_dev->data->dev_conf.rxmode.offloads |=
                           DEV_RX_OFFLOAD_JUMBO_FRAME;
           } else {
                   bp->eth_dev->data->dev_conf.rxmode.offloads &=
                           ~DEV_RX_OFFLOAD_JUMBO_FRAME;
                   bp->flags &= ~BNXT_FLAG_JUMBO;
           }

The backtrace is
rte_eth_dev_configure()
     bnxt_dev_configure_op()
         bnxt_mtu_set_op()
	    //cleans the JUMBO FRAME CONFIG
     //complains requested JUMBO FRAME is not set


How 'ethdev' is checking for jumbo frame is wrong:
http://lxr.dpdk.org/dpdk/latest/source/lib/librte_ethdev/rte_ethdev.c#L1344

If application doesn't set the JUMBO FRAME flag, it doesn't allow max_rx_pkt_len 
to be more than 'RTE_ETHER_MAX_LEN', instead this should be checked against the 
MTU, not 'RTE_ETHER_MAX_LEN', the deprecation notice in other patch is to fix this.

To workaround above behavior, testpmd sets the JUMBO FRAME flag, and bnxt 
detects requested frame size doesn't require the JUMBO FRAME support and unsets 
the flag during configure, but this time 'rte_eth_dev_configure()' API complains 
that requested offload (JUMBO FRAME) is not enabled by the driver.

The ethdev part not fixed in this release because there are PMDs testing JUMBO 
frame against 'max_rx_pkt_len', didn't want to create unexpected side affect for 
them.

Not sure what to do,

Perhaps we can revert the patch for this release, and in next release we can fix 
testpmd, ethdev and PMDs altogether. Even possible to remove the JUMBO FRAME 
offload flag as already suggested:
https://mails.dpdk.org/archives/dev/2020-November/190940.html

> Thanks,
>      Lance
> 
> On Thu, Nov 5, 2020 at 8:52 AM Olivier Matz <olivier.matz@6wind.com> wrote:
>>
>> On Thu, Nov 05, 2020 at 10:50:45AM +0000, Ferruh Yigit wrote:
>>> On 11/5/2020 10:48 AM, Thomas Monjalon wrote:
>>>> + more maintainers Cc'ed
>>>>
>>>> We have a critical issue with testpmd in -rc2.
>>>> It is blocking a lot of testing.
>>>> Would be good to do a -rc3 today.
>>>> Please see below.
>>>>
>>>> 05/11/2020 11:44, Thomas Monjalon:
>>>>> 05/11/2020 11:37, Ferruh Yigit:
>>>>>> On 11/5/2020 9:33 AM, Yang, SteveX wrote:
>>>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>>>> Sent: Thursday, November 5, 2020 4:54 PM
>>>>>>>> To: Thomas Monjalon <thomas@monjalon.net>; Yang, SteveX
>>>>>>>> <stevex.yang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>>>>>>>> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
>>>>>>>> Xing, Beilei <beilei.xing@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
>>>>>>>> Iremonger, Bernard <bernard.iremonger@intel.com>; Yang, Qiming
>>>>>>>> <qiming.yang@intel.com>; mdr@ashroe.eu; nhorman@tuxdriver.com;
>>>>>>>> david.marchand@redhat.com
>>>>>>>> Subject: Re: [dpdk-dev] [PATCH v8 1/2] app/testpmd: fix max rx packet
>>>>>>>> length for VLAN packets
>>>>>>>>
>>>>>>>> On 11/4/20 11:39 PM, Thomas Monjalon wrote:
>>>>>>>>> 04/11/2020 21:19, Ferruh Yigit:
>>>>>>>>>> On 11/4/2020 5:55 PM, Thomas Monjalon wrote:
>>>>>>>>>>> 04/11/2020 18:07, Ferruh Yigit:
>>>>>>>>>>>> On 11/4/2020 4:51 PM, Thomas Monjalon wrote:
>>>>>>>>>>>>> 03/11/2020 14:29, Ferruh Yigit:
>>>>>>>>>>>>>> On 11/2/2020 11:48 AM, Ferruh Yigit wrote:
>>>>>>>>>>>>>>> On 11/2/2020 8:52 AM, SteveX Yang wrote:
>>>>>>>>>>>>>>>> When the max rx packet length is smaller than the sum of mtu
>>>>>>>>>>>>>>>> size and ether overhead size, it should be enlarged, otherwise
>>>>>>>>>>>>>>>> the VLAN packets will be dropped.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines")
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Applied to dpdk-next-net/main, thanks.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> only 1/2 applied since discussion is going on for 2/2.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I'm not sure this testpmd change is good.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Reminder: testpmd is for testing the PMDs.
>>>>>>>>>>>>> Don't we want to see VLAN packets dropped in the case described
>>>>>>>> above?
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> The patch set 'max_rx_pkt_len' in a way to make MTU 1500 for all
>>>>>>>>>>>> PMDs, otherwise testpmd set hard-coded 'RTE_ETHER_MAX_LEN'
>>>>>>>> value,
>>>>>>>>>>>> which makes MTU between 1492-1500 depending on PMD.
>>>>>>>>>>>>
>>>>>>>>>>>> It is application responsibility to provide correct 'max_rx_pkt_len'.
>>>>>>>>>>>> I guess the original intention was to set MTU as 1500 but was not
>>>>>>>>>>>> correct for all PMDs and this patch is fixing it.
>>>>>>>>>>>>
>>>>>>>>>>>> The same problem in the ethdev, (assuming 'RTE_ETHER_MAX_LEN'
>>>>>>>> will
>>>>>>>>>>>> give MTU 1500), the other patch in the set is to fix it later.
>>>>>>>>>>>
>>>>>>>>>>> OK but the testpmd patch is just hiding the issue, isn't it?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I don't think so, issue was application (testpmd) setting the
>>>>>>>> 'max_rx_pkt_len'
>>>>>>>>>> wrong.
>>>>>>>>>>
>>>>>>>>>> What is hidden?
>>>>>>>>>
>>>>>>>>> I was looking for adding a helper in ethdev API.
>>>>>>>>> But I think I can agree with your way of thinking.
>>>>>>>>>
>>>>>>>>
>>>>>>>> The patch breaks running testpmd on Virtio-Net because the driver
>>>>>>>> populates dev_info.max_rx_pktlen but keeps dev_info.max_mtu equal to
>>>>>>>> UINT16_MAX as it was filled in by ethdev. As the result:
>>>>>>>>
>>>>>>>> Ethdev port_id=0 max_rx_pkt_len 11229 > max valid value 9728 Fail to
>>>>>>>> configure port 0
>>>>>>>
>>>>>>> Similar issue occurred for other net PMD drivers which use default max_mtu (UINT16_MAX).
>>>>>>> More strict checking condition will be added within new patch sooner.
>>>>>>>
>>>>>>
>>>>>> :(
>>>>>>
>>>>>> For drivers not providing 'max_mtu' information explicitly, the default
>>>>>> 'UINT16_MAX' is set in ethdev layer.
>>>>>> This prevents calculating PMD specific 'overhead' and the logic in the patch is
>>>>>> broken.
>>>>>>
>>>>>> Indeed this makes inconsistency in the driver too, for example for virtio, it
>>>>>> claims 'max_rx_pktlen' as "VIRTIO_MAX_RX_PKTLEN (9728)" and 'max_mtu' as
>>>>>> UINT16_MAX. From 'virtio_mtu_set()' we can see the real limit is
>>>>>> 'VIRTIO_MAX_RX_PKTLEN'.
>>>>>>
>>>>>> When PMDs fixed, the logic in this patch can work but not sure if post -rc2 is
>>>>>> good time to start fixing the PMDs.
>>>>>
>>>>> Do you suggest revert is the best choice here?
>>>>
>>>>
>>>
>>> (copy/pasting previous reply to this eamil)
>>>
>>> One option is revert, but than the issue this patch is trying to fix still remain.
>>>
>>> Other option is the extend the patch as Steve sent [1], the check there is
>>> more like workaround in application, so not nice to have them, but with
>>> extending the deprecation notice (other patch in this patchset) to fix PMDs
>>> too in next release, I would be OK to have these checks. What do you think?
>>
>> +1 for this second option.
>>
>> I think it is ok to have a workaround to fix an issue. Clarifying and
>> uniformizing the ethdev/drivers behavior in that area can come in a
>> second time.
>>
>>> [1]
>>> https://patches.dpdk.org/patch/83717/
Lance Richardson Nov. 5, 2020, 4:23 p.m. UTC | #17
> First the code cause problem in the driver looks in another place, following in
> 'bnxt_mtu_set_op()':
>
>            if (new_mtu > RTE_ETHER_MTU) {
>                    bp->flags |= BNXT_FLAG_JUMBO;
>                    bp->eth_dev->data->dev_conf.rxmode.offloads |=
>                            DEV_RX_OFFLOAD_JUMBO_FRAME;
>            } else {
>                    bp->eth_dev->data->dev_conf.rxmode.offloads &=
>                            ~DEV_RX_OFFLOAD_JUMBO_FRAME;
>                    bp->flags &= ~BNXT_FLAG_JUMBO;
>            }
>

You're correct, the issue in this case is definitely in bnxt_mtu_set_op(),
not the similar code that is executed for the start op.

+1 to the idea of removing DEV_RX_OFFLOAD_JUMBO_FRAME.
diff mbox series

Patch

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 33fc0fddf..c263121a9 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1421,6 +1421,7 @@  init_config(void)
 	struct rte_gro_param gro_param;
 	uint32_t gso_types;
 	uint16_t data_size;
+	uint16_t overhead_len;
 	bool warning = 0;
 	int k;
 	int ret;
@@ -1457,6 +1458,28 @@  init_config(void)
 			rte_exit(EXIT_FAILURE,
 				 "rte_eth_dev_info_get() failed\n");
 
+		/* Update the max_rx_pkt_len to have MTU as RTE_ETHER_MTU */
+		if (port->dev_info.max_rx_pktlen && port->dev_info.max_mtu)
+			overhead_len = port->dev_info.max_rx_pktlen -
+				port->dev_info.max_mtu;
+		else
+			overhead_len = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
+
+		port->dev_conf.rxmode.max_rx_pkt_len =
+						RTE_ETHER_MTU + overhead_len;
+
+		/*
+		 * This is workaround to avoid resize max rx packet len.
+		 * Ethdev assumes jumbo frame size must be greater than
+		 * RTE_ETHER_MAX_LEN, and will resize 'max_rx_pkt_len' to
+		 * default value when it is greater than RTE_ETHER_MAX_LEN
+		 * for normal frame.
+		 */
+		if (port->dev_conf.rxmode.max_rx_pkt_len > RTE_ETHER_MAX_LEN) {
+			port->dev_conf.rxmode.offloads |=
+						DEV_RX_OFFLOAD_JUMBO_FRAME;
+		}
+
 		if (!(port->dev_info.tx_offload_capa &
 		      DEV_TX_OFFLOAD_MBUF_FAST_FREE))
 			port->dev_conf.txmode.offloads &=