[RFC,1/3] ethdev: add ptype as an Rx offload

Message ID 20190806080206.1572-2-pbhagavatula@marvell.com (mailing list archive)
State Not Applicable, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: add ptype as Rx offload |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Pavan Nikhilesh Bhagavatula Aug. 6, 2019, 8:02 a.m. UTC
  From: Pavan Nikhilesh <pbhagavatula@marvell.com>

Add ptype to DEV_RX_OFFLOAD_* flags which can be used to enable/disable
packet type parsing.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 doc/guides/nics/features.rst   | 3 +++
 lib/librte_ethdev/rte_ethdev.h | 1 +
 2 files changed, 4 insertions(+)
  

Comments

Andrew Rybchenko Aug. 6, 2019, 9 a.m. UTC | #1
On 8/6/19 11:02 AM, pbhagavatula@marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
> Add ptype to DEV_RX_OFFLOAD_* flags which can be used to enable/disable
> packet type parsing.
>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>

I like the idea. I think there are few more Rx features which
lack Rx offload bit:
  - delivery of RSS hash in mbuf (it is not always required when
    RSS is used to distribute packets across Rx queues)
  - maybe Rx mark, since it is an extra information which could
    be passed by NIC to CPU and it is better to know in advance
    at Rx queue setup if it should be requested and processed

API breakage should be considered here. I think it is OK to
introduce it in the next release cycle in a dummy way which
does not affect packet type delivery for existing PMDs
(i.e. add offload capability and advertise in PMD, but do not
take it into account when Rx mbuf is filled in) and
submit deprecation notice that it may be taken into account
by PMDs in 20.02 to avoid packet type delivery if the offload
is not requested. It will allow applications to make transition
smoother.

Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
  
Pavan Nikhilesh Bhagavatula Aug. 6, 2019, 2:31 p.m. UTC | #2
>-----Original Message-----
>From: Andrew Rybchenko <arybchenko@solarflare.com>
>Sent: Tuesday, August 6, 2019 2:30 PM
>To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Jerin
>Jacob Kollanukkaran <jerinj@marvell.com>; John McNamara
><john.mcnamara@intel.com>; Marko Kovacevic
><marko.kovacevic@intel.com>; Thomas Monjalon
><thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
>Cc: dev@dpdk.org
>Subject: [EXT] Re: [dpdk-dev] [RFC 1/3] ethdev: add ptype as an Rx
>offload
>
>External Email
>
>----------------------------------------------------------------------
>On 8/6/19 11:02 AM, pbhagavatula@marvell.com wrote:
>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>
>> Add ptype to DEV_RX_OFFLOAD_* flags which can be used to
>enable/disable
>> packet type parsing.
>>
>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
>I like the idea. I think there are few more Rx features which
>lack Rx offload bit:
>  - delivery of RSS hash in mbuf (it is not always required when
>    RSS is used to distribute packets across Rx queues)

Especially when applications use custom hash functions to store flows.

>  - maybe Rx mark, since it is an extra information which could
>    be passed by NIC to CPU and it is better to know in advance
>    at Rx queue setup if it should be requested and processed

Are you referring to RTE_FLOW_ACTION_TYPE_MARK?

>
>API breakage should be considered here. I think it is OK to
>introduce it in the next release cycle in a dummy way which
>does not affect packet type delivery for existing PMDs
>(i.e. add offload capability and advertise in PMD, but do not
>take it into account when Rx mbuf is filled in) and
>submit deprecation notice that it may be taken into account
>by PMDs in 20.02 to avoid packet type delivery if the offload
>is not requested. It will allow applications to make transition
>smoother.

Couldn’t agree with you more. I could extend the current RFC to include 
RSS and RX mark as we would be modifying the same offload fields across 
all drivers. Easier for PMD maintainers too.

>
>Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
  
Stephen Hemminger Aug. 6, 2019, 3:45 p.m. UTC | #3
On Tue, 6 Aug 2019 14:31:43 +0000
Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> wrote:

