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 3306445D70; Thu, 21 Nov 2024 20:39:17 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EEBEA402DB; Thu, 21 Nov 2024 20:39:16 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 91D03402A7; Thu, 21 Nov 2024 20:39:15 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1213) id E1A67205A779; Thu, 21 Nov 2024 11:39:14 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com E1A67205A779 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1732217954; bh=XzjHskX4/7hO6AOR7nUzhtSN6bS8mf3TgefvC0wJ0Q4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=fIQMbEUYV/qr6xbdkKhBWTuuzhye6cJHXV5fnDhAHzJqgrm3UC/eAcWBiiCyvHDpi 27UPO28pXMUZtbWlLR4aPNn38t6h5/oFFEbxR+4EsUk2f2Uj6A7AxKhzDfIuWpZVzI ZP9DxUxuKxXRqCqdHLcopBWaV0uwFNbePcCRXGYw= Date: Thu, 21 Nov 2024 11:39:14 -0800 From: Andre Muezerie To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: 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 Subject: Re: [PATCH v5 01/16] eal: provide pack start macro for MSVC Message-ID: <20241121193914.GA26557@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1710968771-16435-1-git-send-email-roretzla@linux.microsoft.com> <1731990941-10001-1-git-send-email-andremue@linux.microsoft.com> <1731990941-10001-2-git-send-email-andremue@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35E9F8D2@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: <98CBD80474FA8B44BF855DF32C47DC35E9F8D2@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 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_kGuRHOzqd90RD2HXZyw@mail.gmail.com/ > > The definition of the packing macro in OVS, for reference: > https://github.com/openvswitch/ovs/blob/main/include/openvswitch/compiler.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. 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. Result after moving #if to the outside of the struct: #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN __RTE_PACKED( struct __rte_aligned(2) rte_ipv4_hdr { __extension__ union { uint8_t version_ihl; /**< version and header length */ struct { uint8_t ihl:4; /**< header length */ uint8_t version:4; /**< version */ }; }; uint8_t type_of_service; /**< type of service */ rte_be16_t total_length; /**< length of packet */ ... }); #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN __RTE_PACKED( struct __rte_aligned(2) rte_ipv4_hdr { __extension__ union { uint8_t version_ihl; /**< version and header length */ struct { 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 */ ... }); #endif Andre