[00/15] fix packing of structs when building with MSVC

  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(-)
  

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!