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 98C1241BBE; Fri, 3 Feb 2023 16:12:34 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2F7514067B; Fri, 3 Feb 2023 16:12:34 +0100 (CET) Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by mails.dpdk.org (Postfix) with ESMTP id E37954021E for ; Fri, 3 Feb 2023 16:12:32 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 2D9B35C012A; Fri, 3 Feb 2023 10:12:32 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Fri, 03 Feb 2023 10:12:32 -0500 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=fm3; t=1675437152; x= 1675523552; bh=U39Bj1P/Tle0RA0f6ZxWGojIoeldVMHYI/HiN3N/04k=; b=O 0xz4HgT81QLbLgo88I8Svmq2WbBHtrZinkz3zE0t6RxEDiTHbPH9SBtyPpXVFe2X 44A6nV9iiw9IaUa6xg0bDq9/LP/iijqT6I04HFOOvv3qX1ix9+shlbxx0koIp0o1 hrim560ncxFk7ekkTNXyLN1X0Ir2avrgtWe36M3YFn+q4jfjcisLsjTVuvFISvkS WmBDPDyVQpNTwCok4o9RZMWm/6XGVXsSLRuFHhgN00P8N+Jb2P8wv7rUkh+3nHuj bQUiuUn8i8uPmxZwRVqZFeL3Lh/XCOEv5HQ/52Ajs57OoSQP3jL17NYGzwVB9uII qPK6Rf2rJes88i3lv5TyA== 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=fm3; t=1675437152; x= 1675523552; bh=U39Bj1P/Tle0RA0f6ZxWGojIoeldVMHYI/HiN3N/04k=; b=N RK7+EgNMsJUPF0EupNstUfNZfn0sghxF5/REXlFCJmPD+BHHUBGO5WLVOu1fd+y7 Q8Z5aMVr3lUocGbYrBl5w5TowKdoAJO8OG+y8/UWFrRKAZMPmC3IuRqkzRJQmGhj iyVfDG0hZDGqjiHwPA1ntwAe0882sWRAyTZM09dg6QzwpZo52EzgradlFHYpUQbE 6+iTaLRzDxFX+bvNmerNphWa0DExnCJUo3tr2bpUwQFyJHtIZ3TKVQAAHLzQJ5/M O4FiNmUOLylvctzrHcJlEGjwqh3wtFYDWP1Q/9S8klaE5sfCB0zYaJ+A+aIYbjWg y6Q26U+IupNw2D38gReAg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvhedrudegtddgjedtucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvvefufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpedtjeeiieefhedtfffgvdelteeufeefheeujefgueetfedttdei kefgkeduhedtgfenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfh hrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Fri, 3 Feb 2023 10:12:29 -0500 (EST) From: Thomas Monjalon To: Ferruh Yigit Cc: Wisam Jaddo , Ori Kam , Aman Singh , Yuying Zhang , Ajit Khaparde , Somnath Kotur , Hemant Agrawal , Sachin Saxena , Matan Azrad , Viacheslav Ovsiienko , Chaoyong He , Niklas =?ISO-8859-1?Q?S=F6derlund?= , Andrew Rybchenko , Olivier Matz , David Marchand , dev@dpdk.org Subject: Re: [PATCH v6 4/8] ethdev: use GRE protocol struct for flow matching Date: Fri, 03 Feb 2023 16:12:27 +0100 Message-ID: <10218004.nUPlyArG6x@thomas> In-Reply-To: References: <20221025214410.715864-1-thomas@monjalon.net> <837964623.0ifERbkFSE@thomas> 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 03/02/2023 16:02, Ferruh Yigit: > On 2/2/2023 5:16 PM, Thomas Monjalon wrote: > > 02/02/2023 13:44, Ferruh Yigit: > >> --- a/lib/net/rte_gre.h > >> +++ b/lib/net/rte_gre.h > >> @@ -28,6 +28,8 @@ extern "C" { > >> */ > >> __extension__ > >> struct rte_gre_hdr { > >> + union { > >> + struct { > >> #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN > >> uint16_t res2:4; /**< Reserved */ > >> uint16_t s:1; /**< Sequence Number Present bit */ > >> @@ -45,6 +47,9 @@ struct rte_gre_hdr { > >> uint16_t res3:5; /**< Reserved */ > >> uint16_t ver:3; /**< Version Number */ > >> #endif > >> + }; > >> + rte_be16_t c_rsvd0_ver; > >> + }; > >> uint16_t proto; /**< Protocol Type */ > > > > Why adding an unioned field c_rsvd0_ver? > > > > > > Because existing usage in the drivers require to access these fields as > a single 16 bytes variable. > > like mlx was using it as: > `X(SET_BE16, gre_c_ver, v->c_rsvd0_ver, rte_flow_item_gre) \` > > When all usage switched to flow item specific fields to generic headers, > there needs a way to represent this in the generic header. > > By adding 'c_rsvd0_ver' to generic header it becomes: > `X(SET_BE16, gre_c_ver, v->hdr.c_rsvd0_ver, rte_flow_item_gre) \` > > > Or another sample, previous version of code was updated as following: > ` > - size = sizeof(((struct rte_flow_item_gre *)NULL)->c_rsvd0_ver); > + size = sizeof(((struct rte_flow_item_gre *)NULL)->hdr.proto); > ` > > Because generic field to represent 'c_rsvd0_ver' is missing, 'hdr.proto' > was used, this was wrong. > Although the sizes of fields are same and functionally works, they are > different fields, this is worse than sizeof(uint16_t); > > > Another usage in testpmd: > ` > [ITEM_GRE_C_RSVD0_VER] = { > .name = "c_rsvd0_ver", > @@ -4082,7 +4082,7 @@ static const struct token token_list[] = { > .next = NEXT(item_gre, NEXT_ENTRY(COMMON_UNSIGNED), > item_param), > .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_gre, > - c_rsvd0_ver)), > + hdr.c_rsvd0_ver)), > ` > > > But looking it again perhaps it can be named differently, because it is > not a reserved field in the generic header, though I am not sure what > can be a good variable name. The best would be to try not using this field at all. I suggest to remove this patch from the series for now. I could try to work on it for the next release.