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

["Morten Brørup" <mb@smartsharesystems.com>]

> 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'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