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

[Nipun Gupta <nipun.gupta@nxp.com>]

> -----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

> 
>