On Tue, Oct 6, 2020 at 6:40 PM Nipun Gupta <[email protected]> wrote: > > > > > -----Original Message----- > > From: Jerin Jacob <[email protected]> > > Sent: Tuesday, October 6, 2020 5:31 PM > > To: Nipun Gupta <[email protected]> > > Cc: Stephen Hemminger <[email protected]>; dpdk-dev > > <[email protected]>; Thomas Monjalon <[email protected]>; Ferruh Yigit > > <[email protected]>; Andrew Rybchenko <[email protected]>; > > Hemant Agrawal <[email protected]>; Sachin Saxena > > <[email protected]>; Rohit Raj <[email protected]> > > 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 <[email protected]> wrote: > > > > > > > > > > > > > -----Original Message----- > > > > From: Jerin Jacob <[email protected]> > > > > Sent: Monday, October 5, 2020 9:40 PM > > > > To: Stephen Hemminger <[email protected]> > > > > Cc: Nipun Gupta <[email protected]>; dpdk-dev <[email protected]>; > > Thomas > > > > Monjalon <[email protected]>; Ferruh Yigit <[email protected]>; > > > > Andrew Rybchenko <[email protected]>; Hemant Agrawal > > > > <[email protected]>; Sachin Saxena <[email protected]>; > > Rohit > > > > Raj <[email protected]> > > > > 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 > > > > <[email protected]> wrote: > > > > > > > > > > On Mon, 5 Oct 2020 12:45:04 +0530 > > > > > [email protected] wrote: > > > > > > > > > > > From: Nipun Gupta <[email protected]> > > > > > > > > > > > > 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 <[email protected]> > > > > > > Signed-off-by: Rohit Raj <[email protected]> > > > > > > --- > > > > > > These patches are based over series: > > > > > > > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo > > > > > > rk.dpdk.org%2Fpatch%2F78630%2F&data=02%7C01%7Cnipun.gupta%40nx > > > > > > p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9 > > > > > > 9c5c301635%7C0%7C0%7C637375110263097933&sdata=RBQswMBsfpM6 > > > > nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&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.
