diff mbox series

[v8,2/2] doc: annouce deprecation of jumbo frame flag condition

Message ID 20201102085234.72779-3-stevex.yang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series fix default max mtu size when device configured |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/iol-broadcom-Functional fail Functional Testing issues
ci/iol-intel-Functional fail Functional Testing issues
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS

Commit Message

Steve Yang Nov. 2, 2020, 8:52 a.m. UTC 
  Annouce to replace 'RTE_ETHER_MAX_LEN' with 'RTE_ETHER_MTU' as type
condition of jumbo frame. Involved scopes:
- rte_ethdev;
- app, e.g.: test-pmd, test-eventdev;
- examples, e.g.: ipsec-secgw, l3fwd, vhost;
- net PMDs which support VLAN tag(s) within overhead, e.g.: i40e, ixgbe;

Signed-off-by: SteveX Yang <stevex.yang@intel.com>
---
 doc/guides/rel_notes/deprecation.rst | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Ferruh Yigit Nov. 2, 2020, 11:50 a.m. UTC | #1 
On 11/2/2020 8:52 AM, SteveX Yang wrote:
> Annouce to replace 'RTE_ETHER_MAX_LEN' with 'RTE_ETHER_MTU' as type
> condition of jumbo frame. Involved scopes:
> - rte_ethdev;
> - app, e.g.: test-pmd, test-eventdev;
> - examples, e.g.: ipsec-secgw, l3fwd, vhost;
> - net PMDs which support VLAN tag(s) within overhead, e.g.: i40e, ixgbe;
> 
> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
> ---
>   doc/guides/rel_notes/deprecation.rst | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 2e082499b..fae139f01 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -138,6 +138,18 @@ Deprecation Notices
>     will be limited to maximum 256 queues.
>     Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS`` will be removed.
>   
> +* ethdev: Offload flag ``DEV_RX_OFFLOAD_JUMBO_FRAME`` will be set according to
> +  ``RTE_ETHER_MTU`` in next release. Currently, the jumbo frame uses the
> +  ``RTE_ETHER_MAX_LEN`` as boundary condition. When the MTU (1500) set, the
> +  frame type of rx packet will be different if used different overhead, it will
> +  cause the consistency issue. Hence, using fixed value ``RTE_ETHER_MTU`` can
> +  avoid this issue.
> +  Following scopes will be changed:
> +  - ``rte_ethdev``
> +  - ``app``, e.g.: ``test-pmd``, ``test-eventdev``;
> +  - ``examples``, e.g.: ``ipsec-secgw``, ``l3fwd``, ``vhost``;
> +  - net PMDs which support VLAN tag(s) within overhead, e.g.: ``i40e``;
> +
>   * cryptodev: support for using IV with all sizes is added, J0 still can
>     be used but only when IV length in following structs ``rte_crypto_auth_xform``,
>     ``rte_crypto_aead_xform`` is set to zero. When IV length is greater or equal
> 

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

The mentioned code in the ethdev is:
http://lxr.dpdk.org/dpdk/v20.08/source/lib/librte_ethdev/rte_ethdev.c#L1345

