Re: [PATCH dpdk v2 03/16] net: add structure for ipv6 addresses

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 11 Oct 2024 14:37:35 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > From: Robin Jarry [mailto:rjarry@redhat.com]
> > Sent: Thursday, 10 October 2024 22.08
> > 
> > Morten Brørup, Oct 06, 2024 at 10:18:  
> > > This has been discussed before, but I want to double check...
> > >
> > > If - sometime in the future - we want to add a union to offer a 2-  
> > byte  
> > > access variant and make the structure to 2-byte aligned (which is the
> > > common case in Ethernet packets), it will break both API and ABI.  
> > This  
> > > seems unlikely to get accepted at a later time, so I think we are
> > > - right now - in a situation where it's now or never:
> > >
> > > struct rte_ipv6_addr {
> > > 	__extension__
> > > 	union {
> > > 		unsigned char a[RTE_IPV6_ADDR_SIZE];
> > > 		uint16_t      w[RTE_IPV6_ADDR_SIZE / 2];
> > > 	};
> > > };
> > >
> > > Unless some of the CPU folks want the 2-byte aligned variant, stick
> > > with what you offered.  
> > 
> > I was also thinking the same. I could have added an unnamed union which
> > only contains one field.
> > 
> > However, it does not make much sense if we never want to change the
> > default alignment.
> > 
> > Important note: DPDK is compiled with the following C flags:
> > 
> >   -Wno-address-of-packed-member
> > 
> > Added in 2017 https://git.dpdk.org/dpdk/commit/?id=95cd37070af44
> > 
> > If we had struct rte_ipv6_addr aligned on 2 bytes (or more),
> > applications that do not silence that warning would have a hard time.
> > Example in grout:
> > 
> > ../modules/ip6/datapath/ip6_input.c:99:54: error: taking address of
> > packed member of ‘struct rte_ipv6_hdr’ may result in an unaligned
> > pointer value [-Werror=address-of-packed-member]
> >    99 |                 nh = ip6_route_lookup(iface->vrf_id, &ip-  
> > >dst_addr);  
> >       |
> > ^~~~~~~~~~~~~  
> 
> I don't understand the choice of packing for these types...
> 
> Why are the IPv4 [ipv4h] and IPv6 header [ipv6h] types packed? Is it so they can be stored on unaligned addresses?
> 
> E.g. the IPv4 header's natural alignment is 4 byte (due to the rte_be32_t src_addr, dst_addr fields). The IPv4 header is most often 2 byte aligned, being located after an Ethernet header.
> 
> Maybe they should be 2-byte aligned, like the Ethernet address type [etha] and the Ethernet header type [ethh].
> 
> The VLAN tag type [vlanh] is packed. Why?
> 
> I wonder if the compiler would spit out the same warnings if the above types were __rte_aligned(2) instead of __rte_packed.

I am in the never use packed camp for network headers. The Linux, BSD and glibc headers avoid use of packed. Windows seems to love using it.