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

[Nipun Gupta]

> -----Original Message-----
> From: Andrew Rybchenko <Andrew.Rybchenko at oktetlabs.ru>
> Sent: Monday, October 12, 2020 1:32 PM
> To: Nipun Gupta <nipun.gupta at nxp.com>; dev at dpdk.org
> Cc: thomas at monjalon.net; ferruh.yigit at intel.com; arybchenko at solarflare.com;
> Hemant Agrawal <hemant.agrawal at nxp.com>; Sachin Saxena
> <sachin.saxena at nxp.com>; Rohit Raj <rohit.raj at nxp.com>;
> jerinjacobk at gmail.com; stephen at networkplumber.org; asafp at 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 at nxp.com wrote:
> > From: Nipun Gupta <nipun.gupta at 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 at nxp.com>
> > Signed-off-by: Rohit Raj <rohit.raj at nxp.com>
> > Reviewed-by: Asaf Penso <asafp at 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 */
> >