> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
> Sent: Monday, October 12, 2020 5:53 PM
> To: Nipun Gupta <nipun.gu...@nxp.com>; dev@dpdk.org
> Cc: tho...@monjalon.net; ferruh.yi...@intel.com; arybche...@solarflare.com;
> Hemant Agrawal <hemant.agra...@nxp.com>; Sachin Saxena
> <sachin.sax...@nxp.com>; Rohit Raj <rohit....@nxp.com>;
> jerinjac...@gmail.com; step...@networkplumber.org; as...@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.rybche...@oktetlabs.ru>
> >> Sent: Monday, October 12, 2020 1:32 PM
> >> To: Nipun Gupta <nipun.gu...@nxp.com>; dev@dpdk.org
> >> Cc: tho...@monjalon.net; ferruh.yi...@intel.com;
> arybche...@solarflare.com;
> >> Hemant Agrawal <hemant.agra...@nxp.com>; Sachin Saxena
> >> <sachin.sax...@nxp.com>; Rohit Raj <rohit....@nxp.com>;
> >> jerinjac...@gmail.com; step...@networkplumber.org; as...@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.gu...@nxp.com wrote:
> >>> From: Nipun Gupta <nipun.gu...@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.gu...@nxp.com>
> >>> Signed-off-by: Rohit Raj <rohit....@nxp.com>
> >>> Reviewed-by: Asaf Penso <as...@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 */
> >>>
> >
