On Thu, 18 Jul 2024 22:27:03 +0200 Morten Brørup <m...@smartsharesystems.com> wrote:
> > From: Robin Jarry [mailto:rja...@redhat.com] > > > > Hi folks, > > > > while working on IPv6 support for grout [1], I noticed that all DPDK > > IPv6 APIs used fixed sized arrays in the route lookup functions [2]. > > > > int rte_fib6_lookup_bulk(struct rte_fib6 *fib, > > uint8_t ips[][RTE_FIB6_IPV6_ADDR_SIZE], > > uint64_t *next_hops, > > int n); > > > > If I'm not mistaken, using sized arrays in function signatures is only > > for documentation purposes and does not result in any specific compiler > > checks. In the above example, the ips parameter is considered as a plain > > old `uint8_t **` pointer. > > > > Also, not having a dedicated type for IPv6 addresses requires obscure > > pointer arithmetic [3] and casting [4]. > > > > I'd like to introduce a real IPv6 address structure that has the same > > alignment than a dumb `uint8_t *` pointer but has an union to ease > > casting and most importantly presents the whole thing as an explicit > > typed structure: > > > > #define RTE_IPV6_ADDR_SIZE 16 > > > > struct rte_ipv6_addr { > > union { > > uint8_t u8[RTE_IPV6_ADDR_SIZE]; > > uint16_t u16[RTE_IPV6_ADDR_SIZE / sizeof(uint16_t)]; > > uint32_t u32[RTE_IPV6_ADDR_SIZE / sizeof(uint32_t)]; > > uint64_t u64[RTE_IPV6_ADDR_SIZE / sizeof(uint64_t)]; > > }; > > } __rte_packed __rte_aligned(1); > > > > This would require some breakage of the APIs but I think it would > > benefit code readability and maintainability in the long term. > > In short: Although I like the idea of a unified IPv6 address type very much, > I'm not sure consensus can be reached about the optimal alignment of such a > type. > > The long version: > > Please consider this proposal in a broader perspective. > > The IPv4 FIB lookup takes an uint32_t array, so the IPv4 address type here is > 4 byte aligned: uint32_t *ips > Generally, uint32_t or rte_be32_t is used for IPv4 addresses, and both these > types are 4 byte aligned. In other words: IPv4 addresses are considered 4 > byte aligned by DPDK. > > I don't think it is similarly simple for IPv6 addresses. > > The alignment of IPv6 addresses may depend on how/where they are used, e.g.: > 1. For the FIB library, it might be good for vector implementations to have > the IPv6 addresses naturally aligned (i.e. 16 byte aligned), like the > uint128_t/__int128/__m128i type (or the rte_xmm_t type [XMM]). Furthermore, a > simple integer type (uint128_t equivalent) might be preferable in this API. > 2. In the IPv6 packet header, the IPv6 addresses are not 16 byte aligned, > they are 8 byte aligned. So we cannot make the IPv6 address type 16 byte > aligned. > > I fear that broadly dumbing down the IPv6 address type to always use 1 byte > alignment could potentially introduce unwanted performance penalties (now or > in the future). We didn't do it for IPv4 addresses, so let's not do it for > IPv6 addresses. > > Perhaps we could use the lowest "non-exotic" (considering the use of IPv6 > addresses) alignment, which I would guess is 8 byte - as in the IPv6 packet > header. > For reference, Ethernet addresses are defined as 2 byte aligned [ETH]. > > [XMM]: > https://elixir.bootlin.com/dpdk/v24.03/source/lib/eal/x86/include/rte_vect.h#L42 > [ETH]: > https://elixir.bootlin.com/dpdk/v24.07-rc2/source/lib/net/rte_ether.h#L74 > > > > > int rte_fib6_lookup_bulk(struct rte_fib6 *fib, > > const struct rte_ipv6_addr *ips, > > uint64_t *next_hops, > > int n); > > > > I already have a semi-working draft and am in the process of splitting > > the changes into small chunks to make them easier to review. > > > > https://github.com/DPDK/dpdk/compare/main...rjarry:dpdk:ipv6-address- > > rework > > > > Is that something that would be of interest? If yes, I would like to > > announce API breakage before the release of 24.07 so that the changes > > can be integrated into 24.11. > > > > Cheers! > > > > [1] https://github.com/rjarry/grout > > [2] > > https://doc.dpdk.org/api/rte__fib6_8h.html#a924678410ccb9551cda3e75d742a > > 11e3 > > [3] https://git.dpdk.org/dpdk/tree/lib/fib/trie_avx512.c?h=v24.07- > > rc2#n340 > > [4] https://git.dpdk.org/dpdk/tree/lib/hash/rte_thash.h?h=v24.07- > > rc2#n156 > > > > -- > > Robin > If you look at the standard netinet/in.h the storage of IPv6 addresses is in in6_addr. DPDK has always wanted to do its own thing... The in6_addr is a union with no explicit alignment. struct in6_addr { union { uint8_t __u6_addr8[16]; uint16_t __u6_addr16[8]; uint32_t __u6_addr32[4]; } __in6_u; Better to not have explicit alignment and not have 64 bit value.