cc'ed other ethdev maintainers.
Andrew Rybchenko Nov. 2, 2020, 1:18 p.m. UTC | #2 
On 11/2/20 11:52 AM, SteveX Yang wrote:
> Annouce to replace 'RTE_ETHER_MAX_LEN' with 'RTE_ETHER_MTU' as type
> condition of jumbo frame. Involved scopes:
> - rte_ethdev;
> - app, e.g.: test-pmd, test-eventdev;
> - examples, e.g.: ipsec-secgw, l3fwd, vhost;
> - net PMDs which support VLAN tag(s) within overhead, e.g.: i40e, ixgbe;
> 
> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
> ---
>   doc/guides/rel_notes/deprecation.rst | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 2e082499b..fae139f01 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -138,6 +138,18 @@ Deprecation Notices
>     will be limited to maximum 256 queues.
>     Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS`` will be removed.
>   
> +* ethdev: Offload flag ``DEV_RX_OFFLOAD_JUMBO_FRAME`` will be set according to
> +  ``RTE_ETHER_MTU`` in next release. Currently, the jumbo frame uses the
> +  ``RTE_ETHER_MAX_LEN`` as boundary condition. When the MTU (1500) set, the
> +  frame type of rx packet will be different if used different overhead, it will
> +  cause the consistency issue. Hence, using fixed value ``RTE_ETHER_MTU`` can
> +  avoid this issue.
> +  Following scopes will be changed:
> +  - ``rte_ethdev``
> +  - ``app``, e.g.: ``test-pmd``, ``test-eventdev``;
> +  - ``examples``, e.g.: ``ipsec-secgw``, ``l3fwd``, ``vhost``;
> +  - net PMDs which support VLAN tag(s) within overhead, e.g.: ``i40e``;
> +
>   * cryptodev: support for using IV with all sizes is added, J0 still can
>     be used but only when IV length in following structs ``rte_crypto_auth_xform``,
>     ``rte_crypto_aead_xform`` is set to zero. When IV length is greater or equal
> 

If so, what's the point to have the offload? May be just deprecate and
later remove it?
Ferruh Yigit Nov. 2, 2020, 1:58 p.m. UTC | #3 
On 11/2/2020 1:18 PM, Andrew Rybchenko wrote:
> On 11/2/20 11:52 AM, SteveX Yang wrote:
>> Annouce to replace 'RTE_ETHER_MAX_LEN' with 'RTE_ETHER_MTU' as type
>> condition of jumbo frame. Involved scopes:
>> - rte_ethdev;
>> - app, e.g.: test-pmd, test-eventdev;
>> - examples, e.g.: ipsec-secgw, l3fwd, vhost;
>> - net PMDs which support VLAN tag(s) within overhead, e.g.: i40e, ixgbe;
>>
>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
>> ---
>>   doc/guides/rel_notes/deprecation.rst | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/doc/guides/rel_notes/deprecation.rst 
>> b/doc/guides/rel_notes/deprecation.rst
>> index 2e082499b..fae139f01 100644
>> --- a/doc/guides/rel_notes/deprecation.rst
>> +++ b/doc/guides/rel_notes/deprecation.rst
>> @@ -138,6 +138,18 @@ Deprecation Notices
>>     will be limited to maximum 256 queues.
>>     Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS`` will be removed.
>> +* ethdev: Offload flag ``DEV_RX_OFFLOAD_JUMBO_FRAME`` will be set according to
>> +  ``RTE_ETHER_MTU`` in next release. Currently, the jumbo frame uses the
>> +  ``RTE_ETHER_MAX_LEN`` as boundary condition. When the MTU (1500) set, the
>> +  frame type of rx packet will be different if used different overhead, it will
>> +  cause the consistency issue. Hence, using fixed value ``RTE_ETHER_MTU`` can
>> +  avoid this issue.
>> +  Following scopes will be changed:
>> +  - ``rte_ethdev``
>> +  - ``app``, e.g.: ``test-pmd``, ``test-eventdev``;
>> +  - ``examples``, e.g.: ``ipsec-secgw``, ``l3fwd``, ``vhost``;
>> +  - net PMDs which support VLAN tag(s) within overhead, e.g.: ``i40e``;
>> +
>>   * cryptodev: support for using IV with all sizes is added, J0 still can
>>     be used but only when IV length in following structs 
>> ``rte_crypto_auth_xform``,
>>     ``rte_crypto_aead_xform`` is set to zero. When IV length is greater or equal
>>
> 
> If so, what's the point to have the offload? May be just deprecate and
> later remove it?
> 

Above just changes the condition of what is called jumbo frame, nothing more.

ethdev assumes max frame size (without jumbo frame support) can be 
'RTE_ETHER_MAX_LEN' (1518)

