From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 8FB40A0503; Tue, 17 May 2022 23:12:19 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 432A34068B; Tue, 17 May 2022 23:12:19 +0200 (CEST) Received: from wout2-smtp.messagingengine.com (wout2-smtp.messagingengine.com [64.147.123.25]) by mails.dpdk.org (Postfix) with ESMTP id 5295C40041 for ; Tue, 17 May 2022 23:12:17 +0200 (CEST) Received: from compute2.internal (compute2.nyi.internal [10.202.2.46]) by mailout.west.internal (Postfix) with ESMTP id BB417320016F; Tue, 17 May 2022 17:12:13 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute2.internal (MEProxy); Tue, 17 May 2022 17:12:15 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to; s=fm1; t=1652821933; x= 1652908333; bh=XvAr5TpOMO5bQ2zMQuXBhw4rKDL4Rtohp4ltLAwQIL0=; b=b 2nLxdWyIk2INX8hPrgsQpbkSwFyfyARLDTH0aPqQBSm524NG2g8lmiSWETt0SHPt eDfkfkAkc3Tlvy1UINZfGNHNzkpSe+cvObvPbfPZVjMQHxmgndB6OGM33l9e+2lp QNfS4/+gVD/PGRShe//1zKwNub8sY+tsMPJwqR7zZVkB59nqi+iQ01zSSHPm+T8A iTYd+SYPDZ7Wstck2wkgLi9d2Use+NWw5h9Sfa3utfrOT35cKQLAkl7D5ZDrWLBN 6jp6dXtEvaylR38WIv4p9v1QAfexYD2A2nCc3kSSHsf3IXJysUhI7+82btURLBqH ic9JJDF3F+ZpwIbFAm9kQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:feedback-id:feedback-id:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1652821933; x= 1652908333; bh=XvAr5TpOMO5bQ2zMQuXBhw4rKDL4Rtohp4ltLAwQIL0=; b=X 1Y1UBRgvt3xd3l28OezVXtorwS2rIPcZDoAuO1jR4LcZe2of7O/zUSINIMj8HKiz nc+KbL4D6J34HztnX1zo7HA73JV1TipRgcJu2VgJ5lwSB4vlxzLH+TgPigQLHCS/ jIG0Y8WUc+Zxvd8/OBsmI06V7gcEf+vj+i3g306fySuoixv19v6FOZhVmAbB1CHw 7KB/Kk1ES/fymq5O2uL96DZGRQdgnbR7OxuJrFiozAg2WvTIqAnGihkuT+avT9Au HdLdLv4b/Hz3lB2yBy7e/EtDksMGY6go4yheNdUMTmXtyZl+6LSpyutrphyWgPrS oZ/Vo6DhpkBjuGANdDo9Q== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrheejgdduheegucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpefhvfevufffkfgjfhgggfgtsehtuf ertddttddvnecuhfhrohhmpefvhhhomhgrshcuofhonhhjrghlohhnuceothhhohhmrghs sehmohhnjhgrlhhonhdrnhgvtheqnecuggftrfgrthhtvghrnheptdejieeifeehtdffgf dvleetueeffeehueejgfeuteeftddtieekgfekudehtdfgnecuvehluhhsthgvrhfuihii vgeptdenucfrrghrrghmpehmrghilhhfrhhomhepthhhohhmrghssehmohhnjhgrlhhonh drnhgvth X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 17 May 2022 17:12:09 -0400 (EDT) From: Thomas Monjalon To: xuan.ding@intel.com, yuanx.wang@intel.com, wenxuanx.wu@intel.com Cc: andrew.rybchenko@oktetlabs.ru, xiaoyun.li@intel.com, ferruh.yigit@xilinx.com, aman.deep.singh@intel.com, dev@dpdk.org, yuying.zhang@intel.com, qi.z.zhang@intel.com, jerinjacobk@gmail.com, stephen@networkplumber.org, mb@smartsharesystems.com, viacheslavo@nvidia.com, ping.yu@intel.com Subject: Re: [PATCH v5 1/4] lib/ethdev: introduce protocol type based buffer split Date: Tue, 17 May 2022 23:12:00 +0200 Message-ID: <13015742.y0N7aAr316@thomas> In-Reply-To: <20220426111338.1074785-2-wenxuanx.wu@intel.com> References: <20220402104109.472078-2-wenxuanx.wu@intel.com> <20220426111338.1074785-1-wenxuanx.wu@intel.com> <20220426111338.1074785-2-wenxuanx.wu@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hello, It seems you didn't try to address my main comment on v4: " Before doing anything, the first patch of this series should make the current status clearer. Example, this line does not explain what it does: uint16_t split_hdr_size; /**< hdr buf size (header_split enabled).*/ And header_split has been removed in ab3ce1e0c193 ("ethdev: remove old offload API") If RTE_ETH_RX_OFFLOAD_HEADER_SPLIT is not needed, let's add a comment to start a deprecation. " Also the comment from Andrew about removing limitation to 2 packets is not addressed. All the part about the protocols capability is missing here. It is not encouraging. 26/04/2022 13:13, wenxuanx.wu@intel.com: > From: Wenxuan Wu > > Protocol based buffer split consists of splitting a received packet into two > separate regions based on its content. The split happens after the packet > protocol header and before the packet payload. Splitting is usually between > the packet protocol header that can be posted to a dedicated buffer and the > packet payload that can be posted to a different buffer. > > Currently, Rx buffer split supports length and offset based packet split. > protocol split is based on buffer split, configuring length of buffer split > is not suitable for NICs that do split based on protocol types. Why? Is it impossible to support length split on Intel NIC? > Because tunneling makes the conversion from length > to protocol type impossible. This is not a HW issue. I agree on the need but that a different usage than length split. > This patch extends the current buffer split to support protocol and offset > based buffer split. A new proto field is introduced in the rte_eth_rxseg_split > structure reserved field to specify header protocol type. With Rx queue > offload RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT enabled and corresponding protocol > type configured. PMD will split the ingress packets into two separate regions. > Currently, both inner and outer L2/L3/L4 level protocol based buffer split > can be supported. > > For example, let's suppose we configured the Rx queue with the > following segments: > seg0 - pool0, off0=2B > seg1 - pool1, off1=128B > > With protocol split type configured with RTE_PTYPE_L4_UDP. The packet > consists of MAC_IP_UDP_PAYLOAD will be splitted like following: > seg0 - udp header @ RTE_PKTMBUF_HEADROOM + 2 in mbuf from pool0 > seg1 - payload @ 128 in mbuf from pool1 Not clear what is the calculation. > The memory attributes for the split parts may differ either - for example > the mempool0 and mempool1 belong to dpdk memory and external memory, > respectively. > > Signed-off-by: Xuan Ding > Signed-off-by: Yuan Wang > Signed-off-by: Wenxuan Wu > Reviewed-by: Qi Zhang > --- > lib/ethdev/rte_ethdev.c | 36 +++++++++++++++++++++++++++++------- > lib/ethdev/rte_ethdev.h | 15 ++++++++++++++- > 2 files changed, 43 insertions(+), 8 deletions(-) > > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c > index 29a3d80466..1a2bc172ab 100644 > --- a/lib/ethdev/rte_ethdev.c > +++ b/lib/ethdev/rte_ethdev.c > @@ -1661,6 +1661,7 @@ rte_eth_rx_queue_check_split(const struct rte_eth_rxseg_split *rx_seg, > struct rte_mempool *mpl = rx_seg[seg_idx].mp; > uint32_t length = rx_seg[seg_idx].length; > uint32_t offset = rx_seg[seg_idx].offset; > + uint32_t proto = rx_seg[seg_idx].proto; > > if (mpl == NULL) { > RTE_ETHDEV_LOG(ERR, "null mempool pointer\n"); > @@ -1694,13 +1695,34 @@ rte_eth_rx_queue_check_split(const struct rte_eth_rxseg_split *rx_seg, > } > offset += seg_idx != 0 ? 0 : RTE_PKTMBUF_HEADROOM; > *mbp_buf_size = rte_pktmbuf_data_room_size(mpl); > - length = length != 0 ? length : *mbp_buf_size; > - if (*mbp_buf_size < length + offset) { > - RTE_ETHDEV_LOG(ERR, > - "%s mbuf_data_room_size %u < %u (segment length=%u + segment offset=%u)\n", > - mpl->name, *mbp_buf_size, > - length + offset, length, offset); > - return -EINVAL; > + if (proto == 0) { Add a comment here, /* split at fixed length */ > + length = length != 0 ? length : *mbp_buf_size; > + if (*mbp_buf_size < length + offset) { > + RTE_ETHDEV_LOG(ERR, > + "%s mbuf_data_room_size %u < %u (segment length=%u + segment offset=%u)\n", > + mpl->name, *mbp_buf_size, > + length + offset, length, offset); > + return -EINVAL; > + } > + } else { Add a comment here, /* split after specified protocol header */ > + /* Ensure n_seg is 2 in protocol based buffer split. */ > + if (n_seg != 2) { (should be a space, not a tab before brace) Why do you limit the feature to 2 segments only? > + RTE_ETHDEV_LOG(ERR, "number of buffer split protocol segments should be 2.\n"); > + return -EINVAL; > + } > + /* Length and protocol are exclusive here, so make sure length is 0 in protocol > + based buffer split. */ > + if (length != 0) { > + RTE_ETHDEV_LOG(ERR, "segment length should be set to zero in buffer split\n"); > + return -EINVAL; > + } > + if (*mbp_buf_size < offset) { > + RTE_ETHDEV_LOG(ERR, > + "%s mbuf_data_room_size %u < %u segment offset)\n", > + mpl->name, *mbp_buf_size, > + offset); > + return -EINVAL; [...] > + * - The proto in the elements defines the split position of received packets. > + * > * - If the length in the segment description element is zero > * the actual buffer size will be deduced from the appropriate > * memory pool properties. > @@ -1197,12 +1200,21 @@ struct rte_eth_txmode { > * - pool from the last valid element > * - the buffer size from this pool > * - zero offset > + * > + * - Length based buffer split: > + * - mp, length, offset should be configured. > + * - The proto should not be configured in length split. Zero default. > + * > + * - Protocol based buffer split: > + * - mp, offset, proto should be configured. > + * - The length should not be configured in protocol split. Zero default. What means "Zero default"? You should just ignore the non relevant field. > struct rte_eth_rxseg_split { > struct rte_mempool *mp; /**< Memory pool to allocate segment from. */ > uint16_t length; /**< Segment data length, configures split point. */ > uint16_t offset; /**< Data offset from beginning of mbuf data buffer. */ > - uint32_t reserved; /**< Reserved field. */ How do you manage ABI compatibility? Was the reserved field initialized to 0 in previous versions? > + uint32_t proto; /**< Protocol of buffer split, determines protocol split point. */ What are the values for "proto"? > @@ -1664,6 +1676,7 @@ struct rte_eth_conf { > RTE_ETH_RX_OFFLOAD_QINQ_STRIP) > #define DEV_RX_OFFLOAD_VLAN RTE_DEPRECATED(DEV_RX_OFFLOAD_VLAN) RTE_ETH_RX_OFFLOAD_VLAN > > + It looks to be an useless change. > /* > * If new Rx offload capabilities are defined, they also must be > * mentioned in rte_rx_offload_names in rte_ethdev.c file. >