DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets
@ 2020-08-31  7:53 Nipun Gupta
  2020-08-31 12:58 ` Ferruh Yigit
                   ` (4 more replies)
  0 siblings, 5 replies; 46+ messages in thread
From: Nipun Gupta @ 2020-08-31  7:53 UTC (permalink / raw)
  To: dev
  Cc: thomas, ferruh.yigit, arybchenko, hemant.agrawal, Nipun Gupta, Rohit Raj

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 | \
-- 
2.17.1


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets
  2020-08-31  7:53 [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets Nipun Gupta
@ 2020-08-31 12:58 ` Ferruh Yigit
  2020-08-31 16:04   ` Nipun Gupta
  2020-08-31 17:00 ` Stephen Hemminger
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 46+ messages in thread
From: Ferruh Yigit @ 2020-08-31 12:58 UTC (permalink / raw)
  To: Nipun Gupta, dev; +Cc: thomas, arybchenko, hemant.agrawal, Rohit Raj

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?

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets
  2020-08-31 12:58 ` Ferruh Yigit
@ 2020-08-31 16:04   ` Nipun Gupta
  0 siblings, 0 replies; 46+ messages in thread
From: Nipun Gupta @ 2020-08-31 16:04 UTC (permalink / raw)
  To: Ferruh Yigit, dev; +Cc: thomas, arybchenko, Hemant Agrawal, Rohit Raj

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets
  2020-08-31  7:53 [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets Nipun Gupta
  2020-08-31 12:58 ` Ferruh Yigit
@ 2020-08-31 17:00 ` Stephen Hemminger
  2020-09-01  8:09   ` Thomas Monjalon
  2020-10-05  7:15 ` [dpdk-dev] [PATCH 1/3 v2] " nipun.gupta
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 46+ messages in thread
From: Stephen Hemminger @ 2020-08-31 17:00 UTC (permalink / raw)
  To: Nipun Gupta
  Cc: dev, thomas, ferruh.yigit, arybchenko, hemant.agrawal, Rohit Raj

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?

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets
  2020-08-31 17:00 ` Stephen Hemminger
@ 2020-09-01  8:09   ` Thomas Monjalon
  2020-09-01 10:56     ` Nipun Gupta
  2020-09-21  7:29     ` Ori Kam
  0 siblings, 2 replies; 46+ messages in thread
From: Thomas Monjalon @ 2020-09-01  8:09 UTC (permalink / raw)
  To: Nipun Gupta, Stephen Hemminger, orika
  Cc: dev, ferruh.yigit, arybchenko, hemant.agrawal, Rohit Raj, olivier.matz

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?



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets
  2020-09-01  8:09   ` Thomas Monjalon
@ 2020-09-01 10:56     ` Nipun Gupta
  2020-09-21  7:29     ` Ori Kam
  1 sibling, 0 replies; 46+ messages in thread
From: Nipun Gupta @ 2020-09-01 10:56 UTC (permalink / raw)
  To: Thomas Monjalon, Stephen Hemminger, orika
  Cc: dev, ferruh.yigit, arybchenko, Hemant Agrawal, Rohit Raj, olivier.matz



> -----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?
> 


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets
  2020-09-01  8:09   ` Thomas Monjalon
  2020-09-01 10:56     ` Nipun Gupta
@ 2020-09-21  7:29     ` Ori Kam
  1 sibling, 0 replies; 46+ messages in thread
From: Ori Kam @ 2020-09-21  7:29 UTC (permalink / raw)
  To: NBU-Contact-Thomas Monjalon, Nipun Gupta, Stephen Hemminger, orika
  Cc: dev, ferruh.yigit, arybchenko, hemant.agrawal, Rohit Raj, olivier.matz

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets
  2020-08-31  7:53 [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets Nipun Gupta
  2020-08-31 12:58 ` Ferruh Yigit
  2020-08-31 17:00 ` Stephen Hemminger
@ 2020-10-05  7:15 ` nipun.gupta
  2020-10-05  7:15   ` [dpdk-dev] [PATCH 2/3 v2] net/dpaa: support RX offload for error packet drop nipun.gupta
                     ` (2 more replies)
  2020-10-09 13:13 ` [dpdk-dev] [PATCH 1/3 v3] " nipun.gupta
  2020-10-15 13:23 ` [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx " nipun.gupta
  4 siblings, 3 replies; 46+ messages in thread
From: nipun.gupta @ 2020-10-05  7:15 UTC (permalink / raw)
  To: dev
  Cc: thomas, ferruh.yigit, arybchenko, hemant.agrawal, sachin.saxena,
	rohit.raj, Nipun Gupta

From: Nipun Gupta <nipun.gupta@nxp.com>

This change adds a RX offload capability, which once enabled,
hardware will drop the packets in case there of any 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>
---
These patches are based over series:
https://patchwork.dpdk.org/patch/78630/

Changes in v2:
 - Add support in DPAA1 driver (patch 2/3)
 - Add support and config parameter in testpmd (patch 3/3)

 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 645a18664..314d06c74 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1260,6 +1260,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 | \
-- 
2.17.1


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [dpdk-dev] [PATCH 2/3 v2] net/dpaa: support RX offload for error packet drop
  2020-10-05  7:15 ` [dpdk-dev] [PATCH 1/3 v2] " nipun.gupta
@ 2020-10-05  7:15   ` nipun.gupta
  2020-10-05  7:15   ` [dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to drop error packets nipun.gupta
  2020-10-05 15:34   ` [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx " Stephen Hemminger
  2 siblings, 0 replies; 46+ messages in thread
From: nipun.gupta @ 2020-10-05  7:15 UTC (permalink / raw)
  To: dev
  Cc: thomas, ferruh.yigit, arybchenko, hemant.agrawal, sachin.saxena,
	rohit.raj, Nipun Gupta

From: Nipun Gupta <nipun.gupta@nxp.com>

This patch supports RX offload capability DEV_RX_OFFLOAD_ERR_PKT_DROP,
to drop error packets in the hardware

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
 drivers/bus/dpaa/base/fman/fman_hw.c |  7 ++++++-
 drivers/net/dpaa/dpaa_ethdev.c       | 18 ++++++++++++++----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/dpaa/base/fman/fman_hw.c b/drivers/bus/dpaa/base/fman/fman_hw.c
index 4ab49f785..40e4e0000 100644
--- a/drivers/bus/dpaa/base/fman/fman_hw.c
+++ b/drivers/bus/dpaa/base/fman/fman_hw.c
@@ -597,7 +597,12 @@ void
 fman_if_discard_rx_errors(struct fman_if *fm_if)
 {
 	struct __fman_if *__if = container_of(fm_if, struct __fman_if, __if);
-	unsigned int *fmbm_rfsdm, *fmbm_rfsem;
+	unsigned int *fmbm_rcfg, *fmbm_rfsdm, *fmbm_rfsem;
+	unsigned int val;
+
+	fmbm_rcfg = &((struct rx_bmi_regs *)__if->bmi_map)->fmbm_rcfg;
+	val = in_be32(fmbm_rcfg);
+	out_be32(fmbm_rcfg, val & ~BMI_PORT_CFG_FDOVR);
 
 	fmbm_rfsem = &((struct rx_bmi_regs *)__if->bmi_map)->fmbm_rfsem;
 	out_be32(fmbm_rfsem, 0);
diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index 1f94fc9b9..39f673db5 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -52,7 +52,8 @@
 /* Supported Rx offloads */
 static uint64_t dev_rx_offloads_sup =
 		DEV_RX_OFFLOAD_JUMBO_FRAME |
-		DEV_RX_OFFLOAD_SCATTER;
+		DEV_RX_OFFLOAD_SCATTER |
+		DEV_RX_OFFLOAD_ERR_PKT_DROP;
 
 /* Rx offloads which cannot be disabled */
 static uint64_t dev_rx_offloads_nodis =
@@ -262,6 +263,18 @@ dpaa_eth_dev_configure(struct rte_eth_dev *dev)
 		dev->data->scattered_rx = 1;
 	}
 
+	if (!(rx_offloads & DEV_RX_OFFLOAD_ERR_PKT_DROP)) {
+		struct dpaa_if *dpaa_intf = dev->data->dev_private;
+		struct qman_fq *rxq = &dpaa_intf->rx_queues[0];
+
+		DPAA_PMD_DEBUG("error packets will not be droppped on hw");
+		fman_if_receive_rx_errors(fif, FM_FD_RX_STATUS_ERR_MASK);
+		fman_if_set_err_fqid(fif, rxq->fqid);
+	} else {
+		DPAA_PMD_DEBUG("error packets will be droppped on hw");
+		fman_if_discard_rx_errors(fif);
+	}
+
 	if (!(default_q || fmc_q)) {
 		if (dpaa_fm_config(dev,
 			eth_conf->rx_adv_conf.rss_conf.rss_hf)) {
@@ -2031,9 +2044,6 @@ dpaa_dev_init(struct rte_eth_dev *eth_dev)
 		fman_intf->mac_addr.addr_bytes[5]);
 
 	if (!fman_intf->is_shared_mac) {
-		/* Configure error packet handling */
-		fman_if_receive_rx_errors(fman_intf,
-			FM_FD_RX_STATUS_ERR_MASK);
 		/* Disable RX mode */
 		fman_if_disable_rx(fman_intf);
 		/* Disable promiscuous mode */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to drop error packets
  2020-10-05  7:15 ` [dpdk-dev] [PATCH 1/3 v2] " nipun.gupta
  2020-10-05  7:15   ` [dpdk-dev] [PATCH 2/3 v2] net/dpaa: support RX offload for error packet drop nipun.gupta
@ 2020-10-05  7:15   ` nipun.gupta
  2020-10-08 15:06     ` Asaf Penso
  2020-10-05 15:34   ` [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx " Stephen Hemminger
  2 siblings, 1 reply; 46+ messages in thread
From: nipun.gupta @ 2020-10-05  7:15 UTC (permalink / raw)
  To: dev
  Cc: thomas, ferruh.yigit, arybchenko, hemant.agrawal, sachin.saxena,
	rohit.raj, Nipun Gupta

From: Nipun Gupta <nipun.gupta@nxp.com>

With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
capability, testpmd showcases this with a new added configuration
option 'enable-hw-drop-err'.

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
 app/test-pmd/parameters.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 1ead59579..ada870e2d 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -142,6 +142,7 @@ usage(char* progname)
 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
+	printf("  --enable-hw-drop-err: enable hardware packet drop for error packets.\n");
 	printf("  --enable-drop-en: enable per queue packet drop.\n");
 	printf("  --disable-rss: disable rss.\n");
 	printf("  --port-topology=<paired|chained|loop>: set port topology (paired "
@@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
 		{ "enable-hw-vlan-strip",       0, 0, 0 },
 		{ "enable-hw-vlan-extend",      0, 0, 0 },
 		{ "enable-hw-qinq-strip",       0, 0, 0 },
+		{ "enable-hw-drop-err",         0, 0, 0 },
 		{ "enable-drop-en",            0, 0, 0 },
 		{ "disable-rss",                0, 0, 0 },
 		{ "port-topology",              1, 0, 0 },
@@ -1037,6 +1039,9 @@ launch_args_parse(int argc, char** argv)
 					"enable-hw-qinq-strip"))
 				rx_offloads |= DEV_RX_OFFLOAD_QINQ_STRIP;
 
+			if (!strcmp(lgopts[opt_idx].name, "enable-hw-drop-err"))
+				rx_offloads |= DEV_RX_OFFLOAD_ERR_PKT_DROP;
+
 			if (!strcmp(lgopts[opt_idx].name, "enable-drop-en"))
 				rx_drop_en = 1;
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets
  2020-10-05  7:15 ` [dpdk-dev] [PATCH 1/3 v2] " nipun.gupta
  2020-10-05  7:15   ` [dpdk-dev] [PATCH 2/3 v2] net/dpaa: support RX offload for error packet drop nipun.gupta
  2020-10-05  7:15   ` [dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to drop error packets nipun.gupta
@ 2020-10-05 15:34   ` Stephen Hemminger
  2020-10-05 16:10     ` Jerin Jacob
  2 siblings, 1 reply; 46+ messages in thread
From: Stephen Hemminger @ 2020-10-05 15:34 UTC (permalink / raw)
  To: nipun.gupta
  Cc: dev, thomas, ferruh.yigit, arybchenko, hemant.agrawal,
	sachin.saxena, rohit.raj

On Mon,  5 Oct 2020 12:45:04 +0530
nipun.gupta@nxp.com wrote:

> From: Nipun Gupta <nipun.gupta@nxp.com>
> 
> This change adds a RX offload capability, which once enabled,
> hardware will drop the packets in case there of any 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>
> ---
> These patches are based over series:
> https://patchwork.dpdk.org/patch/78630/
> 
> Changes in v2:
>  - Add support in DPAA1 driver (patch 2/3)
>  - Add support and config parameter in testpmd (patch 3/3)
> 
>  lib/librte_ethdev/rte_ethdev.h | 1 +
>  1 file changed, 1 insertion(+)

Maybe this should be an rte_flow match/action which would then make it
more flexible?

There is not much of a performance gain for this in real life and
if only one driver supports it then I am not convinced this is needed.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets
  2020-10-05 15:34   ` [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx " Stephen Hemminger
@ 2020-10-05 16:10     ` Jerin Jacob
  2020-10-06 10:37       ` Nipun Gupta
  0 siblings, 1 reply; 46+ messages in thread
From: Jerin Jacob @ 2020-10-05 16:10 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Nipun Gupta, dpdk-dev, Thomas Monjalon, Ferruh Yigit,
	Andrew Rybchenko, Hemant Agrawal, Sachin Saxena, rohit.raj

On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Mon,  5 Oct 2020 12:45:04 +0530
> nipun.gupta@nxp.com wrote:
>
> > From: Nipun Gupta <nipun.gupta@nxp.com>
> >
> > This change adds a RX offload capability, which once enabled,
> > hardware will drop the packets in case there of any error in
> > the packet such as L3 checksum error or L4 checksum.

IMO, Providing additional support up to the level to choose the errors
to drops give more control to the application. Meaning,
L1 errors such as FCS error
L2 errors ..
L3 errors such checksum
i.e ethdev spec need to have  error level supported by PMD and the
application can set the layers interested to drop.

> >
> > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > ---
> > These patches are based over series:
> > https://patchwork.dpdk.org/patch/78630/
> >
> > Changes in v2:
> >  - Add support in DPAA1 driver (patch 2/3)
> >  - Add support and config parameter in testpmd (patch 3/3)
> >
> >  lib/librte_ethdev/rte_ethdev.h | 1 +
> >  1 file changed, 1 insertion(+)
>
> Maybe this should be an rte_flow match/action which would then make it
> more flexible?

I think, it is not based on any Patten matching. So IMO, it should be best if it
is part of RX offload.

>
> There is not much of a performance gain for this in real life and
> if only one driver supports it then I am not convinced this is needed.

Marvell HW has this feature.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets
  2020-10-05 16:10     ` Jerin Jacob
@ 2020-10-06 10:37       ` Nipun Gupta
  2020-10-06 12:01         ` Jerin Jacob
  0 siblings, 1 reply; 46+ messages in thread
From: Nipun Gupta @ 2020-10-06 10:37 UTC (permalink / raw)
  To: Jerin Jacob, Stephen Hemminger
  Cc: dpdk-dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko,
	Hemant Agrawal, Sachin Saxena, Rohit Raj



> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Monday, October 5, 2020 9:40 PM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Nipun Gupta <nipun.gupta@nxp.com>; dpdk-dev <dev@dpdk.org>; Thomas
> Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> Andrew Rybchenko <arybchenko@solarflare.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>; Rohit
> Raj <rohit.raj@nxp.com>
> Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> packets
> 
> On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Mon,  5 Oct 2020 12:45:04 +0530
> > nipun.gupta@nxp.com wrote:
> >
> > > From: Nipun Gupta <nipun.gupta@nxp.com>
> > >
> > > This change adds a RX offload capability, which once enabled,
> > > hardware will drop the packets in case there of any error in
> > > the packet such as L3 checksum error or L4 checksum.
> 
> IMO, Providing additional support up to the level to choose the errors
> to drops give more control to the application. Meaning,
> L1 errors such as FCS error
> L2 errors ..
> L3 errors such checksum
> i.e ethdev spec need to have  error level supported by PMD and the
> application can set the layers interested to drop.

Agree, but 'DEV_RX_OFFLOAD_ERR_PKT_DROP' shall also be there to drop all the 
error packets? Maybe we can rename it to DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP.

Currently we have not planned to add separate knobs for separate error in
the driver, maybe we can define them separately, or we need have them in
this series itself?

> 
> > >
> > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > ---
> > > These patches are based over series:
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo
> rk.dpdk.org%2Fpatch%2F78630%2F&amp;data=02%7C01%7Cnipun.gupta%40nx
> p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
> 9c5c301635%7C0%7C0%7C637375110263097933&amp;sdata=RBQswMBsfpM6
> nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&amp;reserved=0
> > >
> > > Changes in v2:
> > >  - Add support in DPAA1 driver (patch 2/3)
> > >  - Add support and config parameter in testpmd (patch 3/3)
> > >
> > >  lib/librte_ethdev/rte_ethdev.h | 1 +
> > >  1 file changed, 1 insertion(+)
> >
> > Maybe this should be an rte_flow match/action which would then make it
> > more flexible?
> 
> I think, it is not based on any Patten matching. So IMO, it should be best if it
> is part of RX offload.
> 
> >
> > There is not much of a performance gain for this in real life and
> > if only one driver supports it then I am not convinced this is needed.
> 
> Marvell HW has this feature.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets
  2020-10-06 10:37       ` Nipun Gupta
@ 2020-10-06 12:01         ` Jerin Jacob
  2020-10-06 13:10           ` Nipun Gupta
  0 siblings, 1 reply; 46+ messages in thread
From: Jerin Jacob @ 2020-10-06 12:01 UTC (permalink / raw)
  To: Nipun Gupta
  Cc: Stephen Hemminger, dpdk-dev, Thomas Monjalon, Ferruh Yigit,
	Andrew Rybchenko, Hemant Agrawal, Sachin Saxena, Rohit Raj

On Tue, Oct 6, 2020 at 4:07 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Monday, October 5, 2020 9:40 PM
> > To: Stephen Hemminger <stephen@networkplumber.org>
> > Cc: Nipun Gupta <nipun.gupta@nxp.com>; dpdk-dev <dev@dpdk.org>; Thomas
> > Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> > Andrew Rybchenko <arybchenko@solarflare.com>; Hemant Agrawal
> > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>; Rohit
> > Raj <rohit.raj@nxp.com>
> > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> > packets
> >
> > On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> > >
> > > On Mon,  5 Oct 2020 12:45:04 +0530
> > > nipun.gupta@nxp.com wrote:
> > >
> > > > From: Nipun Gupta <nipun.gupta@nxp.com>
> > > >
> > > > This change adds a RX offload capability, which once enabled,
> > > > hardware will drop the packets in case there of any error in
> > > > the packet such as L3 checksum error or L4 checksum.
> >
> > IMO, Providing additional support up to the level to choose the errors
> > to drops give more control to the application. Meaning,
> > L1 errors such as FCS error
> > L2 errors ..
> > L3 errors such checksum
> > i.e ethdev spec need to have  error level supported by PMD and the
> > application can set the layers interested to drop.
>
> Agree, but 'DEV_RX_OFFLOAD_ERR_PKT_DROP' shall also be there to drop all the
> error packets? Maybe we can rename it to DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP.

IMHO,  we introduce such shortcut for a single flag for all err drop
then we can not change the scheme
without an API/ABI break.

>
> Currently we have not planned to add separate knobs for separate error in
> the driver, maybe we can define them separately, or we need have them in
> this series itself?

I think, ethdev API can have the capability on what are levels it
supported, in your
driver case, you can express the same.


>
> >
> > > >
> > > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > > ---
> > > > These patches are based over series:
> > > >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo
> > rk.dpdk.org%2Fpatch%2F78630%2F&amp;data=02%7C01%7Cnipun.gupta%40nx
> > p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
> > 9c5c301635%7C0%7C0%7C637375110263097933&amp;sdata=RBQswMBsfpM6
> > nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&amp;reserved=0
> > > >
> > > > Changes in v2:
> > > >  - Add support in DPAA1 driver (patch 2/3)
> > > >  - Add support and config parameter in testpmd (patch 3/3)
> > > >
> > > >  lib/librte_ethdev/rte_ethdev.h | 1 +
> > > >  1 file changed, 1 insertion(+)
> > >
> > > Maybe this should be an rte_flow match/action which would then make it
> > > more flexible?
> >
> > I think, it is not based on any Patten matching. So IMO, it should be best if it
> > is part of RX offload.
> >
> > >
> > > There is not much of a performance gain for this in real life and
> > > if only one driver supports it then I am not convinced this is needed.
> >
> > Marvell HW has this feature.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets
  2020-10-06 12:01         ` Jerin Jacob
@ 2020-10-06 13:10           ` Nipun Gupta
  2020-10-06 13:13             ` Jerin Jacob
  0 siblings, 1 reply; 46+ messages in thread
From: Nipun Gupta @ 2020-10-06 13:10 UTC (permalink / raw)
  To: Jerin Jacob
  Cc: Stephen Hemminger, dpdk-dev, Thomas Monjalon, Ferruh Yigit,
	Andrew Rybchenko, Hemant Agrawal, Sachin Saxena, Rohit Raj



> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Tuesday, October 6, 2020 5:31 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
> <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
> Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> packets
> 
> On Tue, Oct 6, 2020 at 4:07 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > Sent: Monday, October 5, 2020 9:40 PM
> > > To: Stephen Hemminger <stephen@networkplumber.org>
> > > Cc: Nipun Gupta <nipun.gupta@nxp.com>; dpdk-dev <dev@dpdk.org>;
> Thomas
> > > Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> > > Andrew Rybchenko <arybchenko@solarflare.com>; Hemant Agrawal
> > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> Rohit
> > > Raj <rohit.raj@nxp.com>
> > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> > > packets
> > >
> > > On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
> > > <stephen@networkplumber.org> wrote:
> > > >
> > > > On Mon,  5 Oct 2020 12:45:04 +0530
> > > > nipun.gupta@nxp.com wrote:
> > > >
> > > > > From: Nipun Gupta <nipun.gupta@nxp.com>
> > > > >
> > > > > This change adds a RX offload capability, which once enabled,
> > > > > hardware will drop the packets in case there of any error in
> > > > > the packet such as L3 checksum error or L4 checksum.
> > >
> > > IMO, Providing additional support up to the level to choose the errors
> > > to drops give more control to the application. Meaning,
> > > L1 errors such as FCS error
> > > L2 errors ..
> > > L3 errors such checksum
> > > i.e ethdev spec need to have  error level supported by PMD and the
> > > application can set the layers interested to drop.
> >
> > Agree, but 'DEV_RX_OFFLOAD_ERR_PKT_DROP' shall also be there to drop all
> the
> > error packets? Maybe we can rename it to
> DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP.
> 
> IMHO,  we introduce such shortcut for a single flag for all err drop
> then we can not change the scheme
> without an API/ABI break.

Are the following offloads fine:
	DEV_RX_OFFLOAD_L1_FCS_ERR_PKT_DROP
	DEV_RX_OFFLOAD_L3_CSUM_ERR_PKT_DROP
	DEV_RX_OFFLOAD_L4_CSUM_ERR_PKT_DROP
	DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP

Please let me know in case I need to add any other too.
Ill send a v3.

Thanks,
Nipun

> 
> >
> > Currently we have not planned to add separate knobs for separate error in
> > the driver, maybe we can define them separately, or we need have them in
> > this series itself?
> 
> I think, ethdev API can have the capability on what are levels it
> supported, in your
> driver case, you can express the same.
> 
> 
> >
> > >
> > > > >
> > > > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > > > ---
> > > > > These patches are based over series:
> > > > >
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo
> > >
> rk.dpdk.org%2Fpatch%2F78630%2F&amp;data=02%7C01%7Cnipun.gupta%40nx
> > >
> p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
> > >
> 9c5c301635%7C0%7C0%7C637375110263097933&amp;sdata=RBQswMBsfpM6
> > > nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&amp;reserved=0
> > > > >
> > > > > Changes in v2:
> > > > >  - Add support in DPAA1 driver (patch 2/3)
> > > > >  - Add support and config parameter in testpmd (patch 3/3)
> > > > >
> > > > >  lib/librte_ethdev/rte_ethdev.h | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > >
> > > > Maybe this should be an rte_flow match/action which would then make it
> > > > more flexible?
> > >
> > > I think, it is not based on any Patten matching. So IMO, it should be best if it
> > > is part of RX offload.
> > >
> > > >
> > > > There is not much of a performance gain for this in real life and
> > > > if only one driver supports it then I am not convinced this is needed.
> > >
> > > Marvell HW has this feature.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets
  2020-10-06 13:10           ` Nipun Gupta
@ 2020-10-06 13:13             ` Jerin Jacob
  2020-10-08  8:53               ` Nipun Gupta
  0 siblings, 1 reply; 46+ messages in thread
From: Jerin Jacob @ 2020-10-06 13:13 UTC (permalink / raw)
  To: Nipun Gupta
  Cc: Stephen Hemminger, dpdk-dev, Thomas Monjalon, Ferruh Yigit,
	Andrew Rybchenko, Hemant Agrawal, Sachin Saxena, Rohit Raj

On Tue, Oct 6, 2020 at 6:40 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Tuesday, October 6, 2020 5:31 PM
> > To: Nipun Gupta <nipun.gupta@nxp.com>
> > Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
> > <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> > <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>;
> > Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> > <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
> > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> > packets
> >
> > On Tue, Oct 6, 2020 at 4:07 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > Sent: Monday, October 5, 2020 9:40 PM
> > > > To: Stephen Hemminger <stephen@networkplumber.org>
> > > > Cc: Nipun Gupta <nipun.gupta@nxp.com>; dpdk-dev <dev@dpdk.org>;
> > Thomas
> > > > Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> > > > Andrew Rybchenko <arybchenko@solarflare.com>; Hemant Agrawal
> > > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> > Rohit
> > > > Raj <rohit.raj@nxp.com>
> > > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> > > > packets
> > > >
> > > > On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
> > > > <stephen@networkplumber.org> wrote:
> > > > >
> > > > > On Mon,  5 Oct 2020 12:45:04 +0530
> > > > > nipun.gupta@nxp.com wrote:
> > > > >
> > > > > > From: Nipun Gupta <nipun.gupta@nxp.com>
> > > > > >
> > > > > > This change adds a RX offload capability, which once enabled,
> > > > > > hardware will drop the packets in case there of any error in
> > > > > > the packet such as L3 checksum error or L4 checksum.
> > > >
> > > > IMO, Providing additional support up to the level to choose the errors
> > > > to drops give more control to the application. Meaning,
> > > > L1 errors such as FCS error
> > > > L2 errors ..
> > > > L3 errors such checksum
> > > > i.e ethdev spec need to have  error level supported by PMD and the
> > > > application can set the layers interested to drop.
> > >
> > > Agree, but 'DEV_RX_OFFLOAD_ERR_PKT_DROP' shall also be there to drop all
> > the
> > > error packets? Maybe we can rename it to
> > DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP.
> >
> > IMHO,  we introduce such shortcut for a single flag for all err drop
> > then we can not change the scheme
> > without an API/ABI break.
>
> Are the following offloads fine:
>         DEV_RX_OFFLOAD_L1_FCS_ERR_PKT_DROP
>         DEV_RX_OFFLOAD_L3_CSUM_ERR_PKT_DROP
>         DEV_RX_OFFLOAD_L4_CSUM_ERR_PKT_DROP
>         DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP
>
> Please let me know in case I need to add any other too.

I think, single offload flags and some config/capability structure to
define the additional
layer selection would be good, instead of adding a lot of new offload flags.


> Ill send a v3.
>
> Thanks,
> Nipun
>
> >
> > >
> > > Currently we have not planned to add separate knobs for separate error in
> > > the driver, maybe we can define them separately, or we need have them in
> > > this series itself?
> >
> > I think, ethdev API can have the capability on what are levels it
> > supported, in your
> > driver case, you can express the same.
> >
> >
> > >
> > > >
> > > > > >
> > > > > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > > > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > > > > ---
> > > > > > These patches are based over series:
> > > > > >
> > > >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo
> > > >
> > rk.dpdk.org%2Fpatch%2F78630%2F&amp;data=02%7C01%7Cnipun.gupta%40nx
> > > >
> > p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
> > > >
> > 9c5c301635%7C0%7C0%7C637375110263097933&amp;sdata=RBQswMBsfpM6
> > > > nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&amp;reserved=0
> > > > > >
> > > > > > Changes in v2:
> > > > > >  - Add support in DPAA1 driver (patch 2/3)
> > > > > >  - Add support and config parameter in testpmd (patch 3/3)
> > > > > >
> > > > > >  lib/librte_ethdev/rte_ethdev.h | 1 +
> > > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > Maybe this should be an rte_flow match/action which would then make it
> > > > > more flexible?
> > > >
> > > > I think, it is not based on any Patten matching. So IMO, it should be best if it
> > > > is part of RX offload.
> > > >
> > > > >
> > > > > There is not much of a performance gain for this in real life and
> > > > > if only one driver supports it then I am not convinced this is needed.
> > > >
> > > > Marvell HW has this feature.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets
  2020-10-06 13:13             ` Jerin Jacob
@ 2020-10-08  8:53               ` Nipun Gupta
  2020-10-08  8:55                 ` Jerin Jacob
  0 siblings, 1 reply; 46+ messages in thread
From: Nipun Gupta @ 2020-10-08  8:53 UTC (permalink / raw)
  To: Jerin Jacob
  Cc: Stephen Hemminger, dpdk-dev, Thomas Monjalon, Ferruh Yigit,
	Andrew Rybchenko, Hemant Agrawal, Sachin Saxena, Rohit Raj



> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Tuesday, October 6, 2020 6:44 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
> <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
> Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> packets
> 
> On Tue, Oct 6, 2020 at 6:40 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > Sent: Tuesday, October 6, 2020 5:31 PM
> > > To: Nipun Gupta <nipun.gupta@nxp.com>
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
> > > <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> > > <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>;
> > > Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> > > <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
> > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> > > packets
> > >
> > > On Tue, Oct 6, 2020 at 4:07 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > > Sent: Monday, October 5, 2020 9:40 PM
> > > > > To: Stephen Hemminger <stephen@networkplumber.org>
> > > > > Cc: Nipun Gupta <nipun.gupta@nxp.com>; dpdk-dev <dev@dpdk.org>;
> > > Thomas
> > > > > Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@intel.com>;
> > > > > Andrew Rybchenko <arybchenko@solarflare.com>; Hemant Agrawal
> > > > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> > > Rohit
> > > > > Raj <rohit.raj@nxp.com>
> > > > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop
> error
> > > > > packets
> > > > >
> > > > > On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
> > > > > <stephen@networkplumber.org> wrote:
> > > > > >
> > > > > > On Mon,  5 Oct 2020 12:45:04 +0530
> > > > > > nipun.gupta@nxp.com wrote:
> > > > > >
> > > > > > > From: Nipun Gupta <nipun.gupta@nxp.com>
> > > > > > >
> > > > > > > This change adds a RX offload capability, which once enabled,
> > > > > > > hardware will drop the packets in case there of any error in
> > > > > > > the packet such as L3 checksum error or L4 checksum.
> > > > >
> > > > > IMO, Providing additional support up to the level to choose the errors
> > > > > to drops give more control to the application. Meaning,
> > > > > L1 errors such as FCS error
> > > > > L2 errors ..
> > > > > L3 errors such checksum
> > > > > i.e ethdev spec need to have  error level supported by PMD and the
> > > > > application can set the layers interested to drop.
> > > >
> > > > Agree, but 'DEV_RX_OFFLOAD_ERR_PKT_DROP' shall also be there to drop
> all
> > > the
> > > > error packets? Maybe we can rename it to
> > > DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP.
> > >
> > > IMHO,  we introduce such shortcut for a single flag for all err drop
> > > then we can not change the scheme
> > > without an API/ABI break.
> >
> > Are the following offloads fine:
> >         DEV_RX_OFFLOAD_L1_FCS_ERR_PKT_DROP
> >         DEV_RX_OFFLOAD_L3_CSUM_ERR_PKT_DROP
> >         DEV_RX_OFFLOAD_L4_CSUM_ERR_PKT_DROP
> >         DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP
> >
> > Please let me know in case I need to add any other too.
> 
> I think, single offload flags and some config/capability structure to
> define the additional
> layer selection would be good, instead of adding a lot of new offload flags.


+/**
+ * A structure used to enable/disable error packet drop on Rx.
+ */
+struct rte_rx_err_pkt_drop_conf {
+       /** enable/disable all RX error packet drop.
+        * 0 (default) - disable, 1 enable
+        */
+       uint32_t all:1;
+};
+
 /**
  * A structure used to configure an Ethernet port.
  * Depending upon the RX multi-queue mode, extra advanced
@@ -1236,6 +1246,8 @@ struct rte_eth_conf {
        uint32_t dcb_capability_en;
        struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
        struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
+       struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
+       /**< RX error packet drop configuration. */

Is this the kind of changes you are talking about?

Also, more changes will be there in 'struct rte_eth_dev_info' structure, defining
additional separate capability something like 'uint64_t rx_err_drop_offload_capa'.

Regards,
Nipun

> 
> 
> > Ill send a v3.
> >
> > Thanks,
> > Nipun
> >
> > >
> > > >
> > > > Currently we have not planned to add separate knobs for separate error in
> > > > the driver, maybe we can define them separately, or we need have them in
> > > > this series itself?
> > >
> > > I think, ethdev API can have the capability on what are levels it
> > > supported, in your
> > > driver case, you can express the same.
> > >
> > >
> > > >
> > > > >
> > > > > > >
> > > > > > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > > > > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > > > > > ---
> > > > > > > These patches are based over series:
> > > > > > >
> > > > >
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo
> > > > >
> > >
> rk.dpdk.org%2Fpatch%2F78630%2F&amp;data=02%7C01%7Cnipun.gupta%40nx
> > > > >
> > >
> p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
> > > > >
> > >
> 9c5c301635%7C0%7C0%7C637375110263097933&amp;sdata=RBQswMBsfpM6
> > > > > nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&amp;reserved=0
> > > > > > >
> > > > > > > Changes in v2:
> > > > > > >  - Add support in DPAA1 driver (patch 2/3)
> > > > > > >  - Add support and config parameter in testpmd (patch 3/3)
> > > > > > >
> > > > > > >  lib/librte_ethdev/rte_ethdev.h | 1 +
> > > > > > >  1 file changed, 1 insertion(+)
> > > > > >
> > > > > > Maybe this should be an rte_flow match/action which would then make
> it
> > > > > > more flexible?
> > > > >
> > > > > I think, it is not based on any Patten matching. So IMO, it should be best
> if it
> > > > > is part of RX offload.
> > > > >
> > > > > >
> > > > > > There is not much of a performance gain for this in real life and
> > > > > > if only one driver supports it then I am not convinced this is needed.
> > > > >
> > > > > Marvell HW has this feature.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets
  2020-10-08  8:53               ` Nipun Gupta
@ 2020-10-08  8:55                 ` Jerin Jacob
  2020-10-08 15:13                   ` Asaf Penso
  0 siblings, 1 reply; 46+ messages in thread
From: Jerin Jacob @ 2020-10-08  8:55 UTC (permalink / raw)
  To: Nipun Gupta
  Cc: Stephen Hemminger, dpdk-dev, Thomas Monjalon, Ferruh Yigit,
	Andrew Rybchenko, Hemant Agrawal, Sachin Saxena, Rohit Raj

On Thu, Oct 8, 2020 at 2:23 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Tuesday, October 6, 2020 6:44 PM
> > To: Nipun Gupta <nipun.gupta@nxp.com>
> > Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
> > <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> > <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>;
> > Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> > <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
> > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> > packets
> >
> > On Tue, Oct 6, 2020 at 6:40 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > Sent: Tuesday, October 6, 2020 5:31 PM
> > > > To: Nipun Gupta <nipun.gupta@nxp.com>
> > > > Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
> > > > <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> > > > <ferruh.yigit@intel.com>; Andrew Rybchenko
> > <arybchenko@solarflare.com>;
> > > > Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> > > > <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
> > > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> > > > packets
> > > >
> > > > On Tue, Oct 6, 2020 at 4:07 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > > > Sent: Monday, October 5, 2020 9:40 PM
> > > > > > To: Stephen Hemminger <stephen@networkplumber.org>
> > > > > > Cc: Nipun Gupta <nipun.gupta@nxp.com>; dpdk-dev <dev@dpdk.org>;
> > > > Thomas
> > > > > > Monjalon <thomas@monjalon.net>; Ferruh Yigit
> > <ferruh.yigit@intel.com>;
> > > > > > Andrew Rybchenko <arybchenko@solarflare.com>; Hemant Agrawal
> > > > > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> > > > Rohit
> > > > > > Raj <rohit.raj@nxp.com>
> > > > > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop
> > error
> > > > > > packets
> > > > > >
> > > > > > On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
> > > > > > <stephen@networkplumber.org> wrote:
> > > > > > >
> > > > > > > On Mon,  5 Oct 2020 12:45:04 +0530
> > > > > > > nipun.gupta@nxp.com wrote:
> > > > > > >
> > > > > > > > From: Nipun Gupta <nipun.gupta@nxp.com>
> > > > > > > >
> > > > > > > > This change adds a RX offload capability, which once enabled,
> > > > > > > > hardware will drop the packets in case there of any error in
> > > > > > > > the packet such as L3 checksum error or L4 checksum.
> > > > > >
> > > > > > IMO, Providing additional support up to the level to choose the errors
> > > > > > to drops give more control to the application. Meaning,
> > > > > > L1 errors such as FCS error
> > > > > > L2 errors ..
> > > > > > L3 errors such checksum
> > > > > > i.e ethdev spec need to have  error level supported by PMD and the
> > > > > > application can set the layers interested to drop.
> > > > >
> > > > > Agree, but 'DEV_RX_OFFLOAD_ERR_PKT_DROP' shall also be there to drop
> > all
> > > > the
> > > > > error packets? Maybe we can rename it to
> > > > DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP.
> > > >
> > > > IMHO,  we introduce such shortcut for a single flag for all err drop
> > > > then we can not change the scheme
> > > > without an API/ABI break.
> > >
> > > Are the following offloads fine:
> > >         DEV_RX_OFFLOAD_L1_FCS_ERR_PKT_DROP
> > >         DEV_RX_OFFLOAD_L3_CSUM_ERR_PKT_DROP
> > >         DEV_RX_OFFLOAD_L4_CSUM_ERR_PKT_DROP
> > >         DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP
> > >
> > > Please let me know in case I need to add any other too.
> >
> > I think, single offload flags and some config/capability structure to
> > define the additional
> > layer selection would be good, instead of adding a lot of new offload flags.
>
>
> +/**
> + * A structure used to enable/disable error packet drop on Rx.
> + */
> +struct rte_rx_err_pkt_drop_conf {
> +       /** enable/disable all RX error packet drop.
> +        * 0 (default) - disable, 1 enable
> +        */
> +       uint32_t all:1;
> +};
> +
>  /**
>   * A structure used to configure an Ethernet port.
>   * Depending upon the RX multi-queue mode, extra advanced
> @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
>         uint32_t dcb_capability_en;
>         struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
>         struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> +       struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> +       /**< RX error packet drop configuration. */
>
> Is this the kind of changes you are talking about?


Yes.

>
> Also, more changes will be there in 'struct rte_eth_dev_info' structure, defining
> additional separate capability something like 'uint64_t rx_err_drop_offload_capa'.
>
> Regards,
> Nipun
>
> >
> >
> > > Ill send a v3.
> > >
> > > Thanks,
> > > Nipun
> > >
> > > >
> > > > >
> > > > > Currently we have not planned to add separate knobs for separate error in
> > > > > the driver, maybe we can define them separately, or we need have them in
> > > > > this series itself?
> > > >
> > > > I think, ethdev API can have the capability on what are levels it
> > > > supported, in your
> > > > driver case, you can express the same.
> > > >
> > > >
> > > > >
> > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > > > > > > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> > > > > > > > ---
> > > > > > > > These patches are based over series:
> > > > > > > >
> > > > > >
> > > >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo
> > > > > >
> > > >
> > rk.dpdk.org%2Fpatch%2F78630%2F&amp;data=02%7C01%7Cnipun.gupta%40nx
> > > > > >
> > > >
> > p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
> > > > > >
> > > >
> > 9c5c301635%7C0%7C0%7C637375110263097933&amp;sdata=RBQswMBsfpM6
> > > > > > nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&amp;reserved=0
> > > > > > > >
> > > > > > > > Changes in v2:
> > > > > > > >  - Add support in DPAA1 driver (patch 2/3)
> > > > > > > >  - Add support and config parameter in testpmd (patch 3/3)
> > > > > > > >
> > > > > > > >  lib/librte_ethdev/rte_ethdev.h | 1 +
> > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > Maybe this should be an rte_flow match/action which would then make
> > it
> > > > > > > more flexible?
> > > > > >
> > > > > > I think, it is not based on any Patten matching. So IMO, it should be best
> > if it
> > > > > > is part of RX offload.
> > > > > >
> > > > > > >
> > > > > > > There is not much of a performance gain for this in real life and
> > > > > > > if only one driver supports it then I am not convinced this is needed.
> > > > > >
> > > > > > Marvell HW has this feature.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to drop error packets
  2020-10-05  7:15   ` [dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to drop error packets nipun.gupta
@ 2020-10-08 15:06     ` Asaf Penso
  2020-10-08 15:45       ` Nipun Gupta
  0 siblings, 1 reply; 46+ messages in thread
From: Asaf Penso @ 2020-10-08 15:06 UTC (permalink / raw)
  To: nipun.gupta, dev
  Cc: NBU-Contact-Thomas Monjalon, ferruh.yigit, arybchenko,
	hemant.agrawal, sachin.saxena, rohit.raj

>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of nipun.gupta@nxp.com
>Sent: Monday, October 5, 2020 10:15 AM
>To: dev@dpdk.org
>Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
>ferruh.yigit@intel.com; arybchenko@solarflare.com;
>hemant.agrawal@nxp.com; sachin.saxena@nxp.com; rohit.raj@nxp.com;
>Nipun Gupta <nipun.gupta@nxp.com>
>Subject: [dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to
>drop error packets
>
>From: Nipun Gupta <nipun.gupta@nxp.com>
>
>With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
>capability, testpmd showcases this with a new added configuration option
>'enable-hw-drop-err'.
>
>Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
>---
> app/test-pmd/parameters.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
>diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c index
>1ead59579..ada870e2d 100644
>--- a/app/test-pmd/parameters.c
>+++ b/app/test-pmd/parameters.c
>@@ -142,6 +142,7 @@ usage(char* progname)
> 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
> 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
> 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
>+	printf("  --enable-hw-drop-err: enable hardware packet drop for error
>+packets.\n");
> 	printf("  --enable-drop-en: enable per queue packet drop.\n");
> 	printf("  --disable-rss: disable rss.\n");
> 	printf("  --port-topology=<paired|chained|loop>: set port topology
>(paired "
>@@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
> 		{ "enable-hw-vlan-strip",       0, 0, 0 },
> 		{ "enable-hw-vlan-extend",      0, 0, 0 },
> 		{ "enable-hw-qinq-strip",       0, 0, 0 },
>+		{ "enable-hw-drop-err",         0, 0, 0 },
> 		{ "enable-drop-en",            0, 0, 0 },
> 		{ "disable-rss",                0, 0, 0 },
> 		{ "port-topology",              1, 0, 0 },
>@@ -1037,6 +1039,9 @@ launch_args_parse(int argc, char** argv)
> 					"enable-hw-qinq-strip"))
> 				rx_offloads |=
>DEV_RX_OFFLOAD_QINQ_STRIP;
>
>+			if (!strcmp(lgopts[opt_idx].name, "enable-hw-drop-
>err"))
>+				rx_offloads |=
>DEV_RX_OFFLOAD_ERR_PKT_DROP;
>+
> 			if (!strcmp(lgopts[opt_idx].name, "enable-drop-en"))
> 				rx_drop_en = 1;
>
>--
>2.17.1

I think it would be good also to update testpmd run_app.rst with this new option.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets
  2020-10-08  8:55                 ` Jerin Jacob
@ 2020-10-08 15:13                   ` Asaf Penso
  0 siblings, 0 replies; 46+ messages in thread
From: Asaf Penso @ 2020-10-08 15:13 UTC (permalink / raw)
  To: Jerin Jacob, Nipun Gupta
  Cc: Stephen Hemminger, dpdk-dev, NBU-Contact-Thomas Monjalon,
	Ferruh Yigit, Andrew Rybchenko, Hemant Agrawal, Sachin Saxena,
	Rohit Raj

>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of Jerin Jacob
>Sent: Thursday, October 8, 2020 11:56 AM
>To: Nipun Gupta <nipun.gupta@nxp.com>
>Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
><dev@dpdk.org>; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
>Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
><arybchenko@solarflare.com>; Hemant Agrawal
><hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
>Rohit Raj <rohit.raj@nxp.com>
>Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
>packets
>
>On Thu, Oct 8, 2020 at 2:23 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
>>
>>
>>
>> > -----Original Message-----
>> > From: Jerin Jacob <jerinjacobk@gmail.com>
>> > Sent: Tuesday, October 6, 2020 6:44 PM
>> > To: Nipun Gupta <nipun.gupta@nxp.com>
>> > Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
>> > <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh
>Yigit
>> > <ferruh.yigit@intel.com>; Andrew Rybchenko
>> > <arybchenko@solarflare.com>; Hemant Agrawal
>> > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
>> > Rohit Raj <rohit.raj@nxp.com>
>> > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to
>> > drop error packets
>> >
>> > On Tue, Oct 6, 2020 at 6:40 PM Nipun Gupta <nipun.gupta@nxp.com>
>wrote:
>> > >
>> > >
>> > >
>> > > > -----Original Message-----
>> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
>> > > > Sent: Tuesday, October 6, 2020 5:31 PM
>> > > > To: Nipun Gupta <nipun.gupta@nxp.com>
>> > > > Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
>> > > > <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>;
>Ferruh
>> > > > Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
>> > <arybchenko@solarflare.com>;
>> > > > Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
>> > > > <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>
>> > > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to
>> > > > drop error packets
>> > > >
>> > > > On Tue, Oct 6, 2020 at 4:07 PM Nipun Gupta <nipun.gupta@nxp.com>
>wrote:
>> > > > >
>> > > > >
>> > > > >
>> > > > > > -----Original Message-----
>> > > > > > From: Jerin Jacob <jerinjacobk@gmail.com>
>> > > > > > Sent: Monday, October 5, 2020 9:40 PM
>> > > > > > To: Stephen Hemminger <stephen@networkplumber.org>
>> > > > > > Cc: Nipun Gupta <nipun.gupta@nxp.com>; dpdk-dev
>> > > > > > <dev@dpdk.org>;
>> > > > Thomas
>> > > > > > Monjalon <thomas@monjalon.net>; Ferruh Yigit
>> > <ferruh.yigit@intel.com>;
>> > > > > > Andrew Rybchenko <arybchenko@solarflare.com>; Hemant
>Agrawal
>> > > > > > <hemant.agrawal@nxp.com>; Sachin Saxena
>> > > > > > <sachin.saxena@nxp.com>;
>> > > > Rohit
>> > > > > > Raj <rohit.raj@nxp.com>
>> > > > > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx
>> > > > > > offload to drop
>> > error
>> > > > > > packets
>> > > > > >
>> > > > > > On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
>> > > > > > <stephen@networkplumber.org> wrote:
>> > > > > > >
>> > > > > > > On Mon,  5 Oct 2020 12:45:04 +0530 nipun.gupta@nxp.com
>> > > > > > > wrote:
>> > > > > > >
>> > > > > > > > From: Nipun Gupta <nipun.gupta@nxp.com>
>> > > > > > > >
>> > > > > > > > This change adds a RX offload capability, which once
>> > > > > > > > enabled, hardware will drop the packets in case there of
>> > > > > > > > any error in the packet such as L3 checksum error or L4
>checksum.
>> > > > > >
>> > > > > > IMO, Providing additional support up to the level to choose
>> > > > > > the errors to drops give more control to the application.
>> > > > > > Meaning,
>> > > > > > L1 errors such as FCS error
>> > > > > > L2 errors ..
>> > > > > > L3 errors such checksum
>> > > > > > i.e ethdev spec need to have  error level supported by PMD
>> > > > > > and the application can set the layers interested to drop.
>> > > > >
>> > > > > Agree, but 'DEV_RX_OFFLOAD_ERR_PKT_DROP' shall also be there
>> > > > > to drop
>> > all
>> > > > the
>> > > > > error packets? Maybe we can rename it to
>> > > > DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP.
>> > > >
>> > > > IMHO,  we introduce such shortcut for a single flag for all err
>> > > > drop then we can not change the scheme without an API/ABI break.
>> > >
>> > > Are the following offloads fine:
>> > >         DEV_RX_OFFLOAD_L1_FCS_ERR_PKT_DROP
>> > >         DEV_RX_OFFLOAD_L3_CSUM_ERR_PKT_DROP
>> > >         DEV_RX_OFFLOAD_L4_CSUM_ERR_PKT_DROP
>> > >         DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP
>> > >
>> > > Please let me know in case I need to add any other too.
>> >
>> > I think, single offload flags and some config/capability structure
>> > to define the additional layer selection would be good, instead of
>> > adding a lot of new offload flags.
>>
>>
>> +/**
>> + * A structure used to enable/disable error packet drop on Rx.
>> + */
>> +struct rte_rx_err_pkt_drop_conf {
>> +       /** enable/disable all RX error packet drop.
>> +        * 0 (default) - disable, 1 enable
>> +        */
>> +       uint32_t all:1;
>> +};
>> +
>>  /**
>>   * A structure used to configure an Ethernet port.
>>   * Depending upon the RX multi-queue mode, extra advanced @@ -1236,6
>> +1246,8 @@ struct rte_eth_conf {
>>         uint32_t dcb_capability_en;
>>         struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
>>         struct rte_intr_conf intr_conf; /**< Interrupt mode
>> configuration. */
>> +       struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
>> +       /**< RX error packet drop configuration. */
>>
>> Is this the kind of changes you are talking about?
>
>
>Yes.
>
>>
>> Also, more changes will be there in 'struct rte_eth_dev_info'
>> structure, defining additional separate capability something like 'uint64_t
>rx_err_drop_offload_capa'.
>>
>> Regards,
>> Nipun
>>
>> >
>> >
>> > > Ill send a v3.
>> > >
>> > > Thanks,
>> > > Nipun
>> > >
>> > > >
>> > > > >
>> > > > > Currently we have not planned to add separate knobs for
>> > > > > separate error in the driver, maybe we can define them
>> > > > > separately, or we need have them in this series itself?
>> > > >
>> > > > I think, ethdev API can have the capability on what are levels
>> > > > it supported, in your driver case, you can express the same.
>> > > >
>> > > >
>> > > > >
>> > > > > >
>> > > > > > > >
>> > > > > > > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
>> > > > > > > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
>> > > > > > > > ---
>> > > > > > > > These patches are based over series:
>> > > > > > > >
>> > > > > >
>> > > >
>> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
>> > tchwo
>> > > > > >
>> > > >
>> >
>rk.dpdk.org%2Fpatch%2F78630%2F&amp;data=02%7C01%7Cnipun.gupta%40
>nx
>> > > > > >
>> > > >
>> >
>p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
>> > > > > >
>> > > >
>> >
>9c5c301635%7C0%7C0%7C637375110263097933&amp;sdata=RBQswMBsfpM6
>> > > > > >
>nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&amp;reserved=0
>> > > > > > > >
>> > > > > > > > Changes in v2:
>> > > > > > > >  - Add support in DPAA1 driver (patch 2/3)
>> > > > > > > >  - Add support and config parameter in testpmd (patch
>> > > > > > > > 3/3)
>> > > > > > > >
>> > > > > > > >  lib/librte_ethdev/rte_ethdev.h | 1 +
>> > > > > > > >  1 file changed, 1 insertion(+)
>> > > > > > >
>> > > > > > > Maybe this should be an rte_flow match/action which would
>> > > > > > > then make
>> > it
>> > > > > > > more flexible?
>> > > > > >
>> > > > > > I think, it is not based on any Patten matching. So IMO, it
>> > > > > > should be best
>> > if it
>> > > > > > is part of RX offload.
>> > > > > >
>> > > > > > >
>> > > > > > > There is not much of a performance gain for this in real
>> > > > > > > life and if only one driver supports it then I am not convinced this
>is needed.
>> > > > > >
>> > > > > > Marvell HW has this feature.
Reviewed-By: Asaf Penso <asafp@nvidia.com>

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to drop error packets
  2020-10-08 15:06     ` Asaf Penso
@ 2020-10-08 15:45       ` Nipun Gupta
  0 siblings, 0 replies; 46+ messages in thread
From: Nipun Gupta @ 2020-10-08 15:45 UTC (permalink / raw)
  To: Asaf Penso, dev
  Cc: NBU-Contact-Thomas Monjalon, ferruh.yigit, arybchenko,
	Hemant Agrawal, Sachin Saxena, Rohit Raj



> -----Original Message-----
> From: Asaf Penso <asafp@nvidia.com>
> Sent: Thursday, October 8, 2020 8:36 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
> ferruh.yigit@intel.com; arybchenko@solarflare.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>; Rohit
> Raj <rohit.raj@nxp.com>
> Subject: RE: [dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to
> drop error packets
> 
> >-----Original Message-----
> >From: dev <dev-bounces@dpdk.org> On Behalf Of nipun.gupta@nxp.com
> >Sent: Monday, October 5, 2020 10:15 AM
> >To: dev@dpdk.org
> >Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
> >ferruh.yigit@intel.com; arybchenko@solarflare.com;
> >hemant.agrawal@nxp.com; sachin.saxena@nxp.com; rohit.raj@nxp.com;
> >Nipun Gupta <nipun.gupta@nxp.com>
> >Subject: [dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to
> >drop error packets
> >
> >From: Nipun Gupta <nipun.gupta@nxp.com>
> >
> >With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
> >capability, testpmd showcases this with a new added configuration option
> >'enable-hw-drop-err'.
> >
> >Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> >---
> > app/test-pmd/parameters.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> >diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c index
> >1ead59579..ada870e2d 100644
> >--- a/app/test-pmd/parameters.c
> >+++ b/app/test-pmd/parameters.c
> >@@ -142,6 +142,7 @@ usage(char* progname)
> > 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
> > 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
> > 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
> >+	printf("  --enable-hw-drop-err: enable hardware packet drop for error
> >+packets.\n");
> > 	printf("  --enable-drop-en: enable per queue packet drop.\n");
> > 	printf("  --disable-rss: disable rss.\n");
> > 	printf("  --port-topology=<paired|chained|loop>: set port topology
> >(paired "
> >@@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
> > 		{ "enable-hw-vlan-strip",       0, 0, 0 },
> > 		{ "enable-hw-vlan-extend",      0, 0, 0 },
> > 		{ "enable-hw-qinq-strip",       0, 0, 0 },
> >+		{ "enable-hw-drop-err",         0, 0, 0 },
> > 		{ "enable-drop-en",            0, 0, 0 },
> > 		{ "disable-rss",                0, 0, 0 },
> > 		{ "port-topology",              1, 0, 0 },
> >@@ -1037,6 +1039,9 @@ launch_args_parse(int argc, char** argv)
> > 					"enable-hw-qinq-strip"))
> > 				rx_offloads |=
> >DEV_RX_OFFLOAD_QINQ_STRIP;
> >
> >+			if (!strcmp(lgopts[opt_idx].name, "enable-hw-drop-
> >err"))
> >+				rx_offloads |=
> >DEV_RX_OFFLOAD_ERR_PKT_DROP;
> >+
> > 			if (!strcmp(lgopts[opt_idx].name, "enable-drop-en"))
> > 				rx_drop_en = 1;
> >
> >--
> >2.17.1
> 
> I think it would be good also to update testpmd run_app.rst with this new option.

Sure. Will update in next spin.

Thanks,
Nipun

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets
  2020-08-31  7:53 [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets Nipun Gupta
                   ` (2 preceding siblings ...)
  2020-10-05  7:15 ` [dpdk-dev] [PATCH 1/3 v2] " nipun.gupta
@ 2020-10-09 13:13 ` nipun.gupta
  2020-10-09 13:13   ` [dpdk-dev] [PATCH 2/3 v3] net/dpaa: support RX offload for error packet drop nipun.gupta
                     ` (4 more replies)
  2020-10-15 13:23 ` [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx " nipun.gupta
  4 siblings, 5 replies; 46+ messages in thread
From: nipun.gupta @ 2020-10-09 13:13 UTC (permalink / raw)
  To: dev
  Cc: thomas, ferruh.yigit, arybchenko, hemant.agrawal, sachin.saxena,
	rohit.raj, jerinjacobk, stephen, asafp, Nipun Gupta

From: Nipun Gupta <nipun.gupta@nxp.com>

This change adds a RX offload capability and configuration to
enable hardware to drop the packets in case of any error in the
packets 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>
Reviewed-by: Asaf Penso <asafp@nvidia.com>
---

v3:
 - Add additional rx_err_drop_offload_capa, which is specific
   capability flag for RX packets error drop offload. Currently
   only 'all' error packet drops are enabled, but can be extended
   to provide capability to drop any specific errors like L1 FCS,
   L3 Checksum etc.
 - Added separate config structure to enable the drop configuration.
 - Updated doc with the new updated option in testbbdev (patch 3/3)

v2:
 - Add support in DPAA1 driver (patch 2/3)
 - Add support and config parameter in testpmd (patch 3/3)

 lib/librte_ethdev/rte_ethdev.c |  1 +
 lib/librte_ethdev/rte_ethdev.h | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 48d1333b1..be25e947e 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -128,6 +128,7 @@ static const struct {
 	RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
 	RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
 	RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
+	RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
 };
 
 #undef RTE_RX_OFFLOAD_BIT2STR
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index d2bf74f12..cb968d38a 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1194,6 +1194,16 @@ struct rte_intr_conf {
 	uint32_t rmv:1;
 };
 
+/**
+ * A structure used to enable/disable error packet drop on RX.
+ */
+struct rte_rx_err_pkt_drop_conf {
+	/** enable/disable all RX error packet drop.
+	 * 0 (default) - disable, 1 enable
+	 */
+	uint32_t all:1;
+};
+
 /**
  * A structure used to configure an Ethernet port.
  * Depending upon the RX multi-queue mode, extra advanced
@@ -1236,6 +1246,8 @@ struct rte_eth_conf {
 	uint32_t dcb_capability_en;
 	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
 	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
+	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
+	/**< RX error packet drop configuration. */
 };
 
 /**
@@ -1260,6 +1272,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 | \
@@ -1274,6 +1287,13 @@ struct rte_eth_conf {
  * mentioned in rte_rx_offload_names in rte_ethdev.c file.
  */
 
+/**
+ * RX Error Drop offload config/capabilities of a device. These
+ * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
+ * is supported by the device.
+ */
+#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001
+
 /**
  * TX offload capabilities of a device.
  */
@@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
 	/**< Device per-queue RX offload capabilities. */
 	uint64_t tx_queue_offload_capa;
 	/**< Device per-queue TX offload capabilities. */
+	uint64_t rx_err_drop_offload_capa;
+	/**< RX error packet drop offload capabilities. */
 	uint16_t reta_size;
 	/**< Device redirection table size, the total number of entries. */
 	uint8_t hash_key_size; /**< Hash key size in bytes */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [dpdk-dev] [PATCH 2/3 v3] net/dpaa: support RX offload for error packet drop
  2020-10-09 13:13 ` [dpdk-dev] [PATCH 1/3 v3] " nipun.gupta
@ 2020-10-09 13:13   ` nipun.gupta
  2020-10-09 13:13   ` [dpdk-dev] [PATCH 3/3 v3] app/testpmd: support hardware offload to drop error packets nipun.gupta
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 46+ messages in thread
From: nipun.gupta @ 2020-10-09 13:13 UTC (permalink / raw)
  To: dev
  Cc: thomas, ferruh.yigit, arybchenko, hemant.agrawal, sachin.saxena,
	rohit.raj, jerinjacobk, stephen, asafp, Nipun Gupta

From: Nipun Gupta <nipun.gupta@nxp.com>

This patch supports RX offload configuration to drop error
packets in the hardware

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
 drivers/bus/dpaa/base/fman/fman_hw.c |  7 ++++++-
 drivers/net/dpaa/dpaa_ethdev.c       | 23 +++++++++++++++++++----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/dpaa/base/fman/fman_hw.c b/drivers/bus/dpaa/base/fman/fman_hw.c
index 4ab49f785..40e4e0000 100644
--- a/drivers/bus/dpaa/base/fman/fman_hw.c
+++ b/drivers/bus/dpaa/base/fman/fman_hw.c
@@ -597,7 +597,12 @@ void
 fman_if_discard_rx_errors(struct fman_if *fm_if)
 {
 	struct __fman_if *__if = container_of(fm_if, struct __fman_if, __if);
-	unsigned int *fmbm_rfsdm, *fmbm_rfsem;
+	unsigned int *fmbm_rcfg, *fmbm_rfsdm, *fmbm_rfsem;
+	unsigned int val;
+
+	fmbm_rcfg = &((struct rx_bmi_regs *)__if->bmi_map)->fmbm_rcfg;
+	val = in_be32(fmbm_rcfg);
+	out_be32(fmbm_rcfg, val & ~BMI_PORT_CFG_FDOVR);
 
 	fmbm_rfsem = &((struct rx_bmi_regs *)__if->bmi_map)->fmbm_rfsem;
 	out_be32(fmbm_rfsem, 0);
diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index af47c196a..bed84a584 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -52,7 +52,8 @@
 /* Supported Rx offloads */
 static uint64_t dev_rx_offloads_sup =
 		DEV_RX_OFFLOAD_JUMBO_FRAME |
-		DEV_RX_OFFLOAD_SCATTER;
+		DEV_RX_OFFLOAD_SCATTER |
+		DEV_RX_OFFLOAD_ERR_PKT_DROP;
 
 /* Rx offloads which cannot be disabled */
 static uint64_t dev_rx_offloads_nodis =
@@ -62,6 +63,10 @@ static uint64_t dev_rx_offloads_nodis =
 		DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM |
 		DEV_RX_OFFLOAD_RSS_HASH;
 
+/* Supported Rx Error packet drop offload */
+static uint64_t dev_rx_err_drop_offloads_sup =
+		DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL;
+
 /* Supported Tx offloads */
 static uint64_t dev_tx_offloads_sup =
 		DEV_TX_OFFLOAD_MT_LOCKFREE |
@@ -262,6 +267,18 @@ dpaa_eth_dev_configure(struct rte_eth_dev *dev)
 		dev->data->scattered_rx = 1;
 	}
 
+	if (eth_conf->err_pkt_drop_conf.all) {
+		DPAA_PMD_DEBUG("error packets will be dropped on hw");
+		fman_if_discard_rx_errors(fif);
+	} else {
+		struct dpaa_if *dpaa_intf = dev->data->dev_private;
+		struct qman_fq *rxq = &dpaa_intf->rx_queues[0];
+
+		DPAA_PMD_DEBUG("error packets will not be dropped on hw");
+		fman_if_receive_rx_errors(fif, FM_FD_RX_STATUS_ERR_MASK);
+		fman_if_set_err_fqid(fif, rxq->fqid);
+	}
+
 	if (!(default_q || fmc_q)) {
 		if (dpaa_fm_config(dev,
 			eth_conf->rx_adv_conf.rss_conf.rss_hf)) {
@@ -591,6 +608,7 @@ static int dpaa_eth_dev_info(struct rte_eth_dev *dev,
 					dev_rx_offloads_nodis;
 	dev_info->tx_offload_capa = dev_tx_offloads_sup |
 					dev_tx_offloads_nodis;
+	dev_info->rx_err_drop_offload_capa = dev_rx_err_drop_offloads_sup;
 	dev_info->default_rxportconf.burst_size = DPAA_DEF_RX_BURST_SIZE;
 	dev_info->default_txportconf.burst_size = DPAA_DEF_TX_BURST_SIZE;
 	dev_info->default_rxportconf.nb_queues = 1;
@@ -2085,9 +2103,6 @@ dpaa_dev_init(struct rte_eth_dev *eth_dev)
 		fman_intf->mac_addr.addr_bytes[5]);
 
 	if (!fman_intf->is_shared_mac) {
-		/* Configure error packet handling */
-		fman_if_receive_rx_errors(fman_intf,
-			FM_FD_RX_STATUS_ERR_MASK);
 		/* Disable RX mode */
 		fman_if_disable_rx(fman_intf);
 		/* Disable promiscuous mode */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [dpdk-dev] [PATCH 3/3 v3] app/testpmd: support hardware offload to drop error packets
  2020-10-09 13:13 ` [dpdk-dev] [PATCH 1/3 v3] " nipun.gupta
  2020-10-09 13:13   ` [dpdk-dev] [PATCH 2/3 v3] net/dpaa: support RX offload for error packet drop nipun.gupta
@ 2020-10-09 13:13   ` nipun.gupta
  2020-10-11  7:22     ` Asaf Penso
  2020-10-11 10:13   ` [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx " Jerin Jacob
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 46+ messages in thread
From: nipun.gupta @ 2020-10-09 13:13 UTC (permalink / raw)
  To: dev
  Cc: thomas, ferruh.yigit, arybchenko, hemant.agrawal, sachin.saxena,
	rohit.raj, jerinjacobk, stephen, asafp, Nipun Gupta

From: Nipun Gupta <nipun.gupta@nxp.com>

With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
capability, and separate DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL capability
to drop all error packets in hardware, testpmd showcases this
with a new added configuration option 'enable-hw-drop-err-all'.

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
 app/test-pmd/parameters.c             | 7 +++++++
 app/test-pmd/testpmd.c                | 8 ++++++++
 app/test-pmd/testpmd.h                | 1 +
 doc/guides/testpmd_app_ug/run_app.rst | 4 ++++
 4 files changed, 20 insertions(+)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 1ead59579..508612426 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -142,6 +142,7 @@ usage(char* progname)
 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
+	printf("  --enable-hw-drop-err-all: enable hardware packet drop for all error packets.\n");
 	printf("  --enable-drop-en: enable per queue packet drop.\n");
 	printf("  --disable-rss: disable rss.\n");
 	printf("  --port-topology=<paired|chained|loop>: set port topology (paired "
@@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
 		{ "enable-hw-vlan-strip",       0, 0, 0 },
 		{ "enable-hw-vlan-extend",      0, 0, 0 },
 		{ "enable-hw-qinq-strip",       0, 0, 0 },
+		{ "enable-hw-drop-err-all",     0, 0, 0 },
 		{ "enable-drop-en",            0, 0, 0 },
 		{ "disable-rss",                0, 0, 0 },
 		{ "port-topology",              1, 0, 0 },
@@ -1283,6 +1285,11 @@ launch_args_parse(int argc, char** argv)
 				rmv_interrupt = 0;
 			if (!strcmp(lgopts[opt_idx].name, "flow-isolate-all"))
 				flow_isolate_all = 1;
+			if (!strcmp(lgopts[opt_idx].name,
+					"enable-hw-drop-err-all")) {
+				rx_err_pkt_drop_all = 1;
+			}
+
 			if (!strcmp(lgopts[opt_idx].name, "tx-offloads")) {
 				char *end = NULL;
 				n = strtoull(optarg, &end, 16);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index ccba71c07..c9e7397e6 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -359,6 +359,11 @@ uint8_t lsc_interrupt = 1; /* enabled by default */
  */
 uint8_t rmv_interrupt = 1; /* enabled by default */
 
+/*
+ * Drop all RX error packets on HW itself.
+ */
+uint8_t rx_err_pkt_drop_all = 0; /* disabled by default */
+
 uint8_t hot_plug = 0; /**< hotplug disabled by default. */
 
 /* After attach, port setup is called on event or by iterator */
@@ -3359,6 +3364,9 @@ init_port_config(void)
 		    (rte_eth_devices[pid].data->dev_flags &
 		     RTE_ETH_DEV_INTR_RMV))
 			port->dev_conf.intr_conf.rmv = 1;
+
+		if (rx_err_pkt_drop_all)
+			port->dev_conf.err_pkt_drop_conf.all = 1;
 	}
 }
 
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index c7e7e41a9..eab154ed4 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -314,6 +314,7 @@ extern uint8_t no_device_start; /**<set by "--disable-device-start" parameter */
 extern volatile int test_done; /* stop packet forwarding when set to 1. */
 extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-interrupt" parameter */
 extern uint8_t rmv_interrupt; /**< disabled by "--no-rmv-interrupt" parameter */
+extern uint8_t rx_err_pkt_drop_all; /**< enabled by "--enable-hw-drop-err-all" parameter */
 extern uint32_t event_print_mask;
 /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
 extern bool setup_on_probe_event; /**< disabled by port setup-on iterator */
diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
index e2539f693..20f2f8083 100644
--- a/doc/guides/testpmd_app_ug/run_app.rst
+++ b/doc/guides/testpmd_app_ug/run_app.rst
@@ -213,6 +213,10 @@ The command line options are:
 
     Enable hardware QINQ strip.
 
+*  ``--enable-hw-drop-err-all``
+
+    Enable hardware packet drop for any error packets
+
 *   ``--enable-drop-en``
 
     Enable per-queue packet drop for packets with no descriptors.
-- 
2.17.1


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 3/3 v3] app/testpmd: support hardware offload to drop error packets
  2020-10-09 13:13   ` [dpdk-dev] [PATCH 3/3 v3] app/testpmd: support hardware offload to drop error packets nipun.gupta
@ 2020-10-11  7:22     ` Asaf Penso
  0 siblings, 0 replies; 46+ messages in thread
From: Asaf Penso @ 2020-10-11  7:22 UTC (permalink / raw)
  To: nipun.gupta, dev
  Cc: NBU-Contact-Thomas Monjalon, ferruh.yigit, arybchenko,
	hemant.agrawal, sachin.saxena, rohit.raj, jerinjacobk, stephen



Regards,
Asaf Penso

>-----Original Message-----
>From: nipun.gupta@nxp.com <nipun.gupta@nxp.com>
>Sent: Friday, October 9, 2020 4:14 PM
>To: dev@dpdk.org
>Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
>ferruh.yigit@intel.com; arybchenko@solarflare.com;
>hemant.agrawal@nxp.com; sachin.saxena@nxp.com; rohit.raj@nxp.com;
>jerinjacobk@gmail.com; stephen@networkplumber.org; Asaf Penso
><asafp@nvidia.com>; Nipun Gupta <nipun.gupta@nxp.com>
>Subject: [PATCH 3/3 v3] app/testpmd: support hardware offload to drop error
>packets
>
>From: Nipun Gupta <nipun.gupta@nxp.com>
>
>With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
>capability, and separate DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL capability to
>drop all error packets in hardware, testpmd showcases this with a new added
>configuration option 'enable-hw-drop-err-all'.
>
>Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
Reviewed-By: Asaf Penso <asafp@nvidia.com>
>---
> app/test-pmd/parameters.c             | 7 +++++++
> app/test-pmd/testpmd.c                | 8 ++++++++
> app/test-pmd/testpmd.h                | 1 +
> doc/guides/testpmd_app_ug/run_app.rst | 4 ++++
> 4 files changed, 20 insertions(+)
>
>diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c index
>1ead59579..508612426 100644
>--- a/app/test-pmd/parameters.c
>+++ b/app/test-pmd/parameters.c
>@@ -142,6 +142,7 @@ usage(char* progname)
> 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
> 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
> 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
>+	printf("  --enable-hw-drop-err-all: enable hardware packet drop for
>+all error packets.\n");
> 	printf("  --enable-drop-en: enable per queue packet drop.\n");
> 	printf("  --disable-rss: disable rss.\n");
> 	printf("  --port-topology=<paired|chained|loop>: set port topology
>(paired "
>@@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
> 		{ "enable-hw-vlan-strip",       0, 0, 0 },
> 		{ "enable-hw-vlan-extend",      0, 0, 0 },
> 		{ "enable-hw-qinq-strip",       0, 0, 0 },
>+		{ "enable-hw-drop-err-all",     0, 0, 0 },
> 		{ "enable-drop-en",            0, 0, 0 },
> 		{ "disable-rss",                0, 0, 0 },
> 		{ "port-topology",              1, 0, 0 },
>@@ -1283,6 +1285,11 @@ launch_args_parse(int argc, char** argv)
> 				rmv_interrupt = 0;
> 			if (!strcmp(lgopts[opt_idx].name, "flow-isolate-all"))
> 				flow_isolate_all = 1;
>+			if (!strcmp(lgopts[opt_idx].name,
>+					"enable-hw-drop-err-all")) {
>+				rx_err_pkt_drop_all = 1;
>+			}
>+
> 			if (!strcmp(lgopts[opt_idx].name, "tx-offloads")) {
> 				char *end = NULL;
> 				n = strtoull(optarg, &end, 16);
>diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>ccba71c07..c9e7397e6 100644
>--- a/app/test-pmd/testpmd.c
>+++ b/app/test-pmd/testpmd.c
>@@ -359,6 +359,11 @@ uint8_t lsc_interrupt = 1; /* enabled by default */
>  */
> uint8_t rmv_interrupt = 1; /* enabled by default */
>
>+/*
>+ * Drop all RX error packets on HW itself.
>+ */
>+uint8_t rx_err_pkt_drop_all = 0; /* disabled by default */
>+
> uint8_t hot_plug = 0; /**< hotplug disabled by default. */
>
> /* After attach, port setup is called on event or by iterator */ @@ -3359,6
>+3364,9 @@ init_port_config(void)
> 		    (rte_eth_devices[pid].data->dev_flags &
> 		     RTE_ETH_DEV_INTR_RMV))
> 			port->dev_conf.intr_conf.rmv = 1;
>+
>+		if (rx_err_pkt_drop_all)
>+			port->dev_conf.err_pkt_drop_conf.all = 1;
> 	}
> }
>
>diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
>c7e7e41a9..eab154ed4 100644
>--- a/app/test-pmd/testpmd.h
>+++ b/app/test-pmd/testpmd.h
>@@ -314,6 +314,7 @@ extern uint8_t no_device_start; /**<set by "--disable-
>device-start" parameter */  extern volatile int test_done; /* stop packet
>forwarding when set to 1. */  extern uint8_t lsc_interrupt; /**< disabled by "-
>-no-lsc-interrupt" parameter */  extern uint8_t rmv_interrupt; /**< disabled
>by "--no-rmv-interrupt" parameter */
>+extern uint8_t rx_err_pkt_drop_all; /**< enabled by
>+"--enable-hw-drop-err-all" parameter */
> extern uint32_t event_print_mask;
> /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
>extern bool setup_on_probe_event; /**< disabled by port setup-on iterator
>*/ diff --git a/doc/guides/testpmd_app_ug/run_app.rst
>b/doc/guides/testpmd_app_ug/run_app.rst
>index e2539f693..20f2f8083 100644
>--- a/doc/guides/testpmd_app_ug/run_app.rst
>+++ b/doc/guides/testpmd_app_ug/run_app.rst
>@@ -213,6 +213,10 @@ The command line options are:
>
>     Enable hardware QINQ strip.
>
>+*  ``--enable-hw-drop-err-all``
>+
>+    Enable hardware packet drop for any error packets
>+
> *   ``--enable-drop-en``
>
>     Enable per-queue packet drop for packets with no descriptors.
>--
>2.17.1


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets
  2020-10-09 13:13 ` [dpdk-dev] [PATCH 1/3 v3] " nipun.gupta
  2020-10-09 13:13   ` [dpdk-dev] [PATCH 2/3 v3] net/dpaa: support RX offload for error packet drop nipun.gupta
  2020-10-09 13:13   ` [dpdk-dev] [PATCH 3/3 v3] app/testpmd: support hardware offload to drop error packets nipun.gupta
@ 2020-10-11 10:13   ` Jerin Jacob
  2020-10-11 21:41   ` Thomas Monjalon
  2020-10-12  8:01   ` Andrew Rybchenko
  4 siblings, 0 replies; 46+ messages in thread
From: Jerin Jacob @ 2020-10-11 10:13 UTC (permalink / raw)
  To: Nipun Gupta
  Cc: dpdk-dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko,
	Hemant Agrawal, Sachin Saxena, Rohit Raj, Stephen Hemminger,
	asafp

On Fri, Oct 9, 2020 at 6:43 PM <nipun.gupta@nxp.com> wrote:
>
> From: Nipun Gupta <nipun.gupta@nxp.com>
>
> This change adds a RX offload capability and configuration to
> enable hardware to drop the packets in case of any error in the
> packets 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>
> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> ---
>
> v3:
>  - Add additional rx_err_drop_offload_capa, which is specific
>    capability flag for RX packets error drop offload. Currently
>    only 'all' error packet drops are enabled, but can be extended
>    to provide capability to drop any specific errors like L1 FCS,
>    L3 Checksum etc.
>  - Added separate config structure to enable the drop configuration.
>  - Updated doc with the new updated option in testbbdev (patch 3/3)
>
> v2:
>  - Add support in DPAA1 driver (patch 2/3)
>  - Add support and config parameter in testpmd (patch 3/3)
>
>  lib/librte_ethdev/rte_ethdev.c |  1 +
>  lib/librte_ethdev/rte_ethdev.h | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+)


The patch looks good to me.

1) Missed to update port_offload_cap_display() for new oflload in
/app/test-pmd/config.c
2) Please update doc/guides/nics/features.rst for this new offload

See [1] as a reference.
[1]
commit 28f6a3b88d23a348eeffd2bac79b2e0819d3b4e9
Author: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Date:   Tue Oct 2 16:21:41 2018 +0530

    ethdev: support SCTP Rx checksum offload

    Added SCTP Rx checksum offload support

    Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
    Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>



>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 48d1333b1..be25e947e 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -128,6 +128,7 @@ static const struct {
>         RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
>         RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
>         RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
> +       RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
>  };
>
>  #undef RTE_RX_OFFLOAD_BIT2STR
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d2bf74f12..cb968d38a 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1194,6 +1194,16 @@ struct rte_intr_conf {
>         uint32_t rmv:1;
>  };
>
> +/**
> + * A structure used to enable/disable error packet drop on RX.
> + */
> +struct rte_rx_err_pkt_drop_conf {
> +       /** enable/disable all RX error packet drop.
> +        * 0 (default) - disable, 1 enable
> +        */
> +       uint32_t all:1;
> +};
> +
>  /**
>   * A structure used to configure an Ethernet port.
>   * Depending upon the RX multi-queue mode, extra advanced
> @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
>         uint32_t dcb_capability_en;
>         struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
>         struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> +       struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> +       /**< RX error packet drop configuration. */
>  };
>
>  /**
> @@ -1260,6 +1272,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 | \
> @@ -1274,6 +1287,13 @@ struct rte_eth_conf {
>   * mentioned in rte_rx_offload_names in rte_ethdev.c file.
>   */
>
> +/**
> + * RX Error Drop offload config/capabilities of a device. These
> + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
> + * is supported by the device.
> + */
> +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL                0x00000001
> +
>  /**
>   * TX offload capabilities of a device.
>   */
> @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
>         /**< Device per-queue RX offload capabilities. */
>         uint64_t tx_queue_offload_capa;
>         /**< Device per-queue TX offload capabilities. */
> +       uint64_t rx_err_drop_offload_capa;
> +       /**< RX error packet drop offload capabilities. */
>         uint16_t reta_size;
>         /**< Device redirection table size, the total number of entries. */
>         uint8_t hash_key_size; /**< Hash key size in bytes */
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets
  2020-10-09 13:13 ` [dpdk-dev] [PATCH 1/3 v3] " nipun.gupta
                     ` (2 preceding siblings ...)
  2020-10-11 10:13   ` [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx " Jerin Jacob
@ 2020-10-11 21:41   ` Thomas Monjalon
  2020-10-12  5:40     ` Nipun Gupta
  2020-10-12  8:01   ` Andrew Rybchenko
  4 siblings, 1 reply; 46+ messages in thread
From: Thomas Monjalon @ 2020-10-11 21:41 UTC (permalink / raw)
  To: Nipun Gupta
  Cc: dev, ferruh.yigit, arybchenko, hemant.agrawal, sachin.saxena,
	rohit.raj, jerinjacobk, stephen, asafp

Hi,

The configuration of this feature is not clear to me.
Please see the comments below.

09/10/2020 15:13, nipun.gupta@nxp.com:
> From: Nipun Gupta <nipun.gupta@nxp.com>
> 
> This change adds a RX offload capability and configuration to
> enable hardware to drop the packets in case of any error in the
> packets 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>
> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> ---
[...]
> +/**
> + * A structure used to enable/disable error packet drop on RX.

RX -> Rx
same for other occurences below

> + */
> +struct rte_rx_err_pkt_drop_conf {

The name should start with rte_eth_

> +	/** enable/disable all RX error packet drop.
> +	 * 0 (default) - disable, 1 enable
> +	 */
> +	uint32_t all:1;
> +};

I don't understand the meaning of this struct.
Is it just one bit to drop packets having an error?
How do you determine what is an error?

[...]
> @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
>  	uint32_t dcb_capability_en;
>  	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
>  	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> +	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> +	/**< RX error packet drop configuration. */

Why a per-port configuration is needed in addition of the per-queue offload?

[...]
> @@ -1260,6 +1272,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

New offload names should starte with RTE_ prefix.


> +/**
> + * RX Error Drop offload config/capabilities of a device. These
> + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
> + * is supported by the device.
> + */
> +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001

I don't understand the meaning.

> @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
>  	/**< Device per-queue RX offload capabilities. */
>  	uint64_t tx_queue_offload_capa;
>  	/**< Device per-queue TX offload capabilities. */
> +	uint64_t rx_err_drop_offload_capa;
> +	/**< RX error packet drop offload capabilities. */

Why adding a new field here instead of reporting in rx_offload_capa?




^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets
  2020-10-11 21:41   ` Thomas Monjalon
@ 2020-10-12  5:40     ` Nipun Gupta
  2020-10-13  7:22       ` Nipun Gupta
  0 siblings, 1 reply; 46+ messages in thread
From: Nipun Gupta @ 2020-10-12  5:40 UTC (permalink / raw)
  To: Thomas Monjalon, jerinjacobk
  Cc: dev, ferruh.yigit, arybchenko, Hemant Agrawal, Sachin Saxena,
	Rohit Raj, stephen, asafp



> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, October 12, 2020 3:12 AM
> To: Nipun Gupta <nipun.gupta@nxp.com>
> Cc: dev@dpdk.org; ferruh.yigit@intel.com; arybchenko@solarflare.com;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> packets
> 
> Hi,
> 
> The configuration of this feature is not clear to me.
> Please see the comments below.
> 
> 09/10/2020 15:13, nipun.gupta@nxp.com:
> > From: Nipun Gupta <nipun.gupta@nxp.com>
> >
> > This change adds a RX offload capability and configuration to
> > enable hardware to drop the packets in case of any error in the
> > packets 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>
> > Reviewed-by: Asaf Penso <asafp@nvidia.com>
> > ---
> [...]
> > +/**
> > + * A structure used to enable/disable error packet drop on RX.
> 
> RX -> Rx
> same for other occurences below

Okay.

> 
> > + */
> > +struct rte_rx_err_pkt_drop_conf {
> 
> The name should start with rte_eth_
> 
> > +	/** enable/disable all RX error packet drop.
> > +	 * 0 (default) - disable, 1 enable
> > +	 */
> > +	uint32_t all:1;
> > +};
> 
> I don't understand the meaning of this struct.
> Is it just one bit to drop packets having an error?
> How do you determine what is an error?

The error packets which are dropped on the hardware can be from
various reasons, like L1 FCS, L3 CSUM, L4 CSUM or HW can be configured
to drop ALL (or any) of the error packets received on the interface.

Currently DPAA drivers support drop of ALL the error packets, but does
not support configuration of packet drops on basis of separate error types
like dropping only L2 CSUM, L3 CSUM. So this patch is supporting only drop
of 'ALL' the errors packets received on HW.

Marvell supports drop on separate error types, thus Jerin suggested to have
this structure so that it opens the room for adding separate configuration
to drop packets on basis of separate error types.

Jerin,
maybe you can add more on this.

> 
> [...]
> > @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
> >  	uint32_t dcb_capability_en;
> >  	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
> >  	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> > +	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> > +	/**< RX error packet drop configuration. */
> 
> Why a per-port configuration is needed in addition of the per-queue offload?
> 
> [...]
> > @@ -1260,6 +1272,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
> 
> New offload names should starte with RTE_ prefix.

Okay

> 
> 
> > +/**
> > + * RX Error Drop offload config/capabilities of a device. These
> > + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
> > + * is supported by the device.
> > + */
> > +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001
> 
> I don't understand the meaning.

For DPAA drivers there is only one offload required, but once separate offloads
are added to drop packets on basis of separate errors, then it can bloat up the
offload capabilities. So we added new flags ' rte_rx_err_pkt_drop_conf ' instead
of adding this onto 'rx_offload_capa'.

There is only one flag ' DEV_RX_OFFLOAD_ERR_PKT_DROP' added in 'rx_offload_capa'
and if this flag is enabled, one or more of the flag in ' DEV_RX_ERR_PKT_DROP_OFFLOAD...'
can be enabled (currently only DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL) being supported
Others like:
- DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS
- DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM
- DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM etc.
Can be added once any reference driver is supporting this.

P.S. I can add these flags in this patch but without any ref driver support.

> 
> > @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
> >  	/**< Device per-queue RX offload capabilities. */
> >  	uint64_t tx_queue_offload_capa;
> >  	/**< Device per-queue TX offload capabilities. */
> > +	uint64_t rx_err_drop_offload_capa;
> > +	/**< RX error packet drop offload capabilities. */
> 
> Why adding a new field here instead of reporting in rx_offload_capa?

This was part of v2 where flag was added in rx_offload_capa.

Both ways (v2 and v3 versions) seems to have pros and cons:
- Adding only in the rx_offload_capa' seems simpler to implement and use
- But it shall lead to increase in the offload capability types when packet
  drop configuration is added on individual error types.

Regards,
Nipun

> 
> 


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets
  2020-10-09 13:13 ` [dpdk-dev] [PATCH 1/3 v3] " nipun.gupta
                     ` (3 preceding siblings ...)
  2020-10-11 21:41   ` Thomas Monjalon
@ 2020-10-12  8:01   ` Andrew Rybchenko
  2020-10-12 11:30     ` Nipun Gupta
  4 siblings, 1 reply; 46+ messages in thread
From: Andrew Rybchenko @ 2020-10-12  8:01 UTC (permalink / raw)
  To: nipun.gupta, dev
  Cc: thomas, ferruh.yigit, arybchenko, hemant.agrawal, sachin.saxena,
	rohit.raj, jerinjacobk, stephen, asafp

On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
> From: Nipun Gupta <nipun.gupta@nxp.com>
> 
> This change adds a RX offload capability and configuration to
> enable hardware to drop the packets in case of any error in the
> packets 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>
> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> ---
> 
> v3:
>  - Add additional rx_err_drop_offload_capa, which is specific
>    capability flag for RX packets error drop offload. Currently
>    only 'all' error packet drops are enabled, but can be extended
>    to provide capability to drop any specific errors like L1 FCS,
>    L3 Checksum etc.
>  - Added separate config structure to enable the drop configuration.
>  - Updated doc with the new updated option in testbbdev (patch 3/3)
> 
> v2:
>  - Add support in DPAA1 driver (patch 2/3)
>  - Add support and config parameter in testpmd (patch 3/3)
> 
>  lib/librte_ethdev/rte_ethdev.c |  1 +
>  lib/librte_ethdev/rte_ethdev.h | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 48d1333b1..be25e947e 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -128,6 +128,7 @@ static const struct {
>  	RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
>  	RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
>  	RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
> +	RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
>  };
>  
>  #undef RTE_RX_OFFLOAD_BIT2STR
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d2bf74f12..cb968d38a 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1194,6 +1194,16 @@ struct rte_intr_conf {
>  	uint32_t rmv:1;
>  };
>  
> +/**
> + * A structure used to enable/disable error packet drop on RX.
> + */
> +struct rte_rx_err_pkt_drop_conf {
> +	/** enable/disable all RX error packet drop.
> +	 * 0 (default) - disable, 1 enable
> +	 */
> +	uint32_t all:1;

"all" is bad. It should be granular and in the same terms
as mask in dev_info capability.

> +};
> +
>  /**
>   * A structure used to configure an Ethernet port.
>   * Depending upon the RX multi-queue mode, extra advanced
> @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
>  	uint32_t dcb_capability_en;
>  	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
>  	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> +	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> +	/**< RX error packet drop configuration. */

Why not per queue?

>  };
>  
>  /**
> @@ -1260,6 +1272,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 | \
> @@ -1274,6 +1287,13 @@ struct rte_eth_conf {
>   * mentioned in rte_rx_offload_names in rte_ethdev.c file.
>   */
>  
> +/**
> + * RX Error Drop offload config/capabilities of a device. These
> + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
> + * is supported by the device.
> + */
> +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001
> +

I strictly dislike "all". It will always be bad defined.
It must be granular.

>  /**
>   * TX offload capabilities of a device.
>   */
> @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
>  	/**< Device per-queue RX offload capabilities. */
>  	uint64_t tx_queue_offload_capa;
>  	/**< Device per-queue TX offload capabilities. */
> +	uint64_t rx_err_drop_offload_capa;
> +	/**< RX error packet drop offload capabilities. */
>  	uint16_t reta_size;
>  	/**< Device redirection table size, the total number of entries. */
>  	uint8_t hash_key_size; /**< Hash key size in bytes */
> 


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets
  2020-10-12  8:01   ` Andrew Rybchenko
@ 2020-10-12 11:30     ` Nipun Gupta
  2020-10-12 12:22       ` Andrew Rybchenko
  0 siblings, 1 reply; 46+ messages in thread
From: Nipun Gupta @ 2020-10-12 11:30 UTC (permalink / raw)
  To: Andrew Rybchenko, dev
  Cc: thomas, ferruh.yigit, arybchenko, Hemant Agrawal, Sachin Saxena,
	Rohit Raj, jerinjacobk, stephen, asafp



> -----Original Message-----
> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> Sent: Monday, October 12, 2020 1:32 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; arybchenko@solarflare.com;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> packets
> 
> On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
> > From: Nipun Gupta <nipun.gupta@nxp.com>
> >
> > This change adds a RX offload capability and configuration to
> > enable hardware to drop the packets in case of any error in the
> > packets 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>
> > Reviewed-by: Asaf Penso <asafp@nvidia.com>
> > ---
> >
> > v3:
> >  - Add additional rx_err_drop_offload_capa, which is specific
> >    capability flag for RX packets error drop offload. Currently
> >    only 'all' error packet drops are enabled, but can be extended
> >    to provide capability to drop any specific errors like L1 FCS,
> >    L3 Checksum etc.
> >  - Added separate config structure to enable the drop configuration.
> >  - Updated doc with the new updated option in testbbdev (patch 3/3)
> >
> > v2:
> >  - Add support in DPAA1 driver (patch 2/3)
> >  - Add support and config parameter in testpmd (patch 3/3)
> >
> >  lib/librte_ethdev/rte_ethdev.c |  1 +
> >  lib/librte_ethdev/rte_ethdev.h | 22 ++++++++++++++++++++++
> >  2 files changed, 23 insertions(+)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > index 48d1333b1..be25e947e 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -128,6 +128,7 @@ static const struct {
> >  	RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
> >  	RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
> >  	RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
> > +	RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
> >  };
> >
> >  #undef RTE_RX_OFFLOAD_BIT2STR
> > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > index d2bf74f12..cb968d38a 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -1194,6 +1194,16 @@ struct rte_intr_conf {
> >  	uint32_t rmv:1;
> >  };
> >
> > +/**
> > + * A structure used to enable/disable error packet drop on RX.
> > + */
> > +struct rte_rx_err_pkt_drop_conf {
> > +	/** enable/disable all RX error packet drop.
> > +	 * 0 (default) - disable, 1 enable
> > +	 */
> > +	uint32_t all:1;
> 
> "all" is bad. It should be granular and in the same terms
> as mask in dev_info capability.

Consider that application do not want to receive any error packets as it will drop all
the error packets after increasing the error counter maintainer in the application.
If same functionality can be done by the hardware where hardware maintains the error
statistics, then why not have the same?
When hardware does parsing of packet headers and found error during that parsing,
this is to say to HW - hey drop any such error packets which you found during parsing
and just increase specific error counter (if present).

I agree, granular shall also be added, but as our driver does not support them, so I did
not add those bits here.

> 
> > +};
> > +
> >  /**
> >   * A structure used to configure an Ethernet port.
> >   * Depending upon the RX multi-queue mode, extra advanced
> > @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
> >  	uint32_t dcb_capability_en;
> >  	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
> >  	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> > +	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> > +	/**< RX error packet drop configuration. */
> 
> Why not per queue?

We do not support per queue configuration for error packet drop, but only
on ethernet basis.

> 
> >  };
> >
> >  /**
> > @@ -1260,6 +1272,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 | \
> > @@ -1274,6 +1287,13 @@ struct rte_eth_conf {
> >   * mentioned in rte_rx_offload_names in rte_ethdev.c file.
> >   */
> >
> > +/**
> > + * RX Error Drop offload config/capabilities of a device. These
> > + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
> > + * is supported by the device.
> > + */
> > +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001
> > +
> 
> I strictly dislike "all". It will always be bad defined.
> It must be granular.
> 
> >  /**
> >   * TX offload capabilities of a device.
> >   */
> > @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
> >  	/**< Device per-queue RX offload capabilities. */
> >  	uint64_t tx_queue_offload_capa;
> >  	/**< Device per-queue TX offload capabilities. */
> > +	uint64_t rx_err_drop_offload_capa;
> > +	/**< RX error packet drop offload capabilities. */
> >  	uint16_t reta_size;
> >  	/**< Device redirection table size, the total number of entries. */
> >  	uint8_t hash_key_size; /**< Hash key size in bytes */
> >


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets
  2020-10-12 11:30     ` Nipun Gupta
@ 2020-10-12 12:22       ` Andrew Rybchenko
  2020-10-12 12:53         ` Nipun Gupta
  2020-10-13  7:21         ` Andrew Rybchenko
  0 siblings, 2 replies; 46+ messages in thread
From: Andrew Rybchenko @ 2020-10-12 12:22 UTC (permalink / raw)
  To: Nipun Gupta, dev
  Cc: thomas, ferruh.yigit, arybchenko, Hemant Agrawal, Sachin Saxena,
	Rohit Raj, jerinjacobk, stephen, asafp

On 10/12/20 2:30 PM, Nipun Gupta wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
>> Sent: Monday, October 12, 2020 1:32 PM
>> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
>> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; arybchenko@solarflare.com;
>> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
>> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
>> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
>> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
>> packets
>>
>> On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
>>> From: Nipun Gupta <nipun.gupta@nxp.com>
>>>
>>> This change adds a RX offload capability and configuration to
>>> enable hardware to drop the packets in case of any error in the
>>> packets 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>
>>> Reviewed-by: Asaf Penso <asafp@nvidia.com>
>>> ---
>>>
>>> v3:
>>>  - Add additional rx_err_drop_offload_capa, which is specific
>>>    capability flag for RX packets error drop offload. Currently
>>>    only 'all' error packet drops are enabled, but can be extended
>>>    to provide capability to drop any specific errors like L1 FCS,
>>>    L3 Checksum etc.
>>>  - Added separate config structure to enable the drop configuration.
>>>  - Updated doc with the new updated option in testbbdev (patch 3/3)
>>>
>>> v2:
>>>  - Add support in DPAA1 driver (patch 2/3)
>>>  - Add support and config parameter in testpmd (patch 3/3)
>>>
>>>  lib/librte_ethdev/rte_ethdev.c |  1 +
>>>  lib/librte_ethdev/rte_ethdev.h | 22 ++++++++++++++++++++++
>>>  2 files changed, 23 insertions(+)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>> index 48d1333b1..be25e947e 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -128,6 +128,7 @@ static const struct {
>>>  	RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
>>>  	RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
>>>  	RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
>>> +	RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
>>>  };
>>>
>>>  #undef RTE_RX_OFFLOAD_BIT2STR
>>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>> index d2bf74f12..cb968d38a 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>> @@ -1194,6 +1194,16 @@ struct rte_intr_conf {
>>>  	uint32_t rmv:1;
>>>  };
>>>
>>> +/**
>>> + * A structure used to enable/disable error packet drop on RX.
>>> + */
>>> +struct rte_rx_err_pkt_drop_conf {
>>> +	/** enable/disable all RX error packet drop.
>>> +	 * 0 (default) - disable, 1 enable
>>> +	 */
>>> +	uint32_t all:1;
>>
>> "all" is bad. It should be granular and in the same terms
>> as mask in dev_info capability.
> 
> Consider that application do not want to receive any error packets as it will drop all
> the error packets after increasing the error counter maintainer in the application.
> If same functionality can be done by the hardware where hardware maintains the error
> statistics, then why not have the same?
> When hardware does parsing of packet headers and found error during that parsing,
> this is to say to HW - hey drop any such error packets which you found during parsing
> and just increase specific error counter (if present).
> 
> I agree, granular shall also be added, but as our driver does not support them, so I did
> not add those bits here.

There is always precise definition behind "all".
You can report it in dev_info, an application can get it and
use as is (all reported capability bits) in configuration.

> 
>>
>>> +};
>>> +
>>>  /**
>>>   * A structure used to configure an Ethernet port.
>>>   * Depending upon the RX multi-queue mode, extra advanced
>>> @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
>>>  	uint32_t dcb_capability_en;
>>>  	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
>>>  	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
>>> +	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
>>> +	/**< RX error packet drop configuration. */
>>
>> Why not per queue?
> 
> We do not support per queue configuration for error packet drop, but only
> on ethernet basis.
> 
>>
>>>  };
>>>
>>>  /**
>>> @@ -1260,6 +1272,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 | \
>>> @@ -1274,6 +1287,13 @@ struct rte_eth_conf {
>>>   * mentioned in rte_rx_offload_names in rte_ethdev.c file.
>>>   */
>>>
>>> +/**
>>> + * RX Error Drop offload config/capabilities of a device. These
>>> + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
>>> + * is supported by the device.
>>> + */
>>> +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001
>>> +
>>
>> I strictly dislike "all". It will always be bad defined.
>> It must be granular.
>>
>>>  /**
>>>   * TX offload capabilities of a device.
>>>   */
>>> @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
>>>  	/**< Device per-queue RX offload capabilities. */
>>>  	uint64_t tx_queue_offload_capa;
>>>  	/**< Device per-queue TX offload capabilities. */
>>> +	uint64_t rx_err_drop_offload_capa;
>>> +	/**< RX error packet drop offload capabilities. */
>>>  	uint16_t reta_size;
>>>  	/**< Device redirection table size, the total number of entries. */
>>>  	uint8_t hash_key_size; /**< Hash key size in bytes */
>>>
> 


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets
  2020-10-12 12:22       ` Andrew Rybchenko
@ 2020-10-12 12:53         ` Nipun Gupta
  2020-10-13  7:21         ` Andrew Rybchenko
  1 sibling, 0 replies; 46+ messages in thread
From: Nipun Gupta @ 2020-10-12 12:53 UTC (permalink / raw)
  To: Andrew Rybchenko, dev
  Cc: thomas, ferruh.yigit, arybchenko, Hemant Agrawal, Sachin Saxena,
	Rohit Raj, jerinjacobk, stephen, asafp



> -----Original Message-----
> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> Sent: Monday, October 12, 2020 5:53 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; arybchenko@solarflare.com;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> packets
> 
> On 10/12/20 2:30 PM, Nipun Gupta wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> >> Sent: Monday, October 12, 2020 1:32 PM
> >> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> >> Cc: thomas@monjalon.net; ferruh.yigit@intel.com;
> arybchenko@solarflare.com;
> >> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> >> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> >> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
> >> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> >> packets
> >>
> >> On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
> >>> From: Nipun Gupta <nipun.gupta@nxp.com>
> >>>
> >>> This change adds a RX offload capability and configuration to
> >>> enable hardware to drop the packets in case of any error in the
> >>> packets 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>
> >>> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> >>> ---
> >>>
> >>> v3:
> >>>  - Add additional rx_err_drop_offload_capa, which is specific
> >>>    capability flag for RX packets error drop offload. Currently
> >>>    only 'all' error packet drops are enabled, but can be extended
> >>>    to provide capability to drop any specific errors like L1 FCS,
> >>>    L3 Checksum etc.
> >>>  - Added separate config structure to enable the drop configuration.
> >>>  - Updated doc with the new updated option in testbbdev (patch 3/3)
> >>>
> >>> v2:
> >>>  - Add support in DPAA1 driver (patch 2/3)
> >>>  - Add support and config parameter in testpmd (patch 3/3)
> >>>
> >>>  lib/librte_ethdev/rte_ethdev.c |  1 +
> >>>  lib/librte_ethdev/rte_ethdev.h | 22 ++++++++++++++++++++++
> >>>  2 files changed, 23 insertions(+)
> >>>
> >>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> >>> index 48d1333b1..be25e947e 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.c
> >>> +++ b/lib/librte_ethdev/rte_ethdev.c
> >>> @@ -128,6 +128,7 @@ static const struct {
> >>>  	RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
> >>>  	RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
> >>>  	RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
> >>> +	RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
> >>>  };
> >>>
> >>>  #undef RTE_RX_OFFLOAD_BIT2STR
> >>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> >>> index d2bf74f12..cb968d38a 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>> @@ -1194,6 +1194,16 @@ struct rte_intr_conf {
> >>>  	uint32_t rmv:1;
> >>>  };
> >>>
> >>> +/**
> >>> + * A structure used to enable/disable error packet drop on RX.
> >>> + */
> >>> +struct rte_rx_err_pkt_drop_conf {
> >>> +	/** enable/disable all RX error packet drop.
> >>> +	 * 0 (default) - disable, 1 enable
> >>> +	 */
> >>> +	uint32_t all:1;
> >>
> >> "all" is bad. It should be granular and in the same terms
> >> as mask in dev_info capability.
> >
> > Consider that application do not want to receive any error packets as it will
> drop all
> > the error packets after increasing the error counter maintainer in the
> application.
> > If same functionality can be done by the hardware where hardware maintains
> the error
> > statistics, then why not have the same?
> > When hardware does parsing of packet headers and found error during that
> parsing,
> > this is to say to HW - hey drop any such error packets which you found during
> parsing
> > and just increase specific error counter (if present).
> >
> > I agree, granular shall also be added, but as our driver does not support them,
> so I did
> > not add those bits here.
> 
> There is always precise definition behind "all".
> You can report it in dev_info, an application can get it and
> use as is (all reported capability bits) in configuration.

Maybe Ill define individual error also (like l3_csum, l4_csum etc), and then 'all'
will make more sense. It will indicate that all the bits are on irrespective of
individual bits and can also be used in case if offload of individual error packet
drops type is not supported, but dropping all of them can be supported.

> 
> >
> >>
> >>> +};
> >>> +
> >>>  /**
> >>>   * A structure used to configure an Ethernet port.
> >>>   * Depending upon the RX multi-queue mode, extra advanced
> >>> @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
> >>>  	uint32_t dcb_capability_en;
> >>>  	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
> >>>  	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> >>> +	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> >>> +	/**< RX error packet drop configuration. */
> >>
> >> Why not per queue?
> >
> > We do not support per queue configuration for error packet drop, but only
> > on ethernet basis.
> >
> >>
> >>>  };
> >>>
> >>>  /**
> >>> @@ -1260,6 +1272,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 | \
> >>> @@ -1274,6 +1287,13 @@ struct rte_eth_conf {
> >>>   * mentioned in rte_rx_offload_names in rte_ethdev.c file.
> >>>   */
> >>>
> >>> +/**
> >>> + * RX Error Drop offload config/capabilities of a device. These
> >>> + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
> >>> + * is supported by the device.
> >>> + */
> >>> +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001
> >>> +
> >>
> >> I strictly dislike "all". It will always be bad defined.
> >> It must be granular.
> >>
> >>>  /**
> >>>   * TX offload capabilities of a device.
> >>>   */
> >>> @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
> >>>  	/**< Device per-queue RX offload capabilities. */
> >>>  	uint64_t tx_queue_offload_capa;
> >>>  	/**< Device per-queue TX offload capabilities. */
> >>> +	uint64_t rx_err_drop_offload_capa;
> >>> +	/**< RX error packet drop offload capabilities. */
> >>>  	uint16_t reta_size;
> >>>  	/**< Device redirection table size, the total number of entries. */
> >>>  	uint8_t hash_key_size; /**< Hash key size in bytes */
> >>>
> >


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets
  2020-10-12 12:22       ` Andrew Rybchenko
  2020-10-12 12:53         ` Nipun Gupta
@ 2020-10-13  7:21         ` Andrew Rybchenko
  2020-10-13  7:36           ` Nipun Gupta
  1 sibling, 1 reply; 46+ messages in thread
From: Andrew Rybchenko @ 2020-10-13  7:21 UTC (permalink / raw)
  To: Nipun Gupta, dev
  Cc: thomas, ferruh.yigit, Hemant Agrawal, Sachin Saxena, Rohit Raj,
	jerinjacobk, stephen, asafp

On 10/12/20 3:22 PM, Andrew Rybchenko wrote:
> On 10/12/20 2:30 PM, Nipun Gupta wrote:
>>> -----Original Message-----
>>> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
>>> Sent: Monday, October 12, 2020 1:32 PM
>>> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
>>> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; arybchenko@solarflare.com;
>>> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
>>> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
>>> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
>>> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
>>> packets
>>>
>>> On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
>>>> From: Nipun Gupta <nipun.gupta@nxp.com>
>>>>
>>>> This change adds a RX offload capability and configuration to
>>>> enable hardware to drop the packets in case of any error in the
>>>> packets 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>
>>>> Reviewed-by: Asaf Penso <asafp@nvidia.com>

Thinking a bit more about it I agree with Thomas idea that
it should be flow API based solution in fact.
Drop is just a one of possible actions to be done with
packets with bad checksum on one or another layer.
Such packets could be redirected to a slow path
(dedicated queue or port ID (PF, VF)).
It is just a missing feature in various layer
pattern match to say if we want to proceed with packets
with only good or only bad chehcksum (or we don't care
as we do right now). Exact match for checksums is hardly
useful except UDP with zero checksum case.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets
  2020-10-12  5:40     ` Nipun Gupta
@ 2020-10-13  7:22       ` Nipun Gupta
  0 siblings, 0 replies; 46+ messages in thread
From: Nipun Gupta @ 2020-10-13  7:22 UTC (permalink / raw)
  To: Thomas Monjalon, jerinjacobk
  Cc: dev, ferruh.yigit, arybchenko, Hemant Agrawal, Sachin Saxena,
	Rohit Raj, stephen, asafp

Hi Thomas and Jerin,

There seems two views:
1. Use of existing rx_offload_capa, to have additional error packet drop offload capabilities. This is showcased in v2, and also suggested by Thomas.
2. Adding additional offload capability flag types and configuration structure for error packet drop configuration as suggested by Jerin, which is part of v3.

Please let me know which one shall I continue with for the next spin.

Thanks,
Nipun

> -----Original Message-----
> From: Nipun Gupta
> Sent: Monday, October 12, 2020 11:11 AM
> To: Thomas Monjalon <thomas@monjalon.net>; jerinjacobk@gmail.com
> Cc: dev@dpdk.org; ferruh.yigit@intel.com; arybchenko@solarflare.com;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> stephen@networkplumber.org; asafp@nvidia.com
> Subject: RE: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> packets
> 
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Monday, October 12, 2020 3:12 AM
> > To: Nipun Gupta <nipun.gupta@nxp.com>
> > Cc: dev@dpdk.org; ferruh.yigit@intel.com; arybchenko@solarflare.com;
> > Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> > <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> > jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
> > Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> > packets
> >
> > Hi,
> >
> > The configuration of this feature is not clear to me.
> > Please see the comments below.
> >
> > 09/10/2020 15:13, nipun.gupta@nxp.com:
> > > From: Nipun Gupta <nipun.gupta@nxp.com>
> > >
> > > This change adds a RX offload capability and configuration to
> > > enable hardware to drop the packets in case of any error in the
> > > packets 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>
> > > Reviewed-by: Asaf Penso <asafp@nvidia.com>
> > > ---
> > [...]
> > > +/**
> > > + * A structure used to enable/disable error packet drop on RX.
> >
> > RX -> Rx
> > same for other occurences below
> 
> Okay.
> 
> >
> > > + */
> > > +struct rte_rx_err_pkt_drop_conf {
> >
> > The name should start with rte_eth_
> >
> > > +	/** enable/disable all RX error packet drop.
> > > +	 * 0 (default) - disable, 1 enable
> > > +	 */
> > > +	uint32_t all:1;
> > > +};
> >
> > I don't understand the meaning of this struct.
> > Is it just one bit to drop packets having an error?
> > How do you determine what is an error?
> 
> The error packets which are dropped on the hardware can be from
> various reasons, like L1 FCS, L3 CSUM, L4 CSUM or HW can be configured
> to drop ALL (or any) of the error packets received on the interface.
> 
> Currently DPAA drivers support drop of ALL the error packets, but does
> not support configuration of packet drops on basis of separate error types
> like dropping only L2 CSUM, L3 CSUM. So this patch is supporting only drop
> of 'ALL' the errors packets received on HW.
> 
> Marvell supports drop on separate error types, thus Jerin suggested to have
> this structure so that it opens the room for adding separate configuration
> to drop packets on basis of separate error types.
> 
> Jerin,
> maybe you can add more on this.
> 
> >
> > [...]
> > > @@ -1236,6 +1246,8 @@ struct rte_eth_conf {
> > >  	uint32_t dcb_capability_en;
> > >  	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
> > >  	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> > > +	struct rte_rx_err_pkt_drop_conf err_pkt_drop_conf;
> > > +	/**< RX error packet drop configuration. */
> >
> > Why a per-port configuration is needed in addition of the per-queue offload?
> >
> > [...]
> > > @@ -1260,6 +1272,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
> >
> > New offload names should starte with RTE_ prefix.
> 
> Okay
> 
> >
> >
> > > +/**
> > > + * RX Error Drop offload config/capabilities of a device. These
> > > + * are valid only when RX capability DEV_RX_OFFLOAD_ERR_PKT_DROP
> > > + * is supported by the device.
> > > + */
> > > +#define DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x00000001
> >
> > I don't understand the meaning.
> 
> For DPAA drivers there is only one offload required, but once separate offloads
> are added to drop packets on basis of separate errors, then it can bloat up the
> offload capabilities. So we added new flags ' rte_rx_err_pkt_drop_conf ' instead
> of adding this onto 'rx_offload_capa'.
> 
> There is only one flag ' DEV_RX_OFFLOAD_ERR_PKT_DROP' added in
> 'rx_offload_capa'
> and if this flag is enabled, one or more of the flag in '
> DEV_RX_ERR_PKT_DROP_OFFLOAD...'
> can be enabled (currently only DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL) being
> supported
> Others like:
> - DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS
> - DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM
> - DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM etc.
> Can be added once any reference driver is supporting this.
> 
> P.S. I can add these flags in this patch but without any ref driver support.
> 
> >
> > > @@ -1411,6 +1431,8 @@ struct rte_eth_dev_info {
> > >  	/**< Device per-queue RX offload capabilities. */
> > >  	uint64_t tx_queue_offload_capa;
> > >  	/**< Device per-queue TX offload capabilities. */
> > > +	uint64_t rx_err_drop_offload_capa;
> > > +	/**< RX error packet drop offload capabilities. */
> >
> > Why adding a new field here instead of reporting in rx_offload_capa?
> 
> This was part of v2 where flag was added in rx_offload_capa.
> 
> Both ways (v2 and v3 versions) seems to have pros and cons:
> - Adding only in the rx_offload_capa' seems simpler to implement and use
> - But it shall lead to increase in the offload capability types when packet
>   drop configuration is added on individual error types.
> 
> Regards,
> Nipun
> 
> >
> >


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets
  2020-10-13  7:21         ` Andrew Rybchenko
@ 2020-10-13  7:36           ` Nipun Gupta
  2020-10-13  7:51             ` Andrew Rybchenko
  0 siblings, 1 reply; 46+ messages in thread
From: Nipun Gupta @ 2020-10-13  7:36 UTC (permalink / raw)
  To: Andrew Rybchenko, dev
  Cc: thomas, ferruh.yigit, Hemant Agrawal, Sachin Saxena, Rohit Raj,
	jerinjacobk, stephen, asafp



> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Tuesday, October 13, 2020 12:52 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>; Rohit
> Raj <rohit.raj@nxp.com>; jerinjacobk@gmail.com;
> stephen@networkplumber.org; asafp@nvidia.com
> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> packets
> 
> On 10/12/20 3:22 PM, Andrew Rybchenko wrote:
> > On 10/12/20 2:30 PM, Nipun Gupta wrote:
> >>> -----Original Message-----
> >>> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> >>> Sent: Monday, October 12, 2020 1:32 PM
> >>> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> >>> Cc: thomas@monjalon.net; ferruh.yigit@intel.com;
> arybchenko@solarflare.com;
> >>> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> >>> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> >>> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
> >>> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> >>> packets
> >>>
> >>> On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
> >>>> From: Nipun Gupta <nipun.gupta@nxp.com>
> >>>>
> >>>> This change adds a RX offload capability and configuration to
> >>>> enable hardware to drop the packets in case of any error in the
> >>>> packets 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>
> >>>> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> 
> Thinking a bit more about it I agree with Thomas idea that
> it should be flow API based solution in fact.
> Drop is just a one of possible actions to be done with
> packets with bad checksum on one or another layer.
> Such packets could be redirected to a slow path
> (dedicated queue or port ID (PF, VF)).
> It is just a missing feature in various layer
> pattern match to say if we want to proceed with packets
> with only good or only bad chehcksum (or we don't care
> as we do right now). Exact match for checksums is hardly
> useful except UDP with zero checksum case.

I would think of it applicable to both, i.e. it could fit in config option
as well as in flow (e.g. RSS we also have as part of both config and flow).
Having this in flow would increase usage like redirecting as you mentioned,
and having in the config will increase utility when simple config like RSS is
used without flow API's.



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets
  2020-10-13  7:36           ` Nipun Gupta
@ 2020-10-13  7:51             ` Andrew Rybchenko
  2020-10-13  8:12               ` Nipun Gupta
  0 siblings, 1 reply; 46+ messages in thread
From: Andrew Rybchenko @ 2020-10-13  7:51 UTC (permalink / raw)
  To: Nipun Gupta, dev
  Cc: thomas, ferruh.yigit, Hemant Agrawal, Sachin Saxena, Rohit Raj,
	jerinjacobk, stephen, asafp

On 10/13/20 10:36 AM, Nipun Gupta wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Tuesday, October 13, 2020 12:52 PM
>> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
>> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; Hemant Agrawal
>> <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>; Rohit
>> Raj <rohit.raj@nxp.com>; jerinjacobk@gmail.com;
>> stephen@networkplumber.org; asafp@nvidia.com
>> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
>> packets
>>
>> On 10/12/20 3:22 PM, Andrew Rybchenko wrote:
>>> On 10/12/20 2:30 PM, Nipun Gupta wrote:
>>>>> -----Original Message-----
>>>>> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
>>>>> Sent: Monday, October 12, 2020 1:32 PM
>>>>> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
>>>>> Cc: thomas@monjalon.net; ferruh.yigit@intel.com;
>> arybchenko@solarflare.com;
>>>>> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
>>>>> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
>>>>> jerinjacobk@gmail.com; stephen@networkplumber.org; asafp@nvidia.com
>>>>> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
>>>>> packets
>>>>>
>>>>> On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
>>>>>> From: Nipun Gupta <nipun.gupta@nxp.com>
>>>>>>
>>>>>> This change adds a RX offload capability and configuration to
>>>>>> enable hardware to drop the packets in case of any error in the
>>>>>> packets 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>
>>>>>> Reviewed-by: Asaf Penso <asafp@nvidia.com>
>>
>> Thinking a bit more about it I agree with Thomas idea that
>> it should be flow API based solution in fact.
>> Drop is just a one of possible actions to be done with
>> packets with bad checksum on one or another layer.
>> Such packets could be redirected to a slow path
>> (dedicated queue or port ID (PF, VF)).
>> It is just a missing feature in various layer
>> pattern match to say if we want to proceed with packets
>> with only good or only bad chehcksum (or we don't care
>> as we do right now). Exact match for checksums is hardly
>> useful except UDP with zero checksum case.
> 
> I would think of it applicable to both, i.e. it could fit in config option
> as well as in flow (e.g. RSS we also have as part of both config and flow).
> Having this in flow would increase usage like redirecting as you mentioned,
> and having in the config will increase utility when simple config like RSS is
> used without flow API's.

I dislike the idea to have it on both layers.
It adds conflicts by design. It should be no duplication.
That's why mirroring API is deprecated and will be removed
since we have the functionality via flow API now.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets
  2020-10-13  7:51             ` Andrew Rybchenko
@ 2020-10-13  8:12               ` Nipun Gupta
  0 siblings, 0 replies; 46+ messages in thread
From: Nipun Gupta @ 2020-10-13  8:12 UTC (permalink / raw)
  To: Andrew Rybchenko, dev
  Cc: thomas, ferruh.yigit, Hemant Agrawal, Sachin Saxena, Rohit Raj,
	jerinjacobk, stephen, asafp



> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Tuesday, October 13, 2020 1:21 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>; Rohit
> Raj <rohit.raj@nxp.com>; jerinjacobk@gmail.com;
> stephen@networkplumber.org; asafp@nvidia.com
> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> packets
> 
> On 10/13/20 10:36 AM, Nipun Gupta wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Tuesday, October 13, 2020 12:52 PM
> >> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> >> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; Hemant Agrawal
> >> <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> Rohit
> >> Raj <rohit.raj@nxp.com>; jerinjacobk@gmail.com;
> >> stephen@networkplumber.org; asafp@nvidia.com
> >> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> >> packets
> >>
> >> On 10/12/20 3:22 PM, Andrew Rybchenko wrote:
> >>> On 10/12/20 2:30 PM, Nipun Gupta wrote:
> >>>>> -----Original Message-----
> >>>>> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> >>>>> Sent: Monday, October 12, 2020 1:32 PM
> >>>>> To: Nipun Gupta <nipun.gupta@nxp.com>; dev@dpdk.org
> >>>>> Cc: thomas@monjalon.net; ferruh.yigit@intel.com;
> >> arybchenko@solarflare.com;
> >>>>> Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> >>>>> <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@nxp.com>;
> >>>>> jerinjacobk@gmail.com; stephen@networkplumber.org;
> asafp@nvidia.com
> >>>>> Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop
> error
> >>>>> packets
> >>>>>
> >>>>> On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote:
> >>>>>> From: Nipun Gupta <nipun.gupta@nxp.com>
> >>>>>>
> >>>>>> This change adds a RX offload capability and configuration to
> >>>>>> enable hardware to drop the packets in case of any error in the
> >>>>>> packets 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>
> >>>>>> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> >>
> >> Thinking a bit more about it I agree with Thomas idea that
> >> it should be flow API based solution in fact.
> >> Drop is just a one of possible actions to be done with
> >> packets with bad checksum on one or another layer.
> >> Such packets could be redirected to a slow path
> >> (dedicated queue or port ID (PF, VF)).
> >> It is just a missing feature in various layer
> >> pattern match to say if we want to proceed with packets
> >> with only good or only bad chehcksum (or we don't care
> >> as we do right now). Exact match for checksums is hardly
> >> useful except UDP with zero checksum case.
> >
> > I would think of it applicable to both, i.e. it could fit in config option
> > as well as in flow (e.g. RSS we also have as part of both config and flow).
> > Having this in flow would increase usage like redirecting as you mentioned,
> > and having in the config will increase utility when simple config like RSS is
> > used without flow API's.
> 
> I dislike the idea to have it on both layers.
> It adds conflicts by design. It should be no duplication.
> That's why mirroring API is deprecated and will be removed
> since we have the functionality via flow API now.

I would like to agree, but looking at some general configuration which we have
part of both config and flow like RSS packet distribution. It seems it is part of config
too apart from flow and most sample applications using the same because of
simplicity of use?
Seems stripping of VLAN is in similar lines:  RTE_FLOW_ACTION_TYPE_OF_POP_VLAN
and DEV_RX_OFFLOAD_VLAN_STRIP?


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx offload to drop error packets
  2020-08-31  7:53 [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets Nipun Gupta
                   ` (3 preceding siblings ...)
  2020-10-09 13:13 ` [dpdk-dev] [PATCH 1/3 v3] " nipun.gupta
@ 2020-10-15 13:23 ` nipun.gupta
  2020-10-15 13:23   ` [dpdk-dev] [PATCH 2/3 v4] net/dpaa: support Rx offload for error packet drop nipun.gupta
                     ` (3 more replies)
  4 siblings, 4 replies; 46+ messages in thread
From: nipun.gupta @ 2020-10-15 13:23 UTC (permalink / raw)
  To: dev
  Cc: thomas, ferruh.yigit, arybchenko, hemant.agrawal, sachin.saxena,
	rohit.raj, jerinjacobk, stephen, asafp, Nipun Gupta

From: Nipun Gupta <nipun.gupta@nxp.com>

This change adds a Rx offload capability and configuration to
enable hardware to drop the packets in case of any error in the
packets 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>
Reviewed-by: Asaf Penso <asafp@nvidia.com>
---

v4:
 - renamed 'rte_rx_err_pkt_drop_conf' to
   'rte_eth_rx_err_pkt_drop_conf'
 - updated function 'port_offload_cap_display' to display newly
   added offloads
 - added placeholder for L1 FCS, L3 Checksum, L4 Checksum error
   packet drops
 - updated doc/guides/nics/features.rst
 - updated new added 'DEV_RX_OFFLOAD_*' to 'RTE_DEV_RX_OFFLOAD*'
 - updated RX to Rx

v3:
 - Add additional rx_err_drop_offload_capa, which is specific
   capability flag for RX packets error drop offload. Currently
   only 'all' error packet drops are enabled, but can be extended
   to provide capability to drop any specific errors like L1 FCS,
   L3 Checksum etc.
 - Added separate config structure to enable the drop configuration.
 - Updated doc with the new updated option in testbbdev (patch 3/3)

v2:
 - Add support in DPAA1 driver (patch 2/3)
 - Add support and config parameter in testpmd (patch 3/3)

 lib/librte_ethdev/rte_ethdev.c |  1 +
 lib/librte_ethdev/rte_ethdev.h | 39 +++++++++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 48d1333b1..be25e947e 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -128,6 +128,7 @@ static const struct {
 	RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
 	RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
 	RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
+	RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
 };
 
 #undef RTE_RX_OFFLOAD_BIT2STR
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index d2bf74f12..0db4b4021 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1194,6 +1194,28 @@ struct rte_intr_conf {
 	uint32_t rmv:1;
 };
 
+/**
+ * A structure used to enable/disable error packet drop on Rx.
+ */
+struct rte_eth_rx_err_pkt_drop_conf {
+	/** enable/disable L1 FSC error packet drop on Rx.
+	 * 0 (default) - disable, 1 enable
+	 */
+	uint32_t l1_fcs:1;
+	/** enable/disable L3 Checksum error packet drop on Rx.
+	 * 0 (default) - disable, 1 enable
+	 */
+	uint32_t l3_csum:1;
+	/** enable/disable L4 Checksum error packet drop on Rx.
+	 * 0 (default) - disable, 1 enable
+	 */
+	uint32_t l4_csum:1;
+	/** enable/disable all Rx error packet drop.
+	 * 0 (default) - disable, 1 enable
+	 */
+	uint32_t all:1;
+};
+
 /**
  * A structure used to configure an Ethernet port.
  * Depending upon the RX multi-queue mode, extra advanced
@@ -1236,10 +1258,12 @@ struct rte_eth_conf {
 	uint32_t dcb_capability_en;
 	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
 	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
+	struct rte_eth_rx_err_pkt_drop_conf err_pkt_drop_conf;
+	/**< Rx error packet drop configuration. */
 };
 
 /**
- * RX offload capabilities of a device.
+ * Rx offload capabilities of a device.
  */
 #define DEV_RX_OFFLOAD_VLAN_STRIP  0x00000001
 #define DEV_RX_OFFLOAD_IPV4_CKSUM  0x00000002
@@ -1260,6 +1284,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 | \
@@ -1274,6 +1299,16 @@ struct rte_eth_conf {
  * mentioned in rte_rx_offload_names in rte_ethdev.c file.
  */
 
+/**
+ * Rx Error Drop offload config/capabilities of a device. These
+ * are valid only when Rx capability RTE_DEV_RX_OFFLOAD_ERR_PKT_DROP
+ * is supported by the device.
+ */
+#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS		0x00000001
+#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM		0x00000002
+#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM		0x00000004
+#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL		0x80000000
+
 /**
  * TX offload capabilities of a device.
  */
@@ -1411,6 +1446,8 @@ struct rte_eth_dev_info {
 	/**< Device per-queue RX offload capabilities. */
 	uint64_t tx_queue_offload_capa;
 	/**< Device per-queue TX offload capabilities. */
+	uint64_t rx_err_drop_offload_capa;
+	/**< RX error packet drop offload capabilities. */
 	uint16_t reta_size;
 	/**< Device redirection table size, the total number of entries. */
 	uint8_t hash_key_size; /**< Hash key size in bytes */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [dpdk-dev] [PATCH 2/3 v4] net/dpaa: support Rx offload for error packet drop
  2020-10-15 13:23 ` [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx " nipun.gupta
@ 2020-10-15 13:23   ` nipun.gupta
  2020-10-15 13:23   ` [dpdk-dev] [PATCH 3/3 v4] app/testpmd: support hardware offload to drop error packets nipun.gupta
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 46+ messages in thread
From: nipun.gupta @ 2020-10-15 13:23 UTC (permalink / raw)
  To: dev
  Cc: thomas, ferruh.yigit, arybchenko, hemant.agrawal, sachin.saxena,
	rohit.raj, jerinjacobk, stephen, asafp, Nipun Gupta

From: Nipun Gupta <nipun.gupta@nxp.com>

This patch supports Rx offload configuration to drop error
packets in the hardware

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
 drivers/bus/dpaa/base/fman/fman_hw.c |  7 ++++++-
 drivers/net/dpaa/dpaa_ethdev.c       | 23 +++++++++++++++++++----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/dpaa/base/fman/fman_hw.c b/drivers/bus/dpaa/base/fman/fman_hw.c
index 4ab49f785..40e4e0000 100644
--- a/drivers/bus/dpaa/base/fman/fman_hw.c
+++ b/drivers/bus/dpaa/base/fman/fman_hw.c
@@ -597,7 +597,12 @@ void
 fman_if_discard_rx_errors(struct fman_if *fm_if)
 {
 	struct __fman_if *__if = container_of(fm_if, struct __fman_if, __if);
-	unsigned int *fmbm_rfsdm, *fmbm_rfsem;
+	unsigned int *fmbm_rcfg, *fmbm_rfsdm, *fmbm_rfsem;
+	unsigned int val;
+
+	fmbm_rcfg = &((struct rx_bmi_regs *)__if->bmi_map)->fmbm_rcfg;
+	val = in_be32(fmbm_rcfg);
+	out_be32(fmbm_rcfg, val & ~BMI_PORT_CFG_FDOVR);
 
 	fmbm_rfsem = &((struct rx_bmi_regs *)__if->bmi_map)->fmbm_rfsem;
 	out_be32(fmbm_rfsem, 0);
diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index af47c196a..b9cfc4fb3 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -52,7 +52,8 @@
 /* Supported Rx offloads */
 static uint64_t dev_rx_offloads_sup =
 		DEV_RX_OFFLOAD_JUMBO_FRAME |
-		DEV_RX_OFFLOAD_SCATTER;
+		DEV_RX_OFFLOAD_SCATTER |
+		DEV_RX_OFFLOAD_ERR_PKT_DROP;
 
 /* Rx offloads which cannot be disabled */
 static uint64_t dev_rx_offloads_nodis =
@@ -62,6 +63,10 @@ static uint64_t dev_rx_offloads_nodis =
 		DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM |
 		DEV_RX_OFFLOAD_RSS_HASH;
 
+/* Supported Rx Error packet drop offload */
+static uint64_t dev_rx_err_drop_offloads_sup =
+		RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL;
+
 /* Supported Tx offloads */
 static uint64_t dev_tx_offloads_sup =
 		DEV_TX_OFFLOAD_MT_LOCKFREE |
@@ -262,6 +267,18 @@ dpaa_eth_dev_configure(struct rte_eth_dev *dev)
 		dev->data->scattered_rx = 1;
 	}
 
+	if (eth_conf->err_pkt_drop_conf.all) {
+		DPAA_PMD_DEBUG("error packets will be dropped on hw");
+		fman_if_discard_rx_errors(fif);
+	} else {
+		struct dpaa_if *dpaa_intf = dev->data->dev_private;
+		struct qman_fq *rxq = &dpaa_intf->rx_queues[0];
+
+		DPAA_PMD_DEBUG("error packets will not be dropped on hw");
+		fman_if_receive_rx_errors(fif, FM_FD_RX_STATUS_ERR_MASK);
+		fman_if_set_err_fqid(fif, rxq->fqid);
+	}
+
 	if (!(default_q || fmc_q)) {
 		if (dpaa_fm_config(dev,
 			eth_conf->rx_adv_conf.rss_conf.rss_hf)) {
@@ -591,6 +608,7 @@ static int dpaa_eth_dev_info(struct rte_eth_dev *dev,
 					dev_rx_offloads_nodis;
 	dev_info->tx_offload_capa = dev_tx_offloads_sup |
 					dev_tx_offloads_nodis;
+	dev_info->rx_err_drop_offload_capa = dev_rx_err_drop_offloads_sup;
 	dev_info->default_rxportconf.burst_size = DPAA_DEF_RX_BURST_SIZE;
 	dev_info->default_txportconf.burst_size = DPAA_DEF_TX_BURST_SIZE;
 	dev_info->default_rxportconf.nb_queues = 1;
@@ -2085,9 +2103,6 @@ dpaa_dev_init(struct rte_eth_dev *eth_dev)
 		fman_intf->mac_addr.addr_bytes[5]);
 
 	if (!fman_intf->is_shared_mac) {
-		/* Configure error packet handling */
-		fman_if_receive_rx_errors(fman_intf,
-			FM_FD_RX_STATUS_ERR_MASK);
 		/* Disable RX mode */
 		fman_if_disable_rx(fman_intf);
 		/* Disable promiscuous mode */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [dpdk-dev] [PATCH 3/3 v4] app/testpmd: support hardware offload to drop error packets
  2020-10-15 13:23 ` [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx " nipun.gupta
  2020-10-15 13:23   ` [dpdk-dev] [PATCH 2/3 v4] net/dpaa: support Rx offload for error packet drop nipun.gupta
@ 2020-10-15 13:23   ` nipun.gupta
  2020-10-29 17:22     ` Dharmik Thakkar
  2020-10-19  3:30   ` [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx " Ajit Khaparde
  2021-02-18 20:32   ` Ferruh Yigit
  3 siblings, 1 reply; 46+ messages in thread
From: nipun.gupta @ 2020-10-15 13:23 UTC (permalink / raw)
  To: dev
  Cc: thomas, ferruh.yigit, arybchenko, hemant.agrawal, sachin.saxena,
	rohit.raj, jerinjacobk, stephen, asafp, Nipun Gupta

From: Nipun Gupta <nipun.gupta@nxp.com>

With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
capability, and separate RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL
capability to drop all error packets in hardware, testpmd
showcases this with a new added configuration option
'enable-hw-drop-err-all'.

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
 app/test-pmd/config.c                 | 35 +++++++++++++++++++++
 app/test-pmd/parameters.c             |  7 +++++
 app/test-pmd/testpmd.c                |  8 +++++
 app/test-pmd/testpmd.h                |  1 +
 doc/guides/nics/features.rst          | 44 +++++++++++++++++++++++++++
 doc/guides/testpmd_app_ug/run_app.rst |  4 +++
 6 files changed, 99 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index e73dc66c8..14ef4e468 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1027,6 +1027,41 @@ port_offload_cap_display(portid_t port_id)
 			printf("off\n");
 	}
 
+	if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_ERR_PKT_DROP) {
+		if (dev_info.rx_err_drop_offload_capa &
+			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS) {
+			printf("RX L1 FCS Error pkt drop:      ");
+			if (ports[port_id].dev_conf.err_pkt_drop_conf.l1_fcs)
+				printf("on\n");
+			else
+				printf("off\n");
+		}
+		if (dev_info.rx_err_drop_offload_capa &
+			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM) {
+			printf("RX L3 Csum Error pkt drop:     ");
+			if (ports[port_id].dev_conf.err_pkt_drop_conf.l3_csum)
+				printf("on\n");
+			else
+				printf("off\n");
+		}
+		if (dev_info.rx_err_drop_offload_capa &
+			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM) {
+			printf("RX L4 Csum Error pkt drop:     ");
+			if (ports[port_id].dev_conf.err_pkt_drop_conf.l4_csum)
+				printf("on\n");
+			else
+				printf("off\n");
+		}
+		if (dev_info.rx_err_drop_offload_capa &
+			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL) {
+			printf("RX all Error pkt drop:         ");
+			if (ports[port_id].dev_conf.err_pkt_drop_conf.all)
+				printf("on\n");
+			else
+				printf("off\n");
+		}
+	}
+
 	if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VLAN_INSERT) {
 		printf("VLAN insert:                   ");
 		if (ports[port_id].dev_conf.txmode.offloads &
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 1ead59579..508612426 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -142,6 +142,7 @@ usage(char* progname)
 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
+	printf("  --enable-hw-drop-err-all: enable hardware packet drop for all error packets.\n");
 	printf("  --enable-drop-en: enable per queue packet drop.\n");
 	printf("  --disable-rss: disable rss.\n");
 	printf("  --port-topology=<paired|chained|loop>: set port topology (paired "
@@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
 		{ "enable-hw-vlan-strip",       0, 0, 0 },
 		{ "enable-hw-vlan-extend",      0, 0, 0 },
 		{ "enable-hw-qinq-strip",       0, 0, 0 },
+		{ "enable-hw-drop-err-all",     0, 0, 0 },
 		{ "enable-drop-en",            0, 0, 0 },
 		{ "disable-rss",                0, 0, 0 },
 		{ "port-topology",              1, 0, 0 },
@@ -1283,6 +1285,11 @@ launch_args_parse(int argc, char** argv)
 				rmv_interrupt = 0;
 			if (!strcmp(lgopts[opt_idx].name, "flow-isolate-all"))
 				flow_isolate_all = 1;
+			if (!strcmp(lgopts[opt_idx].name,
+					"enable-hw-drop-err-all")) {
+				rx_err_pkt_drop_all = 1;
+			}
+
 			if (!strcmp(lgopts[opt_idx].name, "tx-offloads")) {
 				char *end = NULL;
 				n = strtoull(optarg, &end, 16);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index ccba71c07..c9e7397e6 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -359,6 +359,11 @@ uint8_t lsc_interrupt = 1; /* enabled by default */
  */
 uint8_t rmv_interrupt = 1; /* enabled by default */
 
+/*
+ * Drop all RX error packets on HW itself.
+ */
+uint8_t rx_err_pkt_drop_all = 0; /* disabled by default */
+
 uint8_t hot_plug = 0; /**< hotplug disabled by default. */
 
 /* After attach, port setup is called on event or by iterator */
@@ -3359,6 +3364,9 @@ init_port_config(void)
 		    (rte_eth_devices[pid].data->dev_flags &
 		     RTE_ETH_DEV_INTR_RMV))
 			port->dev_conf.intr_conf.rmv = 1;
+
+		if (rx_err_pkt_drop_all)
+			port->dev_conf.err_pkt_drop_conf.all = 1;
 	}
 }
 
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index c7e7e41a9..eab154ed4 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -314,6 +314,7 @@ extern uint8_t no_device_start; /**<set by "--disable-device-start" parameter */
 extern volatile int test_done; /* stop packet forwarding when set to 1. */
 extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-interrupt" parameter */
 extern uint8_t rmv_interrupt; /**< disabled by "--no-rmv-interrupt" parameter */
+extern uint8_t rx_err_pkt_drop_all; /**< enabled by "--enable-hw-drop-err-all" parameter */
 extern uint32_t event_print_mask;
 /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
 extern bool setup_on_probe_event; /**< disabled by port setup-on iterator */
diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
index dd8c9555b..cb7fdfd8b 100644
--- a/doc/guides/nics/features.rst
+++ b/doc/guides/nics/features.rst
@@ -606,6 +606,50 @@ Supports inner packet L4 checksum.
   ``tx_offload_capa,tx_queue_offload_capa:DEV_TX_OFFLOAD_OUTER_UDP_CKSUM``.
 
 
+.. _nic_features_l1_fcs_rx_error_packet_drop:
+
+L1 FCS Error Packet drop on Rx
+------------------------------
+
+Supports dropping of packets having L1 FCS error on Rx.
+
+* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l1_fcs``.
+* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS``.
+
+
+.. _nic_features_l3_csum_rx_error_packet_drop:
+
+L3 checksum Error Packet drop on Rx
+-----------------------------------
+
+Supports dropping of packets having L3 Checksum error on Rx.
+
+* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l3_csum``.
+* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM``.
+
+
+.. _nic_features_l4_csum_rx_error_packet_drop:
+
+L4 Checksum Error Packet drop on Rx
+-----------------------------------
+
+Supports dropping of packets having L1 FCS error on Rx.
+
+* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l4_csum``.
+* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM``.
+
+
+.. _nic_features_all_rx_error_packet_drop:
+
+All/any Error Packet drop on Rx
+-------------------------------
+
+Supports dropping of packets having any of the errors like L1 FSC, L3/L4 Checksum on Rx.
+
+* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.all``.
+* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL``.
+
+
 .. _nic_features_packet_type_parsing:
 
 Packet type parsing
diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
index e2539f693..20f2f8083 100644
--- a/doc/guides/testpmd_app_ug/run_app.rst
+++ b/doc/guides/testpmd_app_ug/run_app.rst
@@ -213,6 +213,10 @@ The command line options are:
 
     Enable hardware QINQ strip.
 
+*  ``--enable-hw-drop-err-all``
+
+    Enable hardware packet drop for any error packets
+
 *   ``--enable-drop-en``
 
     Enable per-queue packet drop for packets with no descriptors.
-- 
2.17.1


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx offload to drop error packets
  2020-10-15 13:23 ` [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx " nipun.gupta
  2020-10-15 13:23   ` [dpdk-dev] [PATCH 2/3 v4] net/dpaa: support Rx offload for error packet drop nipun.gupta
  2020-10-15 13:23   ` [dpdk-dev] [PATCH 3/3 v4] app/testpmd: support hardware offload to drop error packets nipun.gupta
@ 2020-10-19  3:30   ` Ajit Khaparde
  2021-02-18 20:32   ` Ferruh Yigit
  3 siblings, 0 replies; 46+ messages in thread
From: Ajit Khaparde @ 2020-10-19  3:30 UTC (permalink / raw)
  To: nipun.gupta
  Cc: dpdk-dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko,
	Hemant Agrawal, Sachin Saxena, rohit.raj, Jerin Jacob,
	Stephen Hemminger, asafp

On Thu, Oct 15, 2020 at 6:24 AM <nipun.gupta@nxp.com> wrote:
>
> From: Nipun Gupta <nipun.gupta@nxp.com>
>
> This change adds a Rx offload capability and configuration to
> enable hardware to drop the packets in case of any error in the
> packets 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>
> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> ---
>
> v4:
>  - renamed 'rte_rx_err_pkt_drop_conf' to
>    'rte_eth_rx_err_pkt_drop_conf'
>  - updated function 'port_offload_cap_display' to display newly
>    added offloads
>  - added placeholder for L1 FCS, L3 Checksum, L4 Checksum error
>    packet drops
>  - updated doc/guides/nics/features.rst
>  - updated new added 'DEV_RX_OFFLOAD_*' to 'RTE_DEV_RX_OFFLOAD*'
>  - updated RX to Rx
>
> v3:
>  - Add additional rx_err_drop_offload_capa, which is specific
>    capability flag for RX packets error drop offload. Currently
>    only 'all' error packet drops are enabled, but can be extended
>    to provide capability to drop any specific errors like L1 FCS,
>    L3 Checksum etc.
>  - Added separate config structure to enable the drop configuration.
>  - Updated doc with the new updated option in testbbdev (patch 3/3)
>
> v2:
>  - Add support in DPAA1 driver (patch 2/3)
>  - Add support and config parameter in testpmd (patch 3/3)
>
>  lib/librte_ethdev/rte_ethdev.c |  1 +
>  lib/librte_ethdev/rte_ethdev.h | 39 +++++++++++++++++++++++++++++++++-
>  2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 48d1333b1..be25e947e 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -128,6 +128,7 @@ static const struct {
>         RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
>         RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
>         RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
> +       RTE_RX_OFFLOAD_BIT2STR(ERR_PKT_DROP),
>  };
>
>  #undef RTE_RX_OFFLOAD_BIT2STR
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d2bf74f12..0db4b4021 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1194,6 +1194,28 @@ struct rte_intr_conf {
>         uint32_t rmv:1;
>  };
>
> +/**
> + * A structure used to enable/disable error packet drop on Rx.
> + */
> +struct rte_eth_rx_err_pkt_drop_conf {
> +       /** enable/disable L1 FSC error packet drop on Rx.
s/FSC/FCS
Its L2 FCS. FCS is a part of Ethernet frame.

> +        * 0 (default) - disable, 1 enable
> +        */
> +       uint32_t l1_fcs:1;
l2_fcs;

> +       /** enable/disable L3 Checksum error packet drop on Rx.
> +        * 0 (default) - disable, 1 enable
> +        */
> +       uint32_t l3_csum:1;
> +       /** enable/disable L4 Checksum error packet drop on Rx.
> +        * 0 (default) - disable, 1 enable
> +        */
> +       uint32_t l4_csum:1;
> +       /** enable/disable all Rx error packet drop.
> +        * 0 (default) - disable, 1 enable
> +        */
> +       uint32_t all:1;
> +};
> +
>  /**
>   * A structure used to configure an Ethernet port.
>   * Depending upon the RX multi-queue mode, extra advanced
> @@ -1236,10 +1258,12 @@ struct rte_eth_conf {
>         uint32_t dcb_capability_en;
>         struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
>         struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
> +       struct rte_eth_rx_err_pkt_drop_conf err_pkt_drop_conf;
> +       /**< Rx error packet drop configuration. */
>  };
>
>  /**
> - * RX offload capabilities of a device.
> + * Rx offload capabilities of a device.
>   */
>  #define DEV_RX_OFFLOAD_VLAN_STRIP  0x00000001
>  #define DEV_RX_OFFLOAD_IPV4_CKSUM  0x00000002
> @@ -1260,6 +1284,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 | \
> @@ -1274,6 +1299,16 @@ struct rte_eth_conf {
>   * mentioned in rte_rx_offload_names in rte_ethdev.c file.
>   */
>
> +/**
> + * Rx Error Drop offload config/capabilities of a device. These
> + * are valid only when Rx capability RTE_DEV_RX_OFFLOAD_ERR_PKT_DROP
> + * is supported by the device.
> + */
> +#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS         0x00000001
RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L2_FCS



> +#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM                0x00000002
> +#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM                0x00000004
> +#define RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL            0x80000000
> +
>  /**
>   * TX offload capabilities of a device.
>   */
> @@ -1411,6 +1446,8 @@ struct rte_eth_dev_info {
>         /**< Device per-queue RX offload capabilities. */
>         uint64_t tx_queue_offload_capa;
>         /**< Device per-queue TX offload capabilities. */
> +       uint64_t rx_err_drop_offload_capa;
> +       /**< RX error packet drop offload capabilities. */
>         uint16_t reta_size;
>         /**< Device redirection table size, the total number of entries. */
>         uint8_t hash_key_size; /**< Hash key size in bytes */
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 3/3 v4] app/testpmd: support hardware offload to drop error packets
  2020-10-15 13:23   ` [dpdk-dev] [PATCH 3/3 v4] app/testpmd: support hardware offload to drop error packets nipun.gupta
@ 2020-10-29 17:22     ` Dharmik Thakkar
  2020-10-31 18:16       ` Nipun Gupta
  0 siblings, 1 reply; 46+ messages in thread
From: Dharmik Thakkar @ 2020-10-29 17:22 UTC (permalink / raw)
  To: Nipun.gupta@nxp.com
  Cc: dev, thomas, Ferruh Yigit, arybchenko, hemant.agrawal,
	Sachin.saxena@nxp.com, rohit.raj, Jerin Jacob, Stephen Hemminger,
	asafp, nd

Hi Nipun,

Some nits. Looks good otherwise.

> On Oct 15, 2020, at 8:23 AM, Nipun.gupta@nxp.com <nipun.gupta@nxp.com> wrote:
> 
> From: Nipun Gupta <nipun.gupta@nxp.com>
> 
> With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
> capability, and separate RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL
> capability to drop all error packets in hardware, testpmd
> showcases this with a new added configuration option
> 'enable-hw-drop-err-all'.
> 
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> ---
> app/test-pmd/config.c                 | 35 +++++++++++++++++++++
> app/test-pmd/parameters.c             |  7 +++++
> app/test-pmd/testpmd.c                |  8 +++++
> app/test-pmd/testpmd.h                |  1 +
> doc/guides/nics/features.rst          | 44 +++++++++++++++++++++++++++
> doc/guides/testpmd_app_ug/run_app.rst |  4 +++
> 6 files changed, 99 insertions(+)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index e73dc66c8..14ef4e468 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1027,6 +1027,41 @@ port_offload_cap_display(portid_t port_id)
> 			printf("off\n");
> 	}
> 
> +	if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_ERR_PKT_DROP) {
> +		if (dev_info.rx_err_drop_offload_capa &
> +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS) {
> +			printf("RX L1 FCS Error pkt drop:      ");
> +			if (ports[port_id].dev_conf.err_pkt_drop_conf.l1_fcs)
> +				printf("on\n");
> +			else
> +				printf("off\n");
> +		}
> +		if (dev_info.rx_err_drop_offload_capa &
> +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM) {
> +			printf("RX L3 Csum Error pkt drop:     ");
> +			if (ports[port_id].dev_conf.err_pkt_drop_conf.l3_csum)
> +				printf("on\n");
> +			else
> +				printf("off\n");
> +		}
> +		if (dev_info.rx_err_drop_offload_capa &
> +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM) {
> +			printf("RX L4 Csum Error pkt drop:     ");
> +			if (ports[port_id].dev_conf.err_pkt_drop_conf.l4_csum)
> +				printf("on\n");
> +			else
> +				printf("off\n");
> +		}
> +		if (dev_info.rx_err_drop_offload_capa &
> +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL) {
> +			printf("RX all Error pkt drop:         ");
> +			if (ports[port_id].dev_conf.err_pkt_drop_conf.all)
> +				printf("on\n");
> +			else
> +				printf("off\n");
> +		}
> +	}
> +
> 	if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VLAN_INSERT) {
> 		printf("VLAN insert:                   ");
> 		if (ports[port_id].dev_conf.txmode.offloads &
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> index 1ead59579..508612426 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -142,6 +142,7 @@ usage(char* progname)
> 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
> 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
> 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
> +	printf("  --enable-hw-drop-err-all: enable hardware packet drop for all error packets.\n");
> 	printf("  --enable-drop-en: enable per queue packet drop.\n");
> 	printf("  --disable-rss: disable rss.\n");
> 	printf("  --port-topology=<paired|chained|loop>: set port topology (paired "
> @@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
> 		{ "enable-hw-vlan-strip",       0, 0, 0 },
> 		{ "enable-hw-vlan-extend",      0, 0, 0 },
> 		{ "enable-hw-qinq-strip",       0, 0, 0 },
> +		{ "enable-hw-drop-err-all",     0, 0, 0 },
> 		{ "enable-drop-en",            0, 0, 0 },
> 		{ "disable-rss",                0, 0, 0 },
> 		{ "port-topology",              1, 0, 0 },
> @@ -1283,6 +1285,11 @@ launch_args_parse(int argc, char** argv)
> 				rmv_interrupt = 0;
> 			if (!strcmp(lgopts[opt_idx].name, "flow-isolate-all"))
> 				flow_isolate_all = 1;
> +			if (!strcmp(lgopts[opt_idx].name,
> +					"enable-hw-drop-err-all")) {
> +				rx_err_pkt_drop_all = 1;
> +			}

{ } can be removed.

> +
> 			if (!strcmp(lgopts[opt_idx].name, "tx-offloads")) {
> 				char *end = NULL;
> 				n = strtoull(optarg, &end, 16);
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index ccba71c07..c9e7397e6 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -359,6 +359,11 @@ uint8_t lsc_interrupt = 1; /* enabled by default */
>  */
> uint8_t rmv_interrupt = 1; /* enabled by default */
> 
> +/*
> + * Drop all RX error packets on HW itself.
> + */
> +uint8_t rx_err_pkt_drop_all = 0; /* disabled by default */

= 0 can be removed.

> +
> uint8_t hot_plug = 0; /**< hotplug disabled by default. */
> 
> /* After attach, port setup is called on event or by iterator */
> @@ -3359,6 +3364,9 @@ init_port_config(void)
> 		    (rte_eth_devices[pid].data->dev_flags &
> 		     RTE_ETH_DEV_INTR_RMV))
> 			port->dev_conf.intr_conf.rmv = 1;
> +
> +		if (rx_err_pkt_drop_all)
> +			port->dev_conf.err_pkt_drop_conf.all = 1;
> 	}
> }
> 
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index c7e7e41a9..eab154ed4 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -314,6 +314,7 @@ extern uint8_t no_device_start; /**<set by "--disable-device-start" parameter */
> extern volatile int test_done; /* stop packet forwarding when set to 1. */
> extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-interrupt" parameter */
> extern uint8_t rmv_interrupt; /**< disabled by "--no-rmv-interrupt" parameter */
> +extern uint8_t rx_err_pkt_drop_all; /**< enabled by "--enable-hw-drop-err-all" parameter */
> extern uint32_t event_print_mask;
> /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
> extern bool setup_on_probe_event; /**< disabled by port setup-on iterator */
> diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> index dd8c9555b..cb7fdfd8b 100644
> --- a/doc/guides/nics/features.rst
> +++ b/doc/guides/nics/features.rst
> @@ -606,6 +606,50 @@ Supports inner packet L4 checksum.
>   ``tx_offload_capa,tx_queue_offload_capa:DEV_TX_OFFLOAD_OUTER_UDP_CKSUM``.
> 
> 
> +.. _nic_features_l1_fcs_rx_error_packet_drop:
> +
> +L1 FCS Error Packet drop on Rx
> +------------------------------
> +
> +Supports dropping of packets having L1 FCS error on Rx.
> +
> +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l1_fcs``.
> +* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS``.
> +
> +
> +.. _nic_features_l3_csum_rx_error_packet_drop:
> +
> +L3 checksum Error Packet drop on Rx
> +-----------------------------------
> +
> +Supports dropping of packets having L3 Checksum error on Rx.
> +
> +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l3_csum``.
> +* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM``.
> +
> +
> +.. _nic_features_l4_csum_rx_error_packet_drop:
> +
> +L4 Checksum Error Packet drop on Rx
> +-----------------------------------
> +
> +Supports dropping of packets having L1 FCS error on Rx.
> +
> +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l4_csum``.
> +* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM``.
> +
> +
> +.. _nic_features_all_rx_error_packet_drop:
> +
> +All/any Error Packet drop on Rx
> +-------------------------------
> +
> +Supports dropping of packets having any of the errors like L1 FSC, L3/L4 Checksum on Rx.

s/FSC/FCS

> +
> +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.all``.
> +* **[provides] rte_eth_dev_info**: ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL``.
> +
> +
> .. _nic_features_packet_type_parsing:
> 
> Packet type parsing
> diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
> index e2539f693..20f2f8083 100644
> --- a/doc/guides/testpmd_app_ug/run_app.rst
> +++ b/doc/guides/testpmd_app_ug/run_app.rst
> @@ -213,6 +213,10 @@ The command line options are:
> 
>     Enable hardware QINQ strip.
> 
> +*  ``--enable-hw-drop-err-all``
> +
> +    Enable hardware packet drop for any error packets
> +
> *   ``--enable-drop-en``
> 
>     Enable per-queue packet drop for packets with no descriptors.
> — 
> 2.17.1
> 

Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 3/3 v4] app/testpmd: support hardware offload to drop error packets
  2020-10-29 17:22     ` Dharmik Thakkar
@ 2020-10-31 18:16       ` Nipun Gupta
  0 siblings, 0 replies; 46+ messages in thread
From: Nipun Gupta @ 2020-10-31 18:16 UTC (permalink / raw)
  To: Dharmik Thakkar
  Cc: dev, thomas, Ferruh Yigit, arybchenko, Hemant Agrawal,
	Sachin Saxena, Rohit Raj, Jerin Jacob, Stephen Hemminger, asafp,
	nd



> -----Original Message-----
> From: Dharmik Thakkar <Dharmik.Thakkar@arm.com>
> Sent: Thursday, October 29, 2020 10:52 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>
> Cc: dev <dev@dpdk.org>; thomas@monjalon.net; Ferruh Yigit
> <ferruh.yigit@intel.com>; arybchenko@solarflare.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>; Rohit
> Raj <rohit.raj@nxp.com>; Jerin Jacob <jerinjacobk@gmail.com>; Stephen
> Hemminger <stephen@networkplumber.org>; asafp@nvidia.com; nd
> <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH 3/3 v4] app/testpmd: support hardware offload
> to drop error packets
> 
> Hi Nipun,
> 
> Some nits. Looks good otherwise.
> 
> > On Oct 15, 2020, at 8:23 AM, Nipun.gupta@nxp.com <nipun.gupta@nxp.com>
> wrote:
> >
> > From: Nipun Gupta <nipun.gupta@nxp.com>
> >
> > With DEV_RX_OFFLOAD_ERR_PKT_DROP now defined as an offload
> > capability, and separate RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL
> > capability to drop all error packets in hardware, testpmd
> > showcases this with a new added configuration option
> > 'enable-hw-drop-err-all'.
> >
> > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > ---
> > app/test-pmd/config.c                 | 35 +++++++++++++++++++++
> > app/test-pmd/parameters.c             |  7 +++++
> > app/test-pmd/testpmd.c                |  8 +++++
> > app/test-pmd/testpmd.h                |  1 +
> > doc/guides/nics/features.rst          | 44 +++++++++++++++++++++++++++
> > doc/guides/testpmd_app_ug/run_app.rst |  4 +++
> > 6 files changed, 99 insertions(+)
> >
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> > index e73dc66c8..14ef4e468 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -1027,6 +1027,41 @@ port_offload_cap_display(portid_t port_id)
> > 			printf("off\n");
> > 	}
> >
> > +	if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_ERR_PKT_DROP) {
> > +		if (dev_info.rx_err_drop_offload_capa &
> > +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS) {
> > +			printf("RX L1 FCS Error pkt drop:      ");
> > +			if (ports[port_id].dev_conf.err_pkt_drop_conf.l1_fcs)
> > +				printf("on\n");
> > +			else
> > +				printf("off\n");
> > +		}
> > +		if (dev_info.rx_err_drop_offload_capa &
> > +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM) {
> > +			printf("RX L3 Csum Error pkt drop:     ");
> > +			if (ports[port_id].dev_conf.err_pkt_drop_conf.l3_csum)
> > +				printf("on\n");
> > +			else
> > +				printf("off\n");
> > +		}
> > +		if (dev_info.rx_err_drop_offload_capa &
> > +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM) {
> > +			printf("RX L4 Csum Error pkt drop:     ");
> > +			if (ports[port_id].dev_conf.err_pkt_drop_conf.l4_csum)
> > +				printf("on\n");
> > +			else
> > +				printf("off\n");
> > +		}
> > +		if (dev_info.rx_err_drop_offload_capa &
> > +			RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL) {
> > +			printf("RX all Error pkt drop:         ");
> > +			if (ports[port_id].dev_conf.err_pkt_drop_conf.all)
> > +				printf("on\n");
> > +			else
> > +				printf("off\n");
> > +		}
> > +	}
> > +
> > 	if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VLAN_INSERT) {
> > 		printf("VLAN insert:                   ");
> > 		if (ports[port_id].dev_conf.txmode.offloads &
> > diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> > index 1ead59579..508612426 100644
> > --- a/app/test-pmd/parameters.c
> > +++ b/app/test-pmd/parameters.c
> > @@ -142,6 +142,7 @@ usage(char* progname)
> > 	printf("  --enable-hw-vlan-strip: enable hardware vlan strip.\n");
> > 	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
> > 	printf("  --enable-hw-qinq-strip: enable hardware qinq strip.\n");
> > +	printf("  --enable-hw-drop-err-all: enable hardware packet drop for all
> error packets.\n");
> > 	printf("  --enable-drop-en: enable per queue packet drop.\n");
> > 	printf("  --disable-rss: disable rss.\n");
> > 	printf("  --port-topology=<paired|chained|loop>: set port topology
> (paired "
> > @@ -631,6 +632,7 @@ launch_args_parse(int argc, char** argv)
> > 		{ "enable-hw-vlan-strip",       0, 0, 0 },
> > 		{ "enable-hw-vlan-extend",      0, 0, 0 },
> > 		{ "enable-hw-qinq-strip",       0, 0, 0 },
> > +		{ "enable-hw-drop-err-all",     0, 0, 0 },
> > 		{ "enable-drop-en",            0, 0, 0 },
> > 		{ "disable-rss",                0, 0, 0 },
> > 		{ "port-topology",              1, 0, 0 },
> > @@ -1283,6 +1285,11 @@ launch_args_parse(int argc, char** argv)
> > 				rmv_interrupt = 0;
> > 			if (!strcmp(lgopts[opt_idx].name, "flow-isolate-all"))
> > 				flow_isolate_all = 1;
> > +			if (!strcmp(lgopts[opt_idx].name,
> > +					"enable-hw-drop-err-all")) {
> > +				rx_err_pkt_drop_all = 1;
> > +			}
> 
> { } can be removed.

Agree.

> 
> > +
> > 			if (!strcmp(lgopts[opt_idx].name, "tx-offloads")) {
> > 				char *end = NULL;
> > 				n = strtoull(optarg, &end, 16);
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> > index ccba71c07..c9e7397e6 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -359,6 +359,11 @@ uint8_t lsc_interrupt = 1; /* enabled by default */
> >  */
> > uint8_t rmv_interrupt = 1; /* enabled by default */
> >
> > +/*
> > + * Drop all RX error packets on HW itself.
> > + */
> > +uint8_t rx_err_pkt_drop_all = 0; /* disabled by default */
> 
> = 0 can be removed.

This is on all the other default disabled parameters too probably to show that they
are disabled by default. So I would prefer to keep it like this.

> 
> > +
> > uint8_t hot_plug = 0; /**< hotplug disabled by default. */
> >
> > /* After attach, port setup is called on event or by iterator */
> > @@ -3359,6 +3364,9 @@ init_port_config(void)
> > 		    (rte_eth_devices[pid].data->dev_flags &
> > 		     RTE_ETH_DEV_INTR_RMV))
> > 			port->dev_conf.intr_conf.rmv = 1;
> > +
> > +		if (rx_err_pkt_drop_all)
> > +			port->dev_conf.err_pkt_drop_conf.all = 1;
> > 	}
> > }
> >
> > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> > index c7e7e41a9..eab154ed4 100644
> > --- a/app/test-pmd/testpmd.h
> > +++ b/app/test-pmd/testpmd.h
> > @@ -314,6 +314,7 @@ extern uint8_t no_device_start; /**<set by "--disable-
> device-start" parameter */
> > extern volatile int test_done; /* stop packet forwarding when set to 1. */
> > extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-interrupt" parameter */
> > extern uint8_t rmv_interrupt; /**< disabled by "--no-rmv-interrupt" parameter
> */
> > +extern uint8_t rx_err_pkt_drop_all; /**< enabled by "--enable-hw-drop-err-
> all" parameter */
> > extern uint32_t event_print_mask;
> > /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
> > extern bool setup_on_probe_event; /**< disabled by port setup-on iterator */
> > diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> > index dd8c9555b..cb7fdfd8b 100644
> > --- a/doc/guides/nics/features.rst
> > +++ b/doc/guides/nics/features.rst
> > @@ -606,6 +606,50 @@ Supports inner packet L4 checksum.
> >
> ``tx_offload_capa,tx_queue_offload_capa:DEV_TX_OFFLOAD_OUTER_UDP_CK
> SUM``.
> >
> >
> > +.. _nic_features_l1_fcs_rx_error_packet_drop:
> > +
> > +L1 FCS Error Packet drop on Rx
> > +------------------------------
> > +
> > +Supports dropping of packets having L1 FCS error on Rx.
> > +
> > +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l1_fcs``.
> > +* **[provides] rte_eth_dev_info**:
> ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L1_FCS``.
> > +
> > +
> > +.. _nic_features_l3_csum_rx_error_packet_drop:
> > +
> > +L3 checksum Error Packet drop on Rx
> > +-----------------------------------
> > +
> > +Supports dropping of packets having L3 Checksum error on Rx.
> > +
> > +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l3_csum``.
> > +* **[provides] rte_eth_dev_info**:
> ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L3_CSUM
> ``.
> > +
> > +
> > +.. _nic_features_l4_csum_rx_error_packet_drop:
> > +
> > +L4 Checksum Error Packet drop on Rx
> > +-----------------------------------
> > +
> > +Supports dropping of packets having L1 FCS error on Rx.
> > +
> > +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.l4_csum``.
> > +* **[provides] rte_eth_dev_info**:
> ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_L4_CSUM
> ``.
> > +
> > +
> > +.. _nic_features_all_rx_error_packet_drop:
> > +
> > +All/any Error Packet drop on Rx
> > +-------------------------------
> > +
> > +Supports dropping of packets having any of the errors like L1 FSC, L3/L4
> Checksum on Rx.
> 
> s/FSC/FCS

Will update in the next spin.

Thanks,
Nipun

> 
> > +
> > +* **[uses]     user config**: ``dev_conf.err_pkt_drop_conf.all``.
> > +* **[provides] rte_eth_dev_info**:
> ``rx_err_drop_offload_capa:RTE_DEV_RX_ERR_PKT_DROP_OFFLOAD_ALL``.
> > +
> > +
> > .. _nic_features_packet_type_parsing:
> >
> > Packet type parsing
> > diff --git a/doc/guides/testpmd_app_ug/run_app.rst
> b/doc/guides/testpmd_app_ug/run_app.rst
> > index e2539f693..20f2f8083 100644
> > --- a/doc/guides/testpmd_app_ug/run_app.rst
> > +++ b/doc/guides/testpmd_app_ug/run_app.rst
> > @@ -213,6 +213,10 @@ The command line options are:
> >
> >     Enable hardware QINQ strip.
> >
> > +*  ``--enable-hw-drop-err-all``
> > +
> > +    Enable hardware packet drop for any error packets
> > +
> > *   ``--enable-drop-en``
> >
> >     Enable per-queue packet drop for packets with no descriptors.
> > —
> > 2.17.1
> >
> 
> Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> 


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx offload to drop error packets
  2020-10-15 13:23 ` [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx " nipun.gupta
                     ` (2 preceding siblings ...)
  2020-10-19  3:30   ` [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx " Ajit Khaparde
@ 2021-02-18 20:32   ` Ferruh Yigit
  2021-02-18 20:37     ` Thomas Monjalon
  3 siblings, 1 reply; 46+ messages in thread
From: Ferruh Yigit @ 2021-02-18 20:32 UTC (permalink / raw)
  To: nipun.gupta, dev
  Cc: thomas, arybchenko, hemant.agrawal, sachin.saxena, rohit.raj,
	jerinjacobk, stephen, asafp

On 10/15/2020 2:23 PM, nipun.gupta@nxp.com wrote:
> From: Nipun Gupta <nipun.gupta@nxp.com>
> 
> This change adds a Rx offload capability and configuration to
> enable hardware to drop the packets in case of any error in the
> packets 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>
> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> ---
> 
> v4:
>   - renamed 'rte_rx_err_pkt_drop_conf' to
>     'rte_eth_rx_err_pkt_drop_conf'
>   - updated function 'port_offload_cap_display' to display newly
>     added offloads
>   - added placeholder for L1 FCS, L3 Checksum, L4 Checksum error
>     packet drops
>   - updated doc/guides/nics/features.rst
>   - updated new added 'DEV_RX_OFFLOAD_*' to 'RTE_DEV_RX_OFFLOAD*'
>   - updated RX to Rx
> 
> v3:
>   - Add additional rx_err_drop_offload_capa, which is specific
>     capability flag for RX packets error drop offload. Currently
>     only 'all' error packet drops are enabled, but can be extended
>     to provide capability to drop any specific errors like L1 FCS,
>     L3 Checksum etc.
>   - Added separate config structure to enable the drop configuration.
>   - Updated doc with the new updated option in testbbdev (patch 3/3)
> 
> v2:
>   - Add support in DPAA1 driver (patch 2/3)
>   - Add support and config parameter in testpmd (patch 3/3)
> 
>   lib/librte_ethdev/rte_ethdev.c |  1 +
>   lib/librte_ethdev/rte_ethdev.h | 39 +++++++++++++++++++++++++++++++++-
>   2 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c


This feature touches many main parts,
- new config item for 'rte_eth_dev_configure()'
- a new offload flag
- new capability reporting for 'rte_eth_dev_info_get()'

The feature doesn't look very mainstream to touch all these main parts and add 
complexity to them, which will affect almost all users.

And has some inconsistencies, like configuration is done via config struct, but 
capability is returned as bit-wise.
Or I think config option taken into account only if offload is requested has a 
chance to confuse people in both app and driver end.

What do you think having two specific APIs to get_capabilities and set drop config?
The responsibility of those APIs will be clear and narrowed down, which makes it 
harder to make it wrong.


Also it is an ABI break as it is and needs to wait 21.11, and even after that it 
has a potential to cause more ABI breaks, like trying to add a new error type to 
drop will need to wait 22.11 ..., but if we can have them as separate APIs we 
can have them as experimental without waiting next LTS.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx offload to drop error packets
  2021-02-18 20:32   ` Ferruh Yigit
@ 2021-02-18 20:37     ` Thomas Monjalon
  2021-04-20  1:11       ` Ferruh Yigit
  0 siblings, 1 reply; 46+ messages in thread
From: Thomas Monjalon @ 2021-02-18 20:37 UTC (permalink / raw)
  To: nipun.gupta, Ferruh Yigit
  Cc: dev, arybchenko, hemant.agrawal, sachin.saxena, rohit.raj,
	jerinjacobk, stephen, asafp

18/02/2021 21:32, Ferruh Yigit:
> On 10/15/2020 2:23 PM, nipun.gupta@nxp.com wrote:
> > From: Nipun Gupta <nipun.gupta@nxp.com>
> > 
> > This change adds a Rx offload capability and configuration to
> > enable hardware to drop the packets in case of any error in the
> > packets 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>
> > Reviewed-by: Asaf Penso <asafp@nvidia.com>
> > ---
> 
> This feature touches many main parts,
> - new config item for 'rte_eth_dev_configure()'
> - a new offload flag
> - new capability reporting for 'rte_eth_dev_info_get()'
> 
> The feature doesn't look very mainstream to touch all these main parts and add 
> complexity to them, which will affect almost all users.
> 
> And has some inconsistencies, like configuration is done via config struct, but 
> capability is returned as bit-wise.
> Or I think config option taken into account only if offload is requested has a 
> chance to confuse people in both app and driver end.
> 
> What do you think having two specific APIs to get_capabilities and set drop config?
> The responsibility of those APIs will be clear and narrowed down, which makes it 
> harder to make it wrong.

I agree. In general, it is better adding new functions
instead of adding everything in rte_eth_dev_configure().




^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx offload to drop error packets
  2021-02-18 20:37     ` Thomas Monjalon
@ 2021-04-20  1:11       ` Ferruh Yigit
  0 siblings, 0 replies; 46+ messages in thread
From: Ferruh Yigit @ 2021-04-20  1:11 UTC (permalink / raw)
  To: Thomas Monjalon, nipun.gupta
  Cc: dev, arybchenko, hemant.agrawal, sachin.saxena, rohit.raj,
	jerinjacobk, stephen, asafp

On 2/18/2021 8:37 PM, Thomas Monjalon wrote:
> 18/02/2021 21:32, Ferruh Yigit:
>> On 10/15/2020 2:23 PM, nipun.gupta@nxp.com wrote:
>>> From: Nipun Gupta <nipun.gupta@nxp.com>
>>>
>>> This change adds a Rx offload capability and configuration to
>>> enable hardware to drop the packets in case of any error in the
>>> packets 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>
>>> Reviewed-by: Asaf Penso <asafp@nvidia.com>
>>> ---
>>
>> This feature touches many main parts,
>> - new config item for 'rte_eth_dev_configure()'
>> - a new offload flag
>> - new capability reporting for 'rte_eth_dev_info_get()'
>>
>> The feature doesn't look very mainstream to touch all these main parts and add
>> complexity to them, which will affect almost all users.
>>
>> And has some inconsistencies, like configuration is done via config struct, but
>> capability is returned as bit-wise.
>> Or I think config option taken into account only if offload is requested has a
>> chance to confuse people in both app and driver end.
>>
>> What do you think having two specific APIs to get_capabilities and set drop config?
>> The responsibility of those APIs will be clear and narrowed down, which makes it
>> harder to make it wrong.
> 
> I agree. In general, it is better adding new functions
> instead of adding everything in rte_eth_dev_configure().
> 

The set is stale, rejecting it, please send a new version if required.

^ permalink raw reply	[flat|nested] 46+ messages in thread

end of thread, other threads:[~2021-04-20  1:11 UTC | newest]

Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-31  7:53 [dpdk-dev] [PATCH] ethdev: add rx offload to drop error packets Nipun Gupta
2020-08-31 12:58 ` Ferruh Yigit
2020-08-31 16:04   ` Nipun Gupta
2020-08-31 17:00 ` Stephen Hemminger
2020-09-01  8:09   ` Thomas Monjalon
2020-09-01 10:56     ` Nipun Gupta
2020-09-21  7:29     ` Ori Kam
2020-10-05  7:15 ` [dpdk-dev] [PATCH 1/3 v2] " nipun.gupta
2020-10-05  7:15   ` [dpdk-dev] [PATCH 2/3 v2] net/dpaa: support RX offload for error packet drop nipun.gupta
2020-10-05  7:15   ` [dpdk-dev] [PATCH 3/3 v2] testpmd: support hardware offload to drop error packets nipun.gupta
2020-10-08 15:06     ` Asaf Penso
2020-10-08 15:45       ` Nipun Gupta
2020-10-05 15:34   ` [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx " Stephen Hemminger
2020-10-05 16:10     ` Jerin Jacob
2020-10-06 10:37       ` Nipun Gupta
2020-10-06 12:01         ` Jerin Jacob
2020-10-06 13:10           ` Nipun Gupta
2020-10-06 13:13             ` Jerin Jacob
2020-10-08  8:53               ` Nipun Gupta
2020-10-08  8:55                 ` Jerin Jacob
2020-10-08 15:13                   ` Asaf Penso
2020-10-09 13:13 ` [dpdk-dev] [PATCH 1/3 v3] " nipun.gupta
2020-10-09 13:13   ` [dpdk-dev] [PATCH 2/3 v3] net/dpaa: support RX offload for error packet drop nipun.gupta
2020-10-09 13:13   ` [dpdk-dev] [PATCH 3/3 v3] app/testpmd: support hardware offload to drop error packets nipun.gupta
2020-10-11  7:22     ` Asaf Penso
2020-10-11 10:13   ` [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx " Jerin Jacob
2020-10-11 21:41   ` Thomas Monjalon
2020-10-12  5:40     ` Nipun Gupta
2020-10-13  7:22       ` Nipun Gupta
2020-10-12  8:01   ` Andrew Rybchenko
2020-10-12 11:30     ` Nipun Gupta
2020-10-12 12:22       ` Andrew Rybchenko
2020-10-12 12:53         ` Nipun Gupta
2020-10-13  7:21         ` Andrew Rybchenko
2020-10-13  7:36           ` Nipun Gupta
2020-10-13  7:51             ` Andrew Rybchenko
2020-10-13  8:12               ` Nipun Gupta
2020-10-15 13:23 ` [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx " nipun.gupta
2020-10-15 13:23   ` [dpdk-dev] [PATCH 2/3 v4] net/dpaa: support Rx offload for error packet drop nipun.gupta
2020-10-15 13:23   ` [dpdk-dev] [PATCH 3/3 v4] app/testpmd: support hardware offload to drop error packets nipun.gupta
2020-10-29 17:22     ` Dharmik Thakkar
2020-10-31 18:16       ` Nipun Gupta
2020-10-19  3:30   ` [dpdk-dev] [PATCH 1/3 v4] ethdev: add Rx " Ajit Khaparde
2021-02-18 20:32   ` Ferruh Yigit
2021-02-18 20:37     ` Thomas Monjalon
2021-04-20  1:11       ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).