But a PMD can support double VLAN, and it can have RTE_ETHER_MAX_LEN + 8 bytes 
frame size and still may not support jumbo frame.

In that case ethdev overwrites the frame size as RTE_ETHER_MAX_LEN, and this 
will reduce the supported MTU to 1492 (instead of expected 1500).
Above deprecation notice is to fix this.
Ananyev, Konstantin Nov. 2, 2020, 4:05 p.m. UTC | #4 
> On 11/2/2020 1:18 PM, Andrew Rybchenko wrote:
> > On 11/2/20 11:52 AM, SteveX Yang wrote:
> >> Annouce to replace 'RTE_ETHER_MAX_LEN' with 'RTE_ETHER_MTU' as type
> >> condition of jumbo frame. Involved scopes:
> >> - rte_ethdev;
> >> - app, e.g.: test-pmd, test-eventdev;
> >> - examples, e.g.: ipsec-secgw, l3fwd, vhost;
> >> - net PMDs which support VLAN tag(s) within overhead, e.g.: i40e, ixgbe;
> >>
> >> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
> >> ---
> >>   doc/guides/rel_notes/deprecation.rst | 12 ++++++++++++
> >>   1 file changed, 12 insertions(+)
> >>
> >> diff --git a/doc/guides/rel_notes/deprecation.rst
> >> b/doc/guides/rel_notes/deprecation.rst
> >> index 2e082499b..fae139f01 100644
> >> --- a/doc/guides/rel_notes/deprecation.rst
> >> +++ b/doc/guides/rel_notes/deprecation.rst
> >> @@ -138,6 +138,18 @@ Deprecation Notices
> >>     will be limited to maximum 256 queues.
> >>     Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS`` will be removed.
> >> +* ethdev: Offload flag ``DEV_RX_OFFLOAD_JUMBO_FRAME`` will be set according to
> >> +  ``RTE_ETHER_MTU`` in next release. Currently, the jumbo frame uses the
> >> +  ``RTE_ETHER_MAX_LEN`` as boundary condition. When the MTU (1500) set, the
> >> +  frame type of rx packet will be different if used different overhead, it will
> >> +  cause the consistency issue. Hence, using fixed value ``RTE_ETHER_MTU`` can
> >> +  avoid this issue.
> >> +  Following scopes will be changed:
> >> +  - ``rte_ethdev``
> >> +  - ``app``, e.g.: ``test-pmd``, ``test-eventdev``;
> >> +  - ``examples``, e.g.: ``ipsec-secgw``, ``l3fwd``, ``vhost``;
> >> +  - net PMDs which support VLAN tag(s) within overhead, e.g.: ``i40e``;
> >> +
> >>   * cryptodev: support for using IV with all sizes is added, J0 still can
> >>     be used but only when IV length in following structs
> >> ``rte_crypto_auth_xform``,
> >>     ``rte_crypto_aead_xform`` is set to zero. When IV length is greater or equal
> >>
> >
> > If so, what's the point to have the offload? May be just deprecate and
> > later remove it?
> >

Same thought actually, can we remove DEV_RX_OFFLOAD_JUMBO_FRAME flag completely?
PMD can decide based on provided MTU size does segmentation, etc. is needed.

