Message ID | 1710968771-16435-1-git-send-email-roretzla@linux.microsoft.com (mailing list archive) |
---|---|
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6440D43D0A; Wed, 20 Mar 2024 22:06:20 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DF8F242D80; Wed, 20 Mar 2024 22:06:15 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 98A924026C for <dev@dpdk.org>; Wed, 20 Mar 2024 22:06:13 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id DB7FA20B74C0; Wed, 20 Mar 2024 14:06:12 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com DB7FA20B74C0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1710968772; bh=zgsRTMFvwnxzSyCmlSW36MqVkbffbNrzZyWXSyqNgLA=; h=From:To:Cc:Subject:Date:From; b=E0a7b0OTVTIOu+jRXlkFAblUkicmLWm2TJJKpCQ99X+JB8rwBowlaa+O35Bam3wue EN1jiku83V1Jgcpo/46vadOp2KYPyYoVmN17sOpXcMILQABiPgNbBeGXuYTKDHiZPY 8eTGQkMldxNUeLWr02NW/T4YwCbRa3/6YoHVd4f0= From: Tyler Retzlaff <roretzla@linux.microsoft.com> To: dev@dpdk.org Cc: Akhil Goyal <gakhil@marvell.com>, Aman Singh <aman.deep.singh@intel.com>, Anatoly Burakov <anatoly.burakov@intel.com>, Bruce Richardson <bruce.richardson@intel.com>, Byron Marohn <byron.marohn@intel.com>, Conor Walsh <conor.walsh@intel.com>, Cristian Dumitrescu <cristian.dumitrescu@intel.com>, Dariusz Sosnowski <dsosnowski@nvidia.com>, David Hunt <david.hunt@intel.com>, Jerin Jacob <jerinj@marvell.com>, Jingjing Wu <jingjing.wu@intel.com>, Kirill Rybalchenko <kirill.rybalchenko@intel.com>, Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>, Matan Azrad <matan@nvidia.com>, Ori Kam <orika@nvidia.com>, Radu Nicolau <radu.nicolau@intel.com>, Ruifeng Wang <ruifeng.wang@arm.com>, Sameh Gobriel <sameh.gobriel@intel.com>, Sivaprasad Tummala <sivaprasad.tummala@amd.com>, Suanming Mou <suanmingm@nvidia.com>, Sunil Kumar Kori <skori@marvell.com>, Vamsi Attunuru <vattunuru@marvell.com>, Viacheslav Ovsiienko <viacheslavo@nvidia.com>, Vladimir Medvedkin <vladimir.medvedkin@intel.com>, Yipeng Wang <yipeng1.wang@intel.com>, Yuying Zhang <Yuying.Zhang@intel.com>, Tyler Retzlaff <roretzla@linux.microsoft.com> Subject: [PATCH 00/15] fix packing of structs when building with MSVC Date: Wed, 20 Mar 2024 14:05:56 -0700 Message-Id: <1710968771-16435-1-git-send-email-roretzla@linux.microsoft.com> X-Mailer: git-send-email 1.8.3.1 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org |
Series |
fix packing of structs when building with MSVC
|
|
Message
Tyler Retzlaff
March 20, 2024, 9:05 p.m. UTC
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. I've decided to only add the macro to packed structs that are built for Windows. It seems there is little value in adding the macro tree wide and if new code arrives that is built on Windows the __rte_packed will flag where the preamble macro is required. Tyler Retzlaff (15): eal: provide pack start macro for MSVC eal: pack structures when building with MSVC net: pack structures when building with MSVC common/iavf: pack structures when building with MSVC common/idpf: pack structures when building with MSVC common/mlx5: pack structures when building with MSVC dma/ioat: pack structures when building with MSVC net/i40e: pack structures when building with MSVC net/iavf: pack structures when building with MSVC net/ice: pack structures when building with MSVC net/mlx5: pack structures when building with MSVC net/octeon_ep: pack structures when building with MSVC app/testpmd: pack structures when building with MSVC app/test: pack structures when building with MSVC examples: pack structures when building with MSVC app/test-pmd/csumonly.c | 1 + app/test/test_efd.c | 1 + app/test/test_hash.c | 1 + app/test/test_member.c | 1 + drivers/common/iavf/iavf_osdep.h | 2 ++ drivers/common/iavf/virtchnl_inline_ipsec.h | 11 +++++++++++ drivers/common/idpf/base/idpf_osdep.h | 2 ++ drivers/common/mlx5/mlx5_common_mr.h | 4 ++++ drivers/common/mlx5/mlx5_common_utils.h | 1 + drivers/common/mlx5/mlx5_prm.h | 28 ++++++++++++++++++++++++++++ drivers/dma/ioat/ioat_hw_defs.h | 1 + drivers/net/i40e/base/i40e_osdep.h | 2 ++ drivers/net/iavf/iavf_ipsec_crypto.h | 3 +++ drivers/net/iavf/iavf_rxtx.c | 1 + drivers/net/ice/base/ice_osdep.h | 2 ++ drivers/net/mlx5/hws/mlx5dr.h | 1 + drivers/net/mlx5/mlx5.h | 1 + drivers/net/mlx5/mlx5_flow.h | 5 +++++ drivers/net/mlx5/mlx5_hws_cnt.h | 1 + drivers/net/mlx5/mlx5_utils.h | 4 ++++ drivers/net/octeon_ep/otx_ep_mbox.h | 1 + examples/common/neon/port_group.h | 1 + examples/ip_pipeline/cli.c | 5 +++++ examples/ipsec-secgw/ipsec.h | 1 + examples/l3fwd-power/main.c | 2 ++ examples/ptpclient/ptpclient.c | 8 ++++++++ lib/eal/common/eal_private.h | 1 + lib/eal/include/rte_common.h | 4 +++- lib/eal/include/rte_memory.h | 1 + lib/eal/include/rte_memzone.h | 1 + lib/eal/include/rte_trace_point.h | 1 + lib/eal/x86/include/rte_memcpy.h | 3 +++ lib/net/rte_arp.h | 2 ++ lib/net/rte_dtls.h | 1 + lib/net/rte_esp.h | 2 ++ lib/net/rte_ether.h | 1 + lib/net/rte_geneve.h | 1 + lib/net/rte_gre.h | 4 ++++ lib/net/rte_gtp.h | 5 +++++ lib/net/rte_ib.h | 1 + lib/net/rte_icmp.h | 3 +++ lib/net/rte_ip.h | 4 ++++ lib/net/rte_l2tpv2.h | 4 ++++ lib/net/rte_macsec.h | 2 ++ lib/net/rte_mpls.h | 1 + lib/net/rte_pdcp_hdr.h | 4 ++++ lib/net/rte_ppp.h | 1 + lib/net/rte_sctp.h | 1 + lib/net/rte_tcp.h | 1 + lib/net/rte_tls.h | 1 + lib/net/rte_udp.h | 1 + lib/net/rte_vxlan.h | 2 ++ 52 files changed, 143 insertions(+), 1 deletion(-)
Comments
On Wed, 20 Mar 2024 14:05:56 -0700 Tyler Retzlaff <roretzla@linux.microsoft.com> wrote: > 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. > > I've decided to only add the macro to packed structs that are built > for Windows. It seems there is little value in adding the macro tree > wide and if new code arrives that is built on Windows the __rte_packed > will flag where the preamble macro is required. > Why is packed used so many places in DPDK? Packed should be reserved for things like network protocols with unaligned fields. Many of these structure have no holes and are naturally packed. Adding packed attribute unnecessarily can generate poor code on architectures that do not support unaligned access.
On Thu, Mar 21, 2024 at 08:32:19AM -0700, Stephen Hemminger wrote: > On Wed, 20 Mar 2024 14:05:56 -0700 > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote: > > > 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. > > > > I've decided to only add the macro to packed structs that are built > > for Windows. It seems there is little value in adding the macro tree > > wide and if new code arrives that is built on Windows the __rte_packed > > will flag where the preamble macro is required. > > > > Why is packed used so many places in DPDK? > Packed should be reserved for things like network protocols > with unaligned fields. I made the same observation. Even network frame formats can be handled without packing so long as you do some extra work to copy small units of data out which is often worth doing manually and once only instead of obfuscated behind codegen and direct field access. > Many of these structure have no holes and are naturally packed. > Adding packed attribute unnecessarily can generate poor code on > architectures that do not support unaligned access. For the specific case where their fields and alignments are naturally packed I'm not entirely certain you get different codegen but I agree it might and we'd like to avoid it if it does. In a lot of the cases it seems to be drivers and often appears like it is either structs that are used to hardware registers or firmware protocol data units. Both of which may be need packing. I will commit to removing packing in structs in this series iff individual maintainers identify the specific structs in reply to the series. If there is no request I'll leave it to them to figure out. Maintainers should review and request removal as appropriate. Thanks!