| Message ID | 20201009131331.5897-1-nipun.gupta@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3,v3] ethdev: add rx offload to drop error packets | expand |
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 >
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?
> -----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 > >
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 */ >
> -----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 */ > >
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 */ >>> >
> -----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 */ > >>> > >
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.
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 > > > > >
> -----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.
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.
> -----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?
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 */