> 
> Above just changes the condition of what is called jumbo frame, nothing more.
> 
> ethdev assumes max frame size (without jumbo frame support) can be
> 'RTE_ETHER_MAX_LEN' (1518)
> 
> But a PMD can support double VLAN, and it can have RTE_ETHER_MAX_LEN + 8 bytes
> frame size and still may not support jumbo frame.
> 
> In that case ethdev overwrites the frame size as RTE_ETHER_MAX_LEN, and this
> will reduce the supported MTU to 1492 (instead of expected 1500).
> Above deprecation notice is to fix this.
Ferruh Yigit Nov. 24, 2020, 5:46 p.m. UTC | #5 
On 11/3/2020 9:07 AM, Yang, SteveX wrote:
> 
> 
>> -----Original Message-----
>> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> Sent: Tuesday, November 3, 2020 12:05 AM
>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko
>> <andrew.rybchenko@oktetlabs.ru>; Yang, SteveX <stevex.yang@intel.com>;
>> dev@dpdk.org
>> Cc: 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
>> Subject: RE: [dpdk-dev] [PATCH v8 2/2] doc: annouce deprecation of jumbo
>> frame flag condition
>>
>>
>>
>>> On 11/2/2020 1:18 PM, Andrew Rybchenko wrote:
>>>> On 11/2/20 11:52 AM, SteveX Yang wrote:
>>>>> Annouce to replace 'RTE_ETHER_MAX_LEN' with 'RTE_ETHER_MTU' as
>> type
>>>>> condition of jumbo frame. Involved scopes:
>>>>> - rte_ethdev;
>>>>> - app, e.g.: test-pmd, test-eventdev;
>>>>> - examples, e.g.: ipsec-secgw, l3fwd, vhost;
>>>>> - net PMDs which support VLAN tag(s) within overhead, e.g.: i40e,
>>>>> ixgbe;
>>>>>
>>>>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
>>>>> ---
>>>>>    doc/guides/rel_notes/deprecation.rst | 12 ++++++++++++
>>>>>    1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/doc/guides/rel_notes/deprecation.rst
>>>>> b/doc/guides/rel_notes/deprecation.rst
>>>>> index 2e082499b..fae139f01 100644
>>>>> --- a/doc/guides/rel_notes/deprecation.rst
>>>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>>>> @@ -138,6 +138,18 @@ Deprecation Notices
>>>>>      will be limited to maximum 256 queues.
>>>>>      Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS`` will be
>> removed.
>>>>> +* ethdev: Offload flag ``DEV_RX_OFFLOAD_JUMBO_FRAME`` will be
>> set
>>>>> +according to
>>>>> +  ``RTE_ETHER_MTU`` in next release. Currently, the jumbo frame
>>>>> +uses the
>>>>> +  ``RTE_ETHER_MAX_LEN`` as boundary condition. When the MTU
>> (1500)
>>>>> +set, the
>>>>> +  frame type of rx packet will be different if used different
>>>>> +overhead, it will
>>>>> +  cause the consistency issue. Hence, using fixed value
>>>>> +``RTE_ETHER_MTU`` can
>>>>> +  avoid this issue.
>>>>> +  Following scopes will be changed:
>>>>> +  - ``rte_ethdev``
>>>>> +  - ``app``, e.g.: ``test-pmd``, ``test-eventdev``;
>>>>> +  - ``examples``, e.g.: ``ipsec-secgw``, ``l3fwd``, ``vhost``;
>>>>> +  - net PMDs which support VLAN tag(s) within overhead, e.g.:
>>>>> +``i40e``;
>>>>> +
>>>>>    * cryptodev: support for using IV with all sizes is added, J0
>>>>> still can
>>>>>      be used but only when IV length in following structs
>>>>> ``rte_crypto_auth_xform``,
>>>>>      ``rte_crypto_aead_xform`` is set to zero. When IV length is
>>>>> greater or equal
>>>>>
>>>>
>>>> If so, what's the point to have the offload? May be just deprecate
>>>> and later remove it?
>>>>
>>
>> Same thought actually, can we remove DEV_RX_OFFLOAD_JUMBO_FRAME
>> flag completely?
>> PMD can decide based on provided MTU size does segmentation, etc. is
>> needed.
>>
> 
> Yes, it seems can be removed when base on MTU size.
> I've checked related code of using DEV_RX_OFFLOAD_JUMBO_FRAME.
> Most of them use for checking boundary of max packet length and set 'dev->data->mtu'.
> 

Steve already sent the RFC for above fix:
https://patches.dpdk.org/patch/84486/

We can consider removing 'DEV_RX_OFFLOAD_JUMBO_FRAME' but anyway it is for 21.11.

This deprecation notice is required to fix the ethdev in next release, as in the 
above RFC.

