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 AB92B45D70; Thu, 21 Nov 2024 21:51:46 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9A70E402DB; Thu, 21 Nov 2024 21:51:46 +0100 (CET) Received: from fhigh-a6-smtp.messagingengine.com (fhigh-a6-smtp.messagingengine.com [103.168.172.157]) by mails.dpdk.org (Postfix) with ESMTP id 700DB402A7; Thu, 21 Nov 2024 21:51:44 +0100 (CET) Received: from phl-compute-04.internal (phl-compute-04.phl.internal [10.202.2.44]) by mailfhigh.phl.internal (Postfix) with ESMTP id E1B7B1140098; Thu, 21 Nov 2024 15:51:43 -0500 (EST) Received: from phl-mailfrontend-02 ([10.202.2.163]) by phl-compute-04.internal (MEProxy); Thu, 21 Nov 2024 15:51:43 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to; s=fm1; t=1732222303; x=1732308703; bh=0A5W/6LcR7vCPf89H7iYLb4tJeGeVHTsKK5XbB8o+q0=; b= UampBrZlipAbKEMQKRK4bWNZKNQnpZa/7UCbiE9YlPA0JlehxZaH0JW8SqT2JeZH OmjmgTUanxPUKbjJDbZNz6DZfZOxV2Bv1kb0YurwkkfWR02lvWaLXx1Wxudcojvp lH5NLXW/kkDnelc+ia0gdlRN7q0WrUDOmZ+X4WZmAOE2ueoqRj8EhsycR4ZOU95n e6mZXGn7OZ7TbUhhg3KTuJgZch+CDyw1VI/0U2hzr8mhcpfPcFEGpE+RQ/vzU9sw MBBfPxdbeieenm8W0bdLv38WIR85LSYYKqPedpwT1Dj150TrUOztVS6TkCZt7W2R TKKRg1Fs3FRW//sLeE3GpA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1732222303; x= 1732308703; bh=0A5W/6LcR7vCPf89H7iYLb4tJeGeVHTsKK5XbB8o+q0=; b=k RZvrswvAw77d+v/5uUor2epFDr7CzlhAAHx1w+EBTNflVqrQMdd6KHE0lQ4VViAc w8GPLUuKdRyZWmI8dGYBUZ1rXGuLAB7kVUdXtDdKXUbpde/1gS9GDb/0HJ59PDgI 1XMR73TOph/II08YTFZSGBV45ynjcuTfYlndUXRegz/g98eZf3bfTjaSX1+1L/MO w2+rO1jZvWEeCqpE+z2eYIQI2whxH1Q74MdukoXlZ/I1eze5NwmXu9+aVHWGA2UO HBa+dlfLnyMrwlAzWERxbhPpCky/ngccIzTwZtOVE7/nU+RfdtJMEiXJC/ZtXw6C bqytr5S+bVQG0/x9HchHg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeefuddrfeeigddufeelucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdggtfgfnhhsuhgsshgtrhhisggvpdfu rfetoffkrfgpnffqhgenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnh htshculddquddttddmnecujfgurhephffvvefufffkjghfggfgtgesthhqredttddtjeen ucfhrhhomhepvfhhohhmrghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrg hlohhnrdhnvghtqeenucggtffrrghtthgvrhhnpefftdfguddvffdvkeetgeetudejvdeu gfevudevgffhgeetkeekhefhteehtdevtdenucffohhmrghinhepughpughkrdhorhhgpd hgihhthhhusgdrtghomhenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgr ihhlfhhrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtpdhnsggprhgtphhtth hopeeffedpmhhouggvpehsmhhtphhouhhtpdhrtghpthhtoheprghnughrvghmuhgvsehl ihhnuhigrdhmihgtrhhoshhofhhtrdgtohhmpdhrtghpthhtohepmhgssehsmhgrrhhtsh hhrghrvghshihsthgvmhhsrdgtohhmpdhrtghpthhtoheprhhorhgvthiilhgrsehlihhn uhigrdhmihgtrhhoshhofhhtrdgtohhmpdhrtghpthhtohepthgvtghhsghorghrugesug hpughkrdhorhhgpdhrtghpthhtohephihuhihinhhgrdiihhgrnhhgsehinhhtvghlrdgt ohhmpdhrtghpthhtoheprghmrghnrdguvggvphdrshhinhhghhesihhnthgvlhdrtghomh dprhgtphhtthhopegrnhgrthholhihrdgsuhhrrghkohhvsehinhhtvghlrdgtohhmpdhr tghpthhtohepsghruhgtvgdrrhhitghhrghrughsohhnsehinhhtvghlrdgtohhmpdhrtg hpthhtohepsgihrhhonhdrmhgrrhhohhhnsehinhhtvghlrdgtohhm X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 21 Nov 2024 15:51:38 -0500 (EST) From: Thomas Monjalon To: Andre Muezerie Cc: Morten =?UTF-8?B?QnLDuHJ1cA==?= , roretzla@linux.microsoft.com, techboard@dpdk.org, Yuying.Zhang@intel.com, aman.deep.singh@intel.com, anatoly.burakov@intel.com, bruce.richardson@intel.com, byron.marohn@intel.com, conor.walsh@intel.com, cristian.dumitrescu@intel.com, david.hunt@intel.com, dev@dpdk.org, dsosnowski@nvidia.com, gakhil@marvell.com, jerinj@marvell.com, jingjing.wu@intel.com, kirill.rybalchenko@intel.com, konstantin.v.ananyev@yandex.ru, matan@nvidia.com, orika@nvidia.com, radu.nicolau@intel.com, ruifeng.wang@arm.com, sameh.gobriel@intel.com, sivaprasad.tummala@amd.com, skori@marvell.com, stephen@networkplumber.org, suanmingm@nvidia.com, vattunuru@marvell.com, viacheslavo@nvidia.com, vladimir.medvedkin@intel.com, yipeng1.wang@intel.com, Robin Jarry Subject: Re: [PATCH v5 01/16] eal: provide pack start macro for MSVC Date: Thu, 21 Nov 2024 21:51:36 +0100 Message-ID: <2590421.vzjCzTo3RI@thomas> In-Reply-To: <20241121193914.GA26557@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1710968771-16435-1-git-send-email-roretzla@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35E9F8D2@smartserver.smartshare.dk> <20241121193914.GA26557@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" 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 21/11/2024 20:39, Andre Muezerie: > On Tue, Nov 19, 2024 at 09:32:07AM +0100, Morten Br=C3=B8rup wrote: > > > From: Andre Muezerie [mailto:andremue@linux.microsoft.com] > > > Sent: Tuesday, 19 November 2024 05.35 > > >=20 > > > From: Tyler Retzlaff > > >=20 > > > MSVC struct packing is not compatible with GCC. Provide a macro that > > > can be used to push existing pack value and sets packing to 1-byte. > > > The existing __rte_packed macro is then used to restore the pack value > > > prior to the push. > > >=20 > > > Instead of providing macros exclusively for MSVC and for GCC the > > > existing macro is deliberately utilized to trigger a warning if no > > > existing packing has been pushed allowing easy identification of > > > locations where the __rte_msvc_pack is missing. > > >=20 > > > Signed-off-by: Tyler Retzlaff > > > --- > > > lib/eal/include/rte_common.h | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > >=20 > > > diff --git a/lib/eal/include/rte_common.h > > > b/lib/eal/include/rte_common.h > > > index 4d299f2b36..409890863e 100644 > > > --- a/lib/eal/include/rte_common.h > > > +++ b/lib/eal/include/rte_common.h > > > @@ -103,8 +103,10 @@ typedef uint16_t unaligned_uint16_t; > > > * Force a structure to be packed > > > */ > > > #ifdef RTE_TOOLCHAIN_MSVC > > > -#define __rte_packed > > > +#define __rte_msvc_pack __pragma(pack(push, 1)) > > > +#define __rte_packed __pragma(pack(pop)) > > > #else > > > +#define __rte_msvc_pack > > > #define __rte_packed __attribute__((__packed__)) > > > #endif > > >=20 > > > -- > > > 2.47.0.vfs.0.3 > >=20 > > Before proceeding with this, can we please discuss the alternative, pro= posed here: > > https://inbox.dpdk.org/dev/CAJFAV8yStgiBbe+Nkt9mC30r0+ZP64_kGuRHOzqd90R= D2HXZyw@mail.gmail.com/ > >=20 > > The definition of the packing macro in OVS, for reference: > > https://github.com/openvswitch/ovs/blob/main/include/openvswitch/compil= er.h#L209 > >=20 > > The current solution requires __rte_packed to be placed at the end of a= structure, although __attribute__((packed)) is normally allowed at the beg= inning (between the "struct" tag and the name of the structure), which intr= oduces a high risk of contributors placing it "incorrectly", thus causing e= rrors. > >=20 > > I have a strong preference for an __RTE_PACKED(decl) variant. > >=20 > > Here's a third alternative: > > #ifdef RTE_TOOLCHAIN_MSVC > > #define __rte_msvc_pack_begin __pragma(pack(push, 1)) > > #define __rte_msvc_pack_end __pragma(pack(pop)) > > #else > > #define __rte_msvc_pack_begin > > #define __rte_msvc_pack_end > > #endif > >=20 > > The third alternative is also problematic, e.g. if a contributor forget= s the _end after the structure declaration, or adds another structure decla= ration before the _end. > >=20 > > -Morten >=20 > I looked at the suggestions made and I liked the one having a __RTE_PACKE= D macro > the most. >=20 > Advantages: > - Can be placed in front of the struct, or even in the middle. Good for r= eadability. > - Does not require a different macro to be placed at the end of the struc= ture as was > proposed in V5 series. > - Works well in 99% of the cases. >=20 > Problems can arise when compiler directives are present in the struct, as= they > become arguments for __RTE_PACKED macro. This is not portable. > I've seen two situations in the DPDK code: >=20 > 1) #defines mentioned in the struct. In this situation we can just move t= he > #define out of the struct. >=20 > 2) #if/#ifdef/#elif mentioned in the struct. > This is a somewhat common pattern in structs where fields change based on > endianness. > Example: >=20 > /** > * IPv4 Header > */ > struct __rte_aligned(2) rte_ipv4_hdr { > __extension__ > union { > uint8_t version_ihl; /**< version and header length */ > struct { > #if RTE_BYTE_ORDER =3D=3D RTE_LITTLE_ENDIAN > uint8_t ihl:4; /**< header length */ > uint8_t version:4; /**< version */ > #elif RTE_BYTE_ORDER =3D=3D RTE_BIG_ENDIAN > uint8_t version:4; /**< version */ > uint8_t ihl:4; /**< header length */ > #endif > }; > }; > uint8_t type_of_service; /**< type of service */ > rte_be16_t total_length; /**< length of packet */ > ... > } __rte_packed; >=20 > One way to solve this is to move the #if to the outside. But that involves > defining the struct twice (once for each endianness). It's less than > ideal because common parts would be duplicated. I'm not sure how popular > this would be. > It's not so common though (about 1% of the structs?). I think it's an > acceptable trade-off to get portable code, but I would like to hear your > thoughts. This code would be portable if Microsoft would align with other compilers. Also I'm not sure we really need __rte_packed for most network protocols.