> >-----Original Message-----
> >From: Andrew Rybchenko <arybchenko@solarflare.com>
> >Sent: Tuesday, August 6, 2019 2:30 PM
> >To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Jerin
> >Jacob Kollanukkaran <jerinj@marvell.com>; John McNamara
> ><john.mcnamara@intel.com>; Marko Kovacevic
> ><marko.kovacevic@intel.com>; Thomas Monjalon
> ><thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
> >Cc: dev@dpdk.org
> >Subject: [EXT] Re: [dpdk-dev] [RFC 1/3] ethdev: add ptype as an Rx
> >offload
> >
> >External Email
> >
> >----------------------------------------------------------------------
> >On 8/6/19 11:02 AM, pbhagavatula@marvell.com wrote:  
> >> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >>
> >> Add ptype to DEV_RX_OFFLOAD_* flags which can be used to  
> >enable/disable  
> >> packet type parsing.
> >>
> >> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>  
> >
> >I like the idea. I think there are few more Rx features which
> >lack Rx offload bit:
> >  - delivery of RSS hash in mbuf (it is not always required when
> >    RSS is used to distribute packets across Rx queues)  
> 
> Especially when applications use custom hash functions to store flows.
> 
> >  - maybe Rx mark, since it is an extra information which could
> >    be passed by NIC to CPU and it is better to know in advance
> >    at Rx queue setup if it should be requested and processed  
> 
> Are you referring to RTE_FLOW_ACTION_TYPE_MARK?
> 
> >
> >API breakage should be considered here. I think it is OK to
> >introduce it in the next release cycle in a dummy way which
> >does not affect packet type delivery for existing PMDs
> >(i.e. add offload capability and advertise in PMD, but do not
> >take it into account when Rx mbuf is filled in) and
> >submit deprecation notice that it may be taken into account
> >by PMDs in 20.02 to avoid packet type delivery if the offload
> >is not requested. It will allow applications to make transition
> >smoother.  
> 
> Couldn’t agree with you more. I could extend the current RFC to include 
> RSS and RX mark as we would be modifying the same offload fields across 
> all drivers. Easier for PMD maintainers too.
> 
> >
> >Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>  
> 

I would rather the ptype offload be always on and handled in software
for drivers that don't do it.
  
Andrew Rybchenko Aug. 6, 2019, 6:03 p.m. UTC | #4
On 8/6/19 6:45 PM, Stephen Hemminger wrote:
> On Tue, 6 Aug 2019 14:31:43 +0000
> Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> wrote:
>
>>> -----Original Message-----
>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>> Sent: Tuesday, August 6, 2019 2:30 PM
>>> To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Jerin
>>> Jacob Kollanukkaran <jerinj@marvell.com>; John McNamara
>>> <john.mcnamara@intel.com>; Marko Kovacevic
>>> <marko.kovacevic@intel.com>; Thomas Monjalon
>>> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
>>> Cc: dev@dpdk.org
>>> Subject: [EXT] Re: [dpdk-dev] [RFC 1/3] ethdev: add ptype as an Rx
>>> offload
>>>
>>> External Email
>>>
>>> ----------------------------------------------------------------------
>>> On 8/6/19 11:02 AM, pbhagavatula@marvell.com wrote:
>>>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>>>
>>>> Add ptype to DEV_RX_OFFLOAD_* flags which can be used to
>>> enable/disable
>>>> packet type parsing.
>>>>
>>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>> I like the idea. I think there are few more Rx features which
>>> lack Rx offload bit:
>>>   - delivery of RSS hash in mbuf (it is not always required when
>>>     RSS is used to distribute packets across Rx queues)
>> Especially when applications use custom hash functions to store flows.
>>
>>>   - maybe Rx mark, since it is an extra information which could
>>>     be passed by NIC to CPU and it is better to know in advance
>>>     at Rx queue setup if it should be requested and processed
>> Are you referring to RTE_FLOW_ACTION_TYPE_MARK?
>>
>>> API breakage should be considered here. I think it is OK to
>>> introduce it in the next release cycle in a dummy way which
>>> does not affect packet type delivery for existing PMDs
>>> (i.e. add offload capability and advertise in PMD, but do not
>>> take it into account when Rx mbuf is filled in) and
>>> submit deprecation notice that it may be taken into account
>>> by PMDs in 20.02 to avoid packet type delivery if the offload
>>> is not requested. It will allow applications to make transition
>>> smoother.
>> Couldn’t agree with you more. I could extend the current RFC to include
>> RSS and RX mark as we would be modifying the same offload fields across
>> all drivers. Easier for PMD maintainers too.
>>
>>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> I would rather the ptype offload be always on and handled in software
> for drivers that don't do it.

It sounds like wasting of CPU cycle for nothing in some cases.
Also where should software stop? There are various tunnels etc.
If application is unhappy with supported classification provided
by the driver, it can always use software parser if really required.
  

Patch

diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
index 6f8cac2c8..6b222b270 100644
--- a/doc/guides/nics/features.rst
+++ b/doc/guides/nics/features.rst
@@ -662,6 +662,9 @@  Packet type parsing
 
 Supports packet type parsing and returns a list of supported types.
 
+* **[uses]       rte_eth_rxconf,rte_eth_rxmpde**: ``offloads:DEV_RX_OFFLOAD_PTYPE``.
+* **[provides]   mbuf**: ``mbuf.packet_type``.
+* **[provides]   rte_eth_dev_info**: ``rx_offload_capa,rx_queue_offload_capa:DEV_RX_OFFLOAD_PTYPE``.
 * **[implements] eth_dev_ops**: ``dev_supported_ptypes_get``.
 * **[related]    API**: ``rte_eth_dev_get_supported_ptypes()``.
 
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index dc6596bc9..888b766d4 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1013,6 +1013,7 @@  struct rte_eth_conf {
 #define DEV_RX_OFFLOAD_KEEP_CRC		0x00010000
 #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
 #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
+#define DEV_RX_OFFLOAD_PTYPE		0x00080000
 
 #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
 				 DEV_RX_OFFLOAD_UDP_CKSUM | \