I cc'ed a few more relevant people, can you please review this deprecation notice.

Thanks,
ferruh


>>>
>>> Above just changes the condition of what is called jumbo frame, nothing
>> more.
>>>
>>> ethdev assumes max frame size (without jumbo frame support) can be
>>> 'RTE_ETHER_MAX_LEN' (1518)
>>>
>>> But a PMD can support double VLAN, and it can have
>> RTE_ETHER_MAX_LEN +
>>> 8 bytes frame size and still may not support jumbo frame.
>>>
>>> In that case ethdev overwrites the frame size as RTE_ETHER_MAX_LEN,
>>> and this will reduce the supported MTU to 1492 (instead of expected 1500).
>>> Above deprecation notice is to fix this.
Andrew Rybchenko Nov. 27, 2020, 12:19 p.m. UTC | #6 
On 11/24/20 8:46 PM, Ferruh Yigit wrote:
> On 11/3/2020 9:07 AM, Yang, SteveX wrote:
>>> -----Original Message-----
>>> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>>> Sent: Tuesday, November 3, 2020 12:05 AM
>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko
>>> <andrew.rybchenko@oktetlabs.ru>; Yang, SteveX <stevex.yang@intel.com>;
>>> dev@dpdk.org
>>> Cc: 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
>>> Subject: RE: [dpdk-dev] [PATCH v8 2/2] doc: annouce deprecation of jumbo
>>> frame flag condition
>>>
>>>> On 11/2/2020 1:18 PM, Andrew Rybchenko wrote:
>>>>> On 11/2/20 11:52 AM, SteveX Yang wrote:
>>>>>> Annouce to replace 'RTE_ETHER_MAX_LEN' with 'RTE_ETHER_MTU' as type
>>>>>> condition of jumbo frame. Involved scopes:
>>>>>> - rte_ethdev;
>>>>>> - app, e.g.: test-pmd, test-eventdev;
>>>>>> - examples, e.g.: ipsec-secgw, l3fwd, vhost;
>>>>>> - net PMDs which support VLAN tag(s) within overhead, e.g.: i40e,
>>>>>> ixgbe;
>>>>>>
>>>>>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
>>>>>> ---
>>>>>>    doc/guides/rel_notes/deprecation.rst | 12 ++++++++++++
>>>>>>    1 file changed, 12 insertions(+)
>>>>>>
>>>>>> diff --git a/doc/guides/rel_notes/deprecation.rst
>>>>>> b/doc/guides/rel_notes/deprecation.rst
>>>>>> index 2e082499b..fae139f01 100644
>>>>>> --- a/doc/guides/rel_notes/deprecation.rst
>>>>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>>>>> @@ -138,6 +138,18 @@ Deprecation Notices
>>>>>>      will be limited to maximum 256 queues.
>>>>>>      Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS`` will be removed.
>>>>>> +* ethdev: Offload flag ``DEV_RX_OFFLOAD_JUMBO_FRAME`` will be set
>>>>>> +according to
>>>>>> +  ``RTE_ETHER_MTU`` in next release. Currently, the jumbo frame
>>>>>> +uses the
>>>>>> +  ``RTE_ETHER_MAX_LEN`` as boundary condition. When the MTU (1500)
>>>>>> +set, the
>>>>>> +  frame type of rx packet will be different if used different
>>>>>> +overhead, it will
>>>>>> +  cause the consistency issue. Hence, using fixed value
>>>>>> +``RTE_ETHER_MTU`` can
>>>>>> +  avoid this issue.
>>>>>> +  Following scopes will be changed:
>>>>>> +  - ``rte_ethdev``
>>>>>> +  - ``app``, e.g.: ``test-pmd``, ``test-eventdev``;
>>>>>> +  - ``examples``, e.g.: ``ipsec-secgw``, ``l3fwd``, ``vhost``;
>>>>>> +  - net PMDs which support VLAN tag(s) within overhead, e.g.:
>>>>>> +``i40e``;
>>>>>> +
>>>>>>    * cryptodev: support for using IV with all sizes is added, J0
>>>>>> still can
>>>>>>      be used but only when IV length in following structs
>>>>>> ``rte_crypto_auth_xform``,
>>>>>>      ``rte_crypto_aead_xform`` is set to zero. When IV length is
>>>>>> greater or equal
>>>>>>
>>>>>
>>>>> If so, what's the point to have the offload? May be just deprecate
>>>>> and later remove it?
>>>>>
>>>
>>> Same thought actually, can we remove DEV_RX_OFFLOAD_JUMBO_FRAME
>>> flag completely?
>>> PMD can decide based on provided MTU size does segmentation, etc. is
>>> needed.
>>>
>>
>> Yes, it seems can be removed when base on MTU size.
>> I've checked related code of using DEV_RX_OFFLOAD_JUMBO_FRAME.
>> Most of them use for checking boundary of max packet length and set
>> 'dev->data->mtu'.
>>
> 
> Steve already sent the RFC for above fix:
> https://patches.dpdk.org/patch/84486/
> 
> We can consider removing 'DEV_RX_OFFLOAD_JUMBO_FRAME' but anyway it is
> for 21.11.
> 
> This deprecation notice is required to fix the ethdev in next release,
> as in the above RFC.
> 
> I cc'ed a few more relevant people, can you please review this
> deprecation notice.
> 
> Thanks,
> ferruh
> 
> 
>>>>
>>>> Above just changes the condition of what is called jumbo frame, nothing more.
>>>>
>>>> ethdev assumes max frame size (without jumbo frame support) can be
>>>> 'RTE_ETHER_MAX_LEN' (1518)
>>>>
>>>> But a PMD can support double VLAN, and it can have RTE_ETHER_MAX_LEN +
>>>> 8 bytes frame size and still may not support jumbo frame.
>>>>
>>>> In that case ethdev overwrites the frame size as RTE_ETHER_MAX_LEN,
>>>> and this will reduce the supported MTU to 1492 (instead of expected
>>>> 1500).
>>>> Above deprecation notice is to fix this.

