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

[Nipun Gupta]

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 at monjalon.net>; jerinjacobk at gmail.com
> Cc: dev at dpdk.org; 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>;
> stephen at networkplumber.org; asafp at nvidia.com
> Subject: RE: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error
> packets
> 
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas at monjalon.net>
> > Sent: Monday, October 12, 2020 3:12 AM
> > To: Nipun Gupta <nipun.gupta at nxp.com>
> > Cc: dev at dpdk.org; 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
> >
> > Hi,
> >
> > The configuration of this feature is not clear to me.
> > Please see the comments below.
> >
> > 09/10/2020 15:13, nipun.gupta at nxp.com:
> > > 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>
> > > ---
> > [...]
> > > +/**
> > > + * 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
> 
> >
> >