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 3421D45DA6; Mon, 25 Nov 2024 23:15:41 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F20E14025F; Mon, 25 Nov 2024 23:15:40 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id A36B2400EF; Mon, 25 Nov 2024 23:15:39 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1213) id A3C99205459A; Mon, 25 Nov 2024 14:15:38 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com A3C99205459A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1732572938; bh=SGyM9SU3EgKtIYR0P+SVeLnTL0lNduHzFnJzn940rWw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=W2/zL4+/uxQlkG597lFLdwB7/LmsVY5u4JwN79adXE2jJroTY/p+UukpfFfSKK5ug rNsu1wCpJi7RPbO87N3VOu0TVBXS40NxU0EYpmj90rt4Bu8GoGO32hgxc/dkBR6TqA kW+0JOl3qNRzbOyTuF5xRX7Mt0jRpP5wSBZ+YfwE= Date: Mon, 25 Nov 2024 14:15:38 -0800 From: Andre Muezerie To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: Thomas Monjalon , 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 Message-ID: <20241125221538.GA25446@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> <2590421.vzjCzTo3RI@thomas> <20241122001135.GA28622@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35E9F8E3@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F8E3@smartserver.smartshare.dk> User-Agent: Mutt/1.5.21 (2010-09-15) 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 On Fri, Nov 22, 2024 at 09:13:38AM +0100, Morten Brørup wrote: > > From: Andre Muezerie [mailto:andremue@linux.microsoft.com] > > Sent: Friday, 22 November 2024 01.12 > > > > On Thu, Nov 21, 2024 at 09:51:36PM +0100, Thomas Monjalon wrote: > > > 21/11/2024 20:39, Andre Muezerie: > > > > On Tue, Nov 19, 2024 at 09:32:07AM +0100, Morten Brørup wrote: > > > > > > From: Andre Muezerie [mailto:andremue@linux.microsoft.com] > > > > > > Sent: Tuesday, 19 November 2024 05.35 > > > > > > > > > > > > From: Tyler Retzlaff > > > > > > > > > > > > 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. > > > > > > > > > > > > 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. > > > > > > > > > > > > Signed-off-by: Tyler Retzlaff > > > > > > --- > > > > > > lib/eal/include/rte_common.h | 4 +++- > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > > > 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 > > > > > > > > > > > > -- > > > > > > 2.47.0.vfs.0.3 > > > > > > > > > > Before proceeding with this, can we please discuss the > > alternative, proposed here: > > > > > > > https://inbox.dpdk.org/dev/CAJFAV8yStgiBbe+Nkt9mC30r0+ZP64_kGuRHOzqd90R > > D2HXZyw@mail.gmail.com/ > > > > > > > > > > The definition of the packing macro in OVS, for reference: > > > > > > > https://github.com/openvswitch/ovs/blob/main/include/openvswitch/compil > > er.h#L209 > > > > > > > > > > The current solution requires __rte_packed to be placed at the > > end of a structure, although __attribute__((packed)) is normally > > allowed at the beginning (between the "struct" tag and the name of the > > structure), which introduces a high risk of contributors placing it > > "incorrectly", thus causing errors. > > > > > > > > > > I have a strong preference for an __RTE_PACKED(decl) variant. > > > > > > > > > > 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 > > > > > > > > > > The third alternative is also problematic, e.g. if a contributor > > forgets the _end after the structure declaration, or adds another > > structure declaration before the _end. > > > > > > > > > > -Morten > > > > > > > > I looked at the suggestions made and I liked the one having a > > __RTE_PACKED macro > > > > the most. > > > > > > > > Advantages: > > > > - Can be placed in front of the struct, or even in the middle. Good > > for readability. > > > > - Does not require a different macro to be placed at the end of the > > structure as was > > > > proposed in V5 series. > > > > - Works well in 99% of the cases. > > > > > > > > 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: > > > > > > > > 1) #defines mentioned in the struct. In this situation we can just > > move the > > > > #define out of the struct. > > No problem. > > > > > > > > > 2) #if/#ifdef/#elif mentioned in the struct. > > > > This is a somewhat common pattern in structs where fields change > > based on > > > > endianness. > > > > Example: > > > > > > > > /** > > > > * IPv4 Header > > > > */ > > > > struct __rte_aligned(2) rte_ipv4_hdr { > > > > __extension__ > > > > union { > > > > uint8_t version_ihl; /**< version and header length */ > > > > struct { > > > > #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN > > > > uint8_t ihl:4; /**< header length */ > > > > uint8_t version:4; /**< version */ > > > > #elif RTE_BYTE_ORDER == 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; > > > > > > > > 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. > > Good catch. > Although it is relatively rare, it is a standard design pattern, also in other projects than DPDK. We should either support it, or emit an error on occurrence when compiling. > > > > > > > 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. > > Many network protocol structures contain uint32_t/rte_be32_t fields, which makes the structures 4-byte aligned; but the preceding 14 byte Ethernet header makes them non-4-byte aligned. So their alignment needs to be reduced from 4-byte to 2-byte. > > > > > > > > Indeed, it's is a shame that the way MSVC works differs so much, but > > unfortunately that won't change. > > > > Considering the implications of the __RTE_PACK macro approach on these > > special cases, perhaps the initial proposal made in the series that was > > submitted for review is better after all? At least it does not have > > these "special" cases, so the approach required on the code would be > > uniform in the entire code base. > > Agree. > If we don't like "msvc" in the macro name, we could rename __rte_msvc_pack to __rte_packed_begin. > We could also rename __rte_packed to __rte_packed_end, emphasizing that __rte_packed is no longer allowed at the beginning of structures. > And for backwards compatibility add #define __rte_packed __rte_packed_end. > > Structures would then look like this: > > struct __rte_packed_begin __rte_aligned(2) rte_proto_hdr { > rte_be32_t proto_field; > ... > } __rte_packed_end; > > If you prefer keeping __rte_packed instead of renaming it to __rte_packed_end, it would probably be easy for checkpatch to verify its location in the structure declaration, as it must always be directly preceded by whitespace and directly followed by a semicolon. > (The same goes for __rte_packed_end.) > > Checkpatch could also verify that each __rte_packed_end/__rte_packed is preceded by __rte_packed_begin, for MSVC compatibility. Makes sense. I'll update the series accordingly.