My problem with the deprecation notice is that I don't actually
understand what will be done to address it.

The idea explained by Ferruh in details makes sense. But I
don't understand how the deprecation notice description
corresponding to it. I read
  "Offload flag ``DEV_RX_OFFLOAD_JUMBO_FRAME`` will be set.."
as an enforcement of the offload flag based on other
parameters. I think it is incorrect. Or I still don't
understand something...

Looking at [1] adds more confusion since I don't understand why
we care about dev_conf->rxmode.max_rx_pkt_len when JUMBO_FRAME
offload is disabled. As far as I know JUMBO_FRAME offload
enable means that driver should take a look at it and apply.
Otherwise, just ignore it.

[1]
http://lxr.dpdk.org/dpdk/v20.08/source/lib/librte_ethdev/rte_ethdev.c#L1345
Bruce Richardson Nov. 27, 2020, 5:08 p.m. UTC | #7 
On Fri, Nov 27, 2020 at 03:19:43PM +0300, Andrew Rybchenko wrote:
> On 11/24/20 8:46 PM, Ferruh Yigit wrote:
> > On 11/3/2020 9:07 AM, Yang, SteveX wrote:
> >>> -----Original Message-----
> >>> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> >>> Sent: Tuesday, November 3, 2020 12:05 AM
> >>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko
> >>> <andrew.rybchenko@oktetlabs.ru>; Yang, SteveX <stevex.yang@intel.com>;
> >>> dev@dpdk.org
> >>> Cc: 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
> >>> Subject: RE: [dpdk-dev] [PATCH v8 2/2] doc: annouce deprecation of jumbo
> >>> frame flag condition
> >>>
> >>>> On 11/2/2020 1:18 PM, Andrew Rybchenko wrote:
> >>>>> On 11/2/20 11:52 AM, SteveX Yang wrote:
> >>>>>> Annouce to replace 'RTE_ETHER_MAX_LEN' with 'RTE_ETHER_MTU' as type
> >>>>>> condition of jumbo frame. Involved scopes:
> >>>>>> - rte_ethdev;
> >>>>>> - app, e.g.: test-pmd, test-eventdev;
> >>>>>> - examples, e.g.: ipsec-secgw, l3fwd, vhost;
> >>>>>> - net PMDs which support VLAN tag(s) within overhead, e.g.: i40e,
> >>>>>> ixgbe;
> >>>>>>
> >>>>>> Signed-off-by: SteveX Yang <stevex.yang@intel.com>
> >>>>>> ---
> >>>>>>    doc/guides/rel_notes/deprecation.rst | 12 ++++++++++++
> >>>>>>    1 file changed, 12 insertions(+)
> >>>>>>
> >>>>>> diff --git a/doc/guides/rel_notes/deprecation.rst
> >>>>>> b/doc/guides/rel_notes/deprecation.rst
> >>>>>> index 2e082499b..fae139f01 100644
> >>>>>> --- a/doc/guides/rel_notes/deprecation.rst
> >>>>>> +++ b/doc/guides/rel_notes/deprecation.rst
> >>>>>> @@ -138,6 +138,18 @@ Deprecation Notices
> >>>>>>      will be limited to maximum 256 queues.
> >>>>>>      Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS`` will be removed.
> >>>>>> +* ethdev: Offload flag ``DEV_RX_OFFLOAD_JUMBO_FRAME`` will be set
> >>>>>> +according to
> >>>>>> +  ``RTE_ETHER_MTU`` in next release. Currently, the jumbo frame
> >>>>>> +uses the
> >>>>>> +  ``RTE_ETHER_MAX_LEN`` as boundary condition. When the MTU (1500)
> >>>>>> +set, the
> >>>>>> +  frame type of rx packet will be different if used different
> >>>>>> +overhead, it will
> >>>>>> +  cause the consistency issue. Hence, using fixed value
> >>>>>> +``RTE_ETHER_MTU`` can
> >>>>>> +  avoid this issue.
> >>>>>> +  Following scopes will be changed:
> >>>>>> +  - ``rte_ethdev``
> >>>>>> +  - ``app``, e.g.: ``test-pmd``, ``test-eventdev``;
> >>>>>> +  - ``examples``, e.g.: ``ipsec-secgw``, ``l3fwd``, ``vhost``;
> >>>>>> +  - net PMDs which support VLAN tag(s) within overhead, e.g.:
> >>>>>> +``i40e``;
> >>>>>> +
> >>>>>>    * cryptodev: support for using IV with all sizes is added, J0
> >>>>>> still can
> >>>>>>      be used but only when IV length in following structs
> >>>>>> ``rte_crypto_auth_xform``,
> >>>>>>      ``rte_crypto_aead_xform`` is set to zero. When IV length is
> >>>>>> greater or equal
> >>>>>>
> >>>>>
> >>>>> If so, what's the point to have the offload? May be just deprecate
> >>>>> and later remove it?
> >>>>>
> >>>
> >>> Same thought actually, can we remove DEV_RX_OFFLOAD_JUMBO_FRAME
> >>> flag completely?
> >>> PMD can decide based on provided MTU size does segmentation, etc. is
> >>> needed.
> >>>
> >>
> >> Yes, it seems can be removed when base on MTU size.
> >> I've checked related code of using DEV_RX_OFFLOAD_JUMBO_FRAME.
> >> Most of them use for checking boundary of max packet length and set
> >> 'dev->data->mtu'.
> >>
> > 
> > Steve already sent the RFC for above fix:
> > https://patches.dpdk.org/patch/84486/
> > 
> > We can consider removing 'DEV_RX_OFFLOAD_JUMBO_FRAME' but anyway it is
> > for 21.11.
> > 
> > This deprecation notice is required to fix the ethdev in next release,
> > as in the above RFC.
> > 
> > I cc'ed a few more relevant people, can you please review this
> > deprecation notice.
> > 
> > Thanks,
> > ferruh
> > 
> > 
> >>>>
> >>>> Above just changes the condition of what is called jumbo frame, nothing more.
> >>>>
> >>>> ethdev assumes max frame size (without jumbo frame support) can be
> >>>> 'RTE_ETHER_MAX_LEN' (1518)
> >>>>
> >>>> But a PMD can support double VLAN, and it can have RTE_ETHER_MAX_LEN +
> >>>> 8 bytes frame size and still may not support jumbo frame.
> >>>>
> >>>> In that case ethdev overwrites the frame size as RTE_ETHER_MAX_LEN,
> >>>> and this will reduce the supported MTU to 1492 (instead of expected
> >>>> 1500).
> >>>> Above deprecation notice is to fix this.
> 
> My problem with the deprecation notice is that I don't actually
> understand what will be done to address it.
> 
> The idea explained by Ferruh in details makes sense. But I
> don't understand how the deprecation notice description
> corresponding to it. I read
>   "Offload flag ``DEV_RX_OFFLOAD_JUMBO_FRAME`` will be set.."
> as an enforcement of the offload flag based on other
> parameters. I think it is incorrect. Or I still don't
> understand something...
> 
> Looking at [1] adds more confusion since I don't understand why
> we care about dev_conf->rxmode.max_rx_pkt_len when JUMBO_FRAME
> offload is disabled. As far as I know JUMBO_FRAME offload
> enable means that driver should take a look at it and apply.
> Otherwise, just ignore it.
> 

