Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error packets

[Nipun Gupta <nipun.gupta@nxp.com>]

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