From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 63191A04B6; Mon, 12 Oct 2020 14:22:53 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 09F311D6E5; Mon, 12 Oct 2020 14:22:52 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by dpdk.org (Postfix) with ESMTP id 95B981D6E2 for ; Mon, 12 Oct 2020 14:22:49 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 3DAFD7F57E; Mon, 12 Oct 2020 15:22:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 3DAFD7F57E DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1602505368; bh=MRcEU1c3Mb+nwk2wyOxo7OZvYtJGTgzdWGGp9SrQ3eY=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=xQWG8rzsoVd3hR31uRlMGgekzEhgpZQtFzyyNMu3vGukqHoxx+7M4K7oR7RCmy8q1 cNaOqT0Ru88VGYYgqAIxfvbr5y+d1xpVD2fXOi+ag/I4uhGnB1Nvq7r+lEFwWkCszW OMhLOIfxEqsrediXDyiJA3ZOsUO3zqtE/pY1QMFU= To: Nipun Gupta , "dev@dpdk.org" Cc: "thomas@monjalon.net" , "ferruh.yigit@intel.com" , "arybchenko@solarflare.com" , Hemant Agrawal , Sachin Saxena , Rohit Raj , "jerinjacobk@gmail.com" , "stephen@networkplumber.org" , "asafp@nvidia.com" References: <20200831075333.10135-1-nipun.gupta@nxp.com> <20201009131331.5897-1-nipun.gupta@nxp.com> <794e2ad5-35a4-1b84-2ce4-0df059806f39@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: Date: Mon, 12 Oct 2020 15:22:48 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 1/3 v3] ethdev: add rx offload to drop error packets X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 10/12/20 2:30 PM, Nipun Gupta wrote: > > >> -----Original Message----- >> From: Andrew Rybchenko >> Sent: Monday, October 12, 2020 1:32 PM >> To: Nipun Gupta ; dev@dpdk.org >> Cc: thomas@monjalon.net; ferruh.yigit@intel.com; arybchenko@solarflare.com; >> Hemant Agrawal ; Sachin Saxena >> ; Rohit Raj ; >> 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 >> >> On 10/9/20 4:13 PM, nipun.gupta@nxp.com wrote: >>> From: Nipun Gupta >>> >>> 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 >>> Signed-off-by: Rohit Raj >>> Reviewed-by: Asaf Penso >>> --- >>> >>> 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. > >> >>> +}; >>> + >>> /** >>> * 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 */ >>> >