ethdev: add rx offload to drop error packets

Message ID 20200831075333.10135-1-nipun.gupta@nxp.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: add rx offload to drop error packets |

Checks

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

Commit Message

Nipun Gupta Aug. 31, 2020, 7:53 a.m. UTC
  This change adds a RX offload capability where hardware can drop the
packets in case there is an error in the packet such as L3 checksum
error or L4 checksum.

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
---
 lib/librte_ethdev/rte_ethdev.h | 1 +
 1 file changed, 1 insertion(+)
  

Comments

Ferruh Yigit Aug. 31, 2020, 12:58 p.m. UTC | #1
On 8/31/2020 8:53 AM, Nipun Gupta wrote:
> This change adds a RX offload capability where hardware can drop the
> packets in case there is an error in the packet such as L3 checksum
> error or L4 checksum.
> 
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index a49242bcd..bb9df2fb9 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1120,6 +1120,7 @@ struct rte_eth_conf {
>  #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
>  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
>  #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
>  
>  #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
>  				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> 


Hi Nipun,

You adding a new offload type for network devices, it should have some more
description and relevant PMD/testpmd updates too.

But before that, is dropping faulty packets an offload or a config option for
the NIC? Or is this common for all NICs to have in ethdev?
  
Nipun Gupta Aug. 31, 2020, 4:04 p.m. UTC | #2
Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, August 31, 2020 6:29 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; arybchenko@solarflare.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
> Subject: Re: [PATCH] ethdev: add rx offload to drop error packets
> 
> On 8/31/2020 8:53 AM, Nipun Gupta wrote:
> > This change adds a RX offload capability where hardware can drop the
> > packets in case there is an error in the packet such as L3 checksum
> > error or L4 checksum.
> >
> > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > ---
> >  lib/librte_ethdev/rte_ethdev.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > index a49242bcd..bb9df2fb9 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -1120,6 +1120,7 @@ struct rte_eth_conf {
> >  #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
> >  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
> >  #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> > +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
> >
> >  #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
> >  				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> >
> 
> 
> Hi Nipun,
> 
> You adding a new offload type for network devices, it should have some more
> description and relevant PMD/testpmd updates too.

I should have added an RFC tag in the patch. I sent this for comment for error packet drop
offload support. I will send patches after proper implementation.

> 
> But before that, is dropping faulty packets an offload or a config option for
> the NIC? Or is this common for all NICs to have in ethdev?

We would like to have it as an offload capability, and on the basis of this capability
user can configure the Ethernet device to drop the error packets on the device itself
without providing them to the core.
Sorry I did not get your comment "Or is this common for all NICs to have in ethdev?"

Thanks,
Nipun
  
Stephen Hemminger Aug. 31, 2020, 5 p.m. UTC | #3
On Mon, 31 Aug 2020 13:23:33 +0530
Nipun Gupta <nipun.gupta@nxp.com> wrote:

> This change adds a RX offload capability where hardware can drop the
> packets in case there is an error in the packet such as L3 checksum
> error or L4 checksum.
> 
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index a49242bcd..bb9df2fb9 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1120,6 +1120,7 @@ struct rte_eth_conf {
>  #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
>  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
>  #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
>  
>  #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
>  				 DEV_RX_OFFLOAD_UDP_CKSUM | \

Could/should this be an rte_flow action as well?
  
Thomas Monjalon Sept. 1, 2020, 8:09 a.m. UTC | #4
31/08/2020 19:00, Stephen Hemminger:
> On Mon, 31 Aug 2020 13:23:33 +0530
> Nipun Gupta <nipun.gupta@nxp.com> wrote:
> 
> > This change adds a RX offload capability where hardware can drop the
> > packets in case there is an error in the packet such as L3 checksum
> > error or L4 checksum.
> > 
> > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > ---
> > +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000

Please add RTE_ prefix, even if older macros don't have it.
We could (in a separate effort) alias old ones with RTE_ prefixed names.

> Could/should this be an rte_flow action as well?

I feel rte_flow API is not appropriate here.
Ori, any opinion?
  
Nipun Gupta Sept. 1, 2020, 10:56 a.m. UTC | #5
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, September 1, 2020 1:39 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>; Stephen Hemminger
> <stephen@networkplumber.org>; orika@mellanox.com
> Cc: dev@dpdk.org; ferruh.yigit@intel.com; arybchenko@solarflare.com;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> olivier.matz@6wind.com
> Subject: Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets
> 
> 31/08/2020 19:00, Stephen Hemminger:
> > On Mon, 31 Aug 2020 13:23:33 +0530
> > Nipun Gupta <nipun.gupta@nxp.com> wrote:
> >
> > > This change adds a RX offload capability where hardware can drop the
> > > packets in case there is an error in the packet such as L3 checksum
> > > error or L4 checksum.
> > >
> > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > ---
> > > +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
> 
> Please add RTE_ prefix, even if older macros don't have it.
> We could (in a separate effort) alias old ones with RTE_ prefixed names.

Agree, will update and send the change along with testpmd and dpaa2 driver change.

> 
> > Could/should this be an rte_flow action as well?
> 
> I feel rte_flow API is not appropriate here.
> Ori, any opinion?
>
  
Ori Kam Sept. 21, 2020, 7:29 a.m. UTC | #6
Hi

> -----Original Message-----
> Subject: Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets
> 
> 31/08/2020 19:00, Stephen Hemminger:
> > On Mon, 31 Aug 2020 13:23:33 +0530
> > Nipun Gupta <nipun.gupta@nxp.com> wrote:
> >
> > > This change adds a RX offload capability where hardware can drop the
> > > packets in case there is an error in the packet such as L3 checksum
> > > error or L4 checksum.
> > >
> > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > ---
> > > +#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
> 
> Please add RTE_ prefix, even if older macros don't have it.
> We could (in a separate effort) alias old ones with RTE_ prefixed names.
> 
> > Could/should this be an rte_flow action as well?
> 
> I feel rte_flow API is not appropriate here.
> Ori, any opinion?
> 
I also don't think this is relevant action in case of rte_flow,
If we want to support such a thing in rte_flow we should 
add item that matches on error and then use the drop action.

Best,
Ori
  

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index a49242bcd..bb9df2fb9 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1120,6 +1120,7 @@  struct rte_eth_conf {
 #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
 #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
 #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
+#define DEV_RX_OFFLOAD_ERR_PKT_DROP	0x00100000
 
 #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
 				 DEV_RX_OFFLOAD_UDP_CKSUM | \