On Thu, Oct 8, 2020 at 2:23 PM Nipun Gupta <nipun.gu...@nxp.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjac...@gmail.com>
> > Sent: Tuesday, October 6, 2020 6:44 PM
> > To: Nipun Gupta <nipun.gu...@nxp.com>
> > Cc: Stephen Hemminger <step...@networkplumber.org>; dpdk-dev
> > <dev@dpdk.org>; Thomas Monjalon <tho...@monjalon.net>; Ferruh Yigit
> > <ferruh.yi...@intel.com>; Andrew Rybchenko <arybche...@solarflare.com>;
> > Hemant Agrawal <hemant.agra...@nxp.com>; Sachin Saxena
> > <sachin.sax...@nxp.com>; Rohit Raj <rohit....@nxp.com>
> > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
> > packets
> >
> > On Tue, Oct 6, 2020 at 6:40 PM Nipun Gupta <nipun.gu...@nxp.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jerin Jacob <jerinjac...@gmail.com>
> > > > Sent: Tuesday, October 6, 2020 5:31 PM
> > > > To: Nipun Gupta <nipun.gu...@nxp.com>
> > > > Cc: Stephen Hemminger <step...@networkplumber.org>; dpdk-dev
> > > > <dev@dpdk.org>; Thomas Monjalon <tho...@monjalon.net>; Ferruh Yigit
> > > > <ferruh.yi...@intel.com>; Andrew Rybchenko
> > <arybche...@solarflare.com>;
> > > > Hemant Agrawal <hemant.agra...@nxp.com>; Sachin Saxena
> > > > <sachin.sax...@nxp.com>; Rohit Raj <rohit....@nxp.com>
> > > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop
> > > > error
> > > > packets
> > > >
> > > > On Tue, Oct 6, 2020 at 4:07 PM Nipun Gupta <nipun.gu...@nxp.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jerin Jacob <jerinjac...@gmail.com>
> > > > > > Sent: Monday, October 5, 2020 9:40 PM
> > > > > > To: Stephen Hemminger <step...@networkplumber.org>
> > > > > > Cc: Nipun Gupta <nipun.gu...@nxp.com>; dpdk-dev <dev@dpdk.org>;
> > > > Thomas
> > > > > > Monjalon <tho...@monjalon.net>; Ferruh Yigit
> > <ferruh.yi...@intel.com>;
> > > > > > Andrew Rybchenko <arybche...@solarflare.com>; Hemant Agrawal
> > > > > > <hemant.agra...@nxp.com>; Sachin Saxena <sachin.sax...@nxp.com>;
> > > > Rohit
> > > > > > Raj <rohit....@nxp.com>
> > > > > > Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to
> > > > > > drop
> > error
> > > > > > packets
> > > > > >
> > > > > > On Mon, Oct 5, 2020 at 9:05 PM Stephen Hemminger
> > > > > > <step...@networkplumber.org> wrote:
> > > > > > >
> > > > > > > On Mon, 5 Oct 2020 12:45:04 +0530
> > > > > > > nipun.gu...@nxp.com wrote:
> > > > > > >
> > > > > > > > From: Nipun Gupta <nipun.gu...@nxp.com>
> > > > > > > >
> > > > > > > > This change adds a RX offload capability, which once enabled,
> > > > > > > > hardware will drop the packets in case there of any error in
> > > > > > > > the packet such as L3 checksum error or L4 checksum.
> > > > > >
> > > > > > IMO, Providing additional support up to the level to choose the
> > > > > > errors
> > > > > > to drops give more control to the application. Meaning,
> > > > > > L1 errors such as FCS error
> > > > > > L2 errors ..
> > > > > > L3 errors such checksum
> > > > > > i.e ethdev spec need to have error level supported by PMD and the
> > > > > > application can set the layers interested to drop.
> > > > >
> > > > > Agree, but 'DEV_RX_OFFLOAD_ERR_PKT_DROP' shall also be there to drop
> > all
> > > > the
> > > > > error packets? Maybe we can rename it to
> > > > DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP.
> > > >
> > > > IMHO, we introduce such shortcut for a single flag for all err drop
> > > > then we can not change the scheme
> > > > without an API/ABI break.
> > >
> > > Are the following offloads fine:
> > > DEV_RX_OFFLOAD_L1_FCS_ERR_PKT_DROP
> > > DEV_RX_OFFLOAD_L3_CSUM_ERR_PKT_DROP
> > > DEV_RX_OFFLOAD_L4_CSUM_ERR_PKT_DROP
> > > DEV_RX_OFFLOAD_ALL_ERR_PKT_DROP
> > >
> > > Please let me know in case I need to add any other too.
> >
> > I think, single offload flags and some config/capability structure to
> > define the additional
> > layer selection would be good, instead of adding a lot of new offload flags.
>
>
> +/**
> + * 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. */
>
> Is this the kind of changes you are talking about?
Yes.
>
> Also, more changes will be there in 'struct rte_eth_dev_info' structure,
> defining
> additional separate capability something like 'uint64_t
> rx_err_drop_offload_capa'.
>
> Regards,
> Nipun
>
> >
> >
> > > Ill send a v3.
> > >
> > > Thanks,
> > > Nipun
> > >
> > > >
> > > > >
> > > > > Currently we have not planned to add separate knobs for separate
> > > > > error in
> > > > > the driver, maybe we can define them separately, or we need have them
> > > > > in
> > > > > this series itself?
> > > >
> > > > I think, ethdev API can have the capability on what are levels it
> > > > supported, in your
> > > > driver case, you can express the same.
> > > >
> > > >
> > > > >
> > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Nipun Gupta <nipun.gu...@nxp.com>
> > > > > > > > Signed-off-by: Rohit Raj <rohit....@nxp.com>
> > > > > > > > ---
> > > > > > > > These patches are based over series:
> > > > > > > >
> > > > > >
> > > >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo
> > > > > >
> > > >
> > rk.dpdk.org%2Fpatch%2F78630%2F&data=02%7C01%7Cnipun.gupta%40nx
> > > > > >
> > > >
> > p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
> > > > > >
> > > >
> > 9c5c301635%7C0%7C0%7C637375110263097933&sdata=RBQswMBsfpM6
> > > > > > nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&reserved=0
> > > > > > > >
> > > > > > > > Changes in v2:
> > > > > > > > - Add support in DPAA1 driver (patch 2/3)
> > > > > > > > - Add support and config parameter in testpmd (patch 3/3)
> > > > > > > >
> > > > > > > > lib/librte_ethdev/rte_ethdev.h | 1 +
> > > > > > > > 1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > Maybe this should be an rte_flow match/action which would then
> > > > > > > make
> > it
> > > > > > > more flexible?
> > > > > >
> > > > > > I think, it is not based on any Patten matching. So IMO, it should
> > > > > > be best
> > if it
> > > > > > is part of RX offload.
> > > > > >
> > > > > > >
> > > > > > > There is not much of a performance gain for this in real life and
> > > > > > > if only one driver supports it then I am not convinced this is
> > > > > > > needed.
> > > > > >
> > > > > > Marvell HW has this feature.
