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

[Asaf Penso <asafp@nvidia.com>]

>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of Jerin Jacob
>Sent: Thursday, October 8, 2020 11:56 AM
>To: Nipun Gupta <nipun.gupta@nxp.com>
>Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
><dev@dpdk.org>; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
>Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
><arybchenko@solarflare.com>; Hemant Agrawal
><hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
>Rohit Raj <rohit.raj@nxp.com>
>Subject: Re: [dpdk-dev] [PATCH 1/3 v2] ethdev: add rx offload to drop error
>packets
>
>On Thu, Oct 8, 2020 at 2:23 PM Nipun Gupta <nipun.gupta@nxp.com> wrote:
>>
>>
>>
>> > -----Original Message-----
>> > From: Jerin Jacob <jerinjacobk@gmail.com>
>> > Sent: Tuesday, October 6, 2020 6:44 PM
>> > To: Nipun Gupta <nipun.gupta@nxp.com>
>> > Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
>> > <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh
>Yigit
>> > <ferruh.yigit@intel.com>; Andrew Rybchenko
>> > <arybchenko@solarflare.com>; Hemant Agrawal
>> > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
>> > Rohit Raj <rohit.raj@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.gupta@nxp.com>
>wrote:
>> > >
>> > >
>> > >
>> > > > -----Original Message-----
>> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
>> > > > Sent: Tuesday, October 6, 2020 5:31 PM
>> > > > To: Nipun Gupta <nipun.gupta@nxp.com>
>> > > > Cc: Stephen Hemminger <stephen@networkplumber.org>; dpdk-dev
>> > > > <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>;
>Ferruh
>> > > > Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
>> > <arybchenko@solarflare.com>;
>> > > > Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
>> > > > <sachin.saxena@nxp.com>; Rohit Raj <rohit.raj@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.gupta@nxp.com>
>wrote:
>> > > > >
>> > > > >
>> > > > >
>> > > > > > -----Original Message-----
>> > > > > > From: Jerin Jacob <jerinjacobk@gmail.com>
>> > > > > > Sent: Monday, October 5, 2020 9:40 PM
>> > > > > > To: Stephen Hemminger <stephen@networkplumber.org>
>> > > > > > Cc: Nipun Gupta <nipun.gupta@nxp.com>; dpdk-dev
>> > > > > > <dev@dpdk.org>;
>> > > > Thomas
>> > > > > > Monjalon <thomas@monjalon.net>; Ferruh Yigit
>> > <ferruh.yigit@intel.com>;
>> > > > > > Andrew Rybchenko <arybchenko@solarflare.com>; Hemant
>Agrawal
>> > > > > > <hemant.agrawal@nxp.com>; Sachin Saxena
>> > > > > > <sachin.saxena@nxp.com>;
>> > > > Rohit
>> > > > > > Raj <rohit.raj@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
>> > > > > > <stephen@networkplumber.org> wrote:
>> > > > > > >
>> > > > > > > On Mon,  5 Oct 2020 12:45:04 +0530 nipun.gupta@nxp.com
>> > > > > > > wrote:
>> > > > > > >
>> > > > > > > > From: Nipun Gupta <nipun.gupta@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.gupta@nxp.com>
>> > > > > > > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
>> > > > > > > > ---
>> > > > > > > > These patches are based over series:
>> > > > > > > >
>> > > > > >
>> > > >
>> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
>> > tchwo
>> > > > > >
>> > > >
>> >
>rk.dpdk.org%2Fpatch%2F78630%2F&amp;data=02%7C01%7Cnipun.gupta%40
>nx
>> > > > > >
>> > > >
>> >
>p.com%7C90b516fd465c48945e7008d869492b3e%7C686ea1d3bc2b4c6fa92cd9
>> > > > > >
>> > > >
>> >
>9c5c301635%7C0%7C0%7C637375110263097933&amp;sdata=RBQswMBsfpM6
>> > > > > >
>nyKur%2FaHvOMvNK7RU%2BRyhHt%2FXBsP1OM%3D&amp;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.
Reviewed-By: Asaf Penso <asafp@nvidia.com>