> From: Robin Jarry [mailto:rja...@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'd say that it is better to keep it simple :) Yes, I don't see the CPU folks screaming about performance for this patch, so let's remain ignorant about the reasons for packing and make the safe choice: Keep the IPv6 addr type as you suggested, and do not make it 2-byte aligned by adding the unnamed union with the uint16_t field. Also, I think we are too close to the 24.11 deadline to go deep enough into this to make qualified decisions about changing it. For educational purposes, sometime in the future, I would like to learn about the choices of packing and 2-byte alignment questioned above. [ipv4h] https://elixir.bootlin.com/dpdk/v24.07/source/lib/net/rte_ip.h#L41 [ipv6h]: https://elixir.bootlin.com/dpdk/v24.07/source/lib/net/rte_ip.h#L526 [etha]: https://elixir.bootlin.com/dpdk/v24.07/source/lib/net/rte_ether.h#L74 [ethh]: https://elixir.bootlin.com/dpdk/v24.07/source/lib/net/rte_ether.h#L293 [vlanh]: https://elixir.bootlin.com/dpdk/v24.07/source/lib/net/rte_ether.h#L304
