| Message ID | 20201005071506.28861-1-nipun.gupta@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3,v2] ethdev: add rx offload to drop error packets | expand |
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.
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.
> -----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&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.
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&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.
> -----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&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.
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&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.
> -----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&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.
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&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.
>-----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&data=02%7C01%7Cnipun.gupta%40 >nx >> > > > > > >> > > > >> > >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. Reviewed-By: Asaf Penso <asafp@nvidia.com>
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 | \