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 961A841BB3; Thu, 2 Feb 2023 22:23:32 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2D8D840EDC; Thu, 2 Feb 2023 22:23:32 +0100 (CET) Received: from wout4-smtp.messagingengine.com (wout4-smtp.messagingengine.com [64.147.123.20]) by mails.dpdk.org (Postfix) with ESMTP id 5590940693 for ; Thu, 2 Feb 2023 22:23:31 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.west.internal (Postfix) with ESMTP id 743AB32006F5; Thu, 2 Feb 2023 16:23:29 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Thu, 02 Feb 2023 16:23:30 -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=1675373009; x= 1675459409; bh=C9oeN5UaR2A2l86deAfK9n5z5W4Tza86IQNn921j68A=; b=O REe7BKYqqtIRvRnan9cU0y/3saM7/bVZdyL0hXvlywq1kNGT+yBxhgHT9IO0bXrX hEvew+1k2+3q9AhSgREYuhoK1At5t4250Qa8D7kfetBECpKbpGBP/H/NZzm5QnXl 9tlvBw4OA8VDt55NHufG73Dp9KGnAoYugzQ8x86bXKXH9uQ3OpYmpZN+baihcC4T 7POnIpL14M0MU7bE99LlFoirOUoQkk5N3CvpEPFWRzj6EV9wNf0ectiud9X5eMJS osihEDfapM/GVg9obe/EFMty4/4Zd/hQVniPsrEp4STYoBpR9ZUHW6VFDiXB7Gd9 meJAPvYPK7nmA0VRpEuJA== 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=1675373009; x= 1675459409; bh=C9oeN5UaR2A2l86deAfK9n5z5W4Tza86IQNn921j68A=; b=D BujlJ0LWHPo57vGdN/hqPGmIgLkwurLvXuA729h0UnXN8QCtfIcZbY3G+wJ5OKPE qeblPDZsYsP4fRkGTPbevpgacSciVJyrpxQ24DVHjKU30uevA/7dCk8Z6Gr7THhI 3VFnCVCndvnzR2/JzMlay15/8TNYcWLnYJo7NH/dk2Lbt26WkQHwWAZwwpHkP7Aa 1tHCvnwjJQEA/Xkf6bVYcHqBam32MTF2HfI31yZoNZKIAmnSORaCqwH82J+VvVjQ RF9FLMlWguihDwDyntCBASKWHQ8bMWPXztl+E/hyW3jznnL5ZTKS5YYh9m+DHly3 dfROiAvOj+6b5xwFh/B2A== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvhedrudefkedgudeghecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefhvfevufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhho mhgrshcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqne cuggftrfgrthhtvghrnheptdejieeifeehtdffgfdvleetueeffeehueejgfeuteeftddt ieekgfekudehtdfgnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilh hfrhhomhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 2 Feb 2023 16:23:27 -0500 (EST) From: Thomas Monjalon To: "Leo Xu (Networking SW)" Cc: "dev@dpdk.org" , Bing Zhao , Ori Kam , Aman Singh , Yuying Zhang , Ferruh Yigit , Andrew Rybchenko , Olivier Matz , "david.marchand@redhat.com" Subject: Re: [PATCH v2 1/3] ethdev: add ICMPv6 ID and sequence Date: Thu, 02 Feb 2023 22:23:25 +0100 Message-ID: <1991609.QkHrqEjB74@thomas> In-Reply-To: References: <20221212085923.2314350-1-yongquanx@nvidia.com> <6520018.4vTCxPXJkl@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 02/02/2023 19:33, Leo Xu (Networking SW): > > 31/01/2023 07:53, Leo Xu (Networking SW): > > > From: Thomas Monjalon > > > > 20/12/2022 08:44, Leo Xu: > > > > > +/** > > > > > + * ICMP6 header > > > > > + */ > > > > > +struct rte_icmp6_hdr { > > > > > + uint8_t type; > > > > > + uint8_t code; > > > > > + rte_be16_t checksum; > > > > > +} __rte_packed; > > > > > + > > > > > +/** > > > > > + * ICMP6 echo > > > > > + */ > > > > > +struct rte_icmp6_echo { > > > > > + struct rte_icmp6_hdr hdr; > > > > > + rte_be16_t identifier; > > > > > + rte_be16_t sequence; > > > > > +} __rte_packed; > > > > > > > > It is exactly the same as struct rte_icmp_hdr. > > > > Why not reuse it? > > > > Maybe introduce struct rte_icmp_base_hdr and define > > > > rte_icmp_echo_hdr as rte_icmp_hdr? > > > > > > Hi Thomas, > > > Looks like, using rte_icmp_hdr as base header for both icmp and icmpv6 is > > not that good. > > > since, rte_icmp_hdr default their headers always having id and sequence > > fields, which is not applicable for most other icmp6/icmp types packets. > > > > > > I may suggest to keep icmp and icmp6 structures independent against each > > other, because, looks like these two protocols definitions do not share common > > base. > > > > What about introducing rte_icmp_base_hdr? > > We should try to avoid duplicating things. > > > > You mean introduce rte_icmp_base_hdr like following? > struct rte_icmp_base_hdr { > uint8_t icmp_type; > uint8_t icmp_code; > rte_be16_t icmp_cksum; > } __rte_packed; > > And change the existing rte_icmp_hdr to be: > struct rte_icmp_hdr { > rte_icmp_base_hdr bash_hdr; > rte_be16_t icmp_ident; > rte_be16_t icmp_seq_nb; > } __rte_packed; > #define rte_icmp6_echo struct rte_icmp_hdr; > > If it is, there will be some compatibilities issues, since we changed existing structure. > > Or, maybe I'm missing something. > Would you help to give more details about above comment? Currently we have this: struct rte_icmp_hdr { uint8_t icmp_type; /* ICMP packet type. */ uint8_t icmp_code; /* ICMP packet code. */ rte_be16_t icmp_cksum; /* ICMP packet checksum. */ rte_be16_t icmp_ident; /* ICMP packet identifier. */ rte_be16_t icmp_seq_nb; /* ICMP packet sequence number. */ } __rte_packed; I agree we can move some fields in a base struct, it would change the API. We could manage with a union, but we would lose the benefit. It looks like we need to keep rte_icmp_hdr as is. So we need to duplicate and define new structs. What about removing the "6" from the new structs, so it would apply both to IPv4 and IPv6? struct rte_icmp_base_hdr { uint8_t type; uint8_t code; rte_be16_t checksum; } __rte_packed; struct rte_icmp_echo_hdr { struct rte_icmp_base_hdr base; rte_be16_t identifier; rte_be16_t sequence; } __rte_packed;