I agree with the comment here - my understanding is the same that if the
JUMBO_FRAME offload flag is not set, then the max_rx_pkt_len should be
ignored (which for me implies that it should be set to 0 or similar
sentinal value in ethdev to ensure drivers don't accidentally use it).

In terms of the deprecation notice, I also think it's fairly confusing, and
after talking with Ferruh, I'm not convinced we need one. It seems that the
planned changes based on this are just bug fixes, where packets that
should not have been dropped were dropped. Perhaps someone could comment on
the specific behaviour change that would affect apps (where it's not just
plain buggy behaviour!)

However, it does appear that this area is in need of clean-up generally.
The suggestion to drop the jumbo frame flag, packet_len/mtu value from the
ethdev config, and just use the existing API calls, sounds interesting. If
that is not the approach taken, I'd like to see the existing approach kept,
so that a zero-initialized config is acceptable for packet size setting,
i.e. no jumbo frame flag and zero max-length == default ethernet MTU.

Just my 2c.
/Bruce
diff mbox series

Patch

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 2e082499b..fae139f01 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -138,6 +138,18 @@  Deprecation Notices
   will be limited to maximum 256 queues.
   Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS`` will be removed.
 
+* ethdev: Offload flag ``DEV_RX_OFFLOAD_JUMBO_FRAME`` will be set according to
+  ``RTE_ETHER_MTU`` in next release. Currently, the jumbo frame uses the
+  ``RTE_ETHER_MAX_LEN`` as boundary condition. When the MTU (1500) set, the
+  frame type of rx packet will be different if used different overhead, it will
+  cause the consistency issue. Hence, using fixed value ``RTE_ETHER_MTU`` can
+  avoid this issue.
+  Following scopes will be changed:
+  - ``rte_ethdev``
+  - ``app``, e.g.: ``test-pmd``, ``test-eventdev``;
+  - ``examples``, e.g.: ``ipsec-secgw``, ``l3fwd``, ``vhost``;
+  - net PMDs which support VLAN tag(s) within overhead, e.g.: ``i40e``;
+
 * cryptodev: support for using IV with all sizes is added, J0 still can
   be used but only when IV length in following structs ``rte_crypto_auth_xform``,
   ``rte_crypto_aead_xform`` is set to zero. When IV length is greater or equal