Re: [dpdk-dev] [RFC 0/3] ethdev: add ptype as Rx offload

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 8/7/19 6:22 PM, Stephen Hemminger wrote:
> On Wed, 7 Aug 2019 11:32:35 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> 
>> On 8/7/19 5:04 AM, Jerin Jacob Kollanukkaran wrote:
>>>> -----Original Message-----
>>>> From: Stephen Hemminger <stephen@networkplumber.org>
>>>> Sent: Wednesday, August 7, 2019 4:45 AM
>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> Cc: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Hemant
>>>> Agrawal <hemant.agrawal@nxp.com>; Jerin Jacob Kollanukkaran
>>>> <jerinj@marvell.com>; dev@dpdk.org
>>>> Subject: [EXT] Re: [dpdk-dev] [RFC 0/3] ethdev: add ptype as Rx offload
>>>>
>>>> On Tue, 6 Aug 2019 12:06:35 +0300
>>>> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>>>>   
>>>>> On 8/6/19 11:47 AM, Pavan Nikhilesh Bhagavatula wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Hemant Agrawal <hemant.agrawal@nxp.com>
>>>>>>> Sent: Tuesday, August 6, 2019 1:49 PM
>>>>>>> To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Jerin
>>>>>>> Jacob Kollanukkaran <jerinj@marvell.com>
>>>>>>> Cc: dev@dpdk.org
>>>>>>> Subject: RE: [dpdk-dev] [RFC 0/3] ethdev: add ptype as Rx offload
>>>>>>>> Add PTYPE to DEV_RX_OFFLOAD_* flags.
>>>>>>>>
>>>>>>>> Currently, most of the NICs already support PTYPE parsing and
>>>>>>>> update
>>>>>>> the
>>>>>>>> mbuf->packet_type through an internal lookup table, but there is
>>>>>>>> mbuf->no
>>>>>>> way to
>>>>>>>> disable the lookup if the application is not intrested in ptypes
>>>>>>> returned by
>>>>>>>> `rte_eth_dev_get_supported_ptypes`.
>>>>>>>>   
>>>>>>> [Hemant]  it will also mean introducing another check in datapath,
>>>>>>> if the application has asked for PTYPE offload - copy the results
>>>>>>> to mbuf-
>>>>>>>> packet_type otherwise don't do it.
>>>>>> I think that having the check would give better performance than
>>>>>> loading ptype table to L1 doing  a lookup and copying it to mbuf when the
>>>> application doesn't need it.
>>>>> Anyway, if PMD decides that it is better to always provide packet type
>>>>> information - there is no harm. Basically if the offload is not
>>>>> requested it makes packet_type undefined in mbuf.
>>>>>   
>>>>>>> Your second patch is incomplete in the sense that it only adds the
>>>>>>> capability. But it does not disable the lookups?
>>>>>> It is upto the maintainer of the PMD to disable the lookup in data
>>>>>> path. If there is a scope of optimization then they could do it. There is no
>>>> harm in exposing  PTYPE even RX_OFFLOAD_PTYPE is not enabled.
>>>>>> I was hesitant to touch data path as it would be impossible to verify
>>>> performance effect on all NICs.
>>>>> I think it is the right way to approach it especially taking
>>>>> transition into account.
>>>>>   
>>>> With hardline API policy, this has to fail on compile for old applications.
>>
>> Stephen, could you explain a bit more why.
> 
> Existing releases packets will be received with ptype for hardware that
> supports it. We should not require users to change their application to
> continue to get mbufs with ptype.  If your change would break that, and
> require application to change; then your change should break the API in
> a hard way that causes compile rather than runtime failure.

Many thanks, I got it.

> The best solution would be to just keep old applications running and compiling
> without breaking anything. That means ptype should still be received.
> 
> If (as an optimization) you want to allow application to turn of getting
> ptype; then that would be a useful. Probably best done at the port level
> as part of configuration.

I see, but it contradicts to the existing practice that offloads should
be disabled by default and a way to enable should be provided.
May be techboard should discuss it and make a decision (covering RSS
hash information and Rx mark mentioned in my review notes).

>>> Not specific to this API change. That's is the propriety any new symbol addition
>>> to the code base.
>>>
>>> Planning to make this API change available fromv19.11 LTS.
>>
>> The only way to to require applications to enable PTYPE offload to get
>> ptypes in mbuf since 19.11 LTS is to have deprecation notice in 19.08.
>>
>>>> You can't magically assume that applications using ptype will set new feature.
>>> When OFFLOAD flags got introduced, we decided to disable all offloads by default.
>>> So, need to add positive logic here to enable offload instead of enable something by
>>> Default and disable if required to get have synergy with other offloads.
>>>
>>> Will update the release note as usual to document the change.
>>> Since there is NO ABI change, IMO, we don't need deprecation notice.
>>
>> Sorry, but it is a behaviour change. Before an application does not need
>> to enable ptype offload, but now it is required. It means that application
>> will be broken and, therefore, it requires deprecation notice.
> 
> The DPDK development community has to make not breaking applications
> a higher priority than adding marginal enhancements

Fair, but where is marginal enhancements boundary?