<div dir="ltr"><div dir="ltr">Hi Robin,<div><br></div><div>Thanks, that is a good idea.</div><div><br></div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">чт, 18 июл. 2024 г. в 21:27, Morten Brørup <<a href="mailto:mb@smartsharesystems.com">mb@smartsharesystems.com</a>>:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">> From: Robin Jarry [mailto:<a href="mailto:rjarry@redhat.com" target="_blank">rjarry@redhat.com</a>]<br> > <br> > Hi folks,<br> > <br> > while working on IPv6 support for grout [1], I noticed that all DPDK<br> > IPv6 APIs used fixed sized arrays in the route lookup functions [2].<br> > <br> > int rte_fib6_lookup_bulk(struct rte_fib6 *fib,<br> > uint8_t ips[][RTE_FIB6_IPV6_ADDR_SIZE],<br> > uint64_t *next_hops,<br> > int n);<br> > <br> > If I'm not mistaken, using sized arrays in function signatures is only<br> > for documentation purposes and does not result in any specific compiler<br> > checks. In the above example, the ips parameter is considered as a plain<br> > old `uint8_t **` pointer.<br> > <br> > Also, not having a dedicated type for IPv6 addresses requires obscure<br> > pointer arithmetic [3] and casting [4].<br> > <br> > I'd like to introduce a real IPv6 address structure that has the same<br> > alignment than a dumb `uint8_t *` pointer but has an union to ease<br> > casting and most importantly presents the whole thing as an explicit<br> > typed structure:<br> > <br> > #define RTE_IPV6_ADDR_SIZE 16<br> > <br> > struct rte_ipv6_addr {<br> > union {<br> > uint8_t u8[RTE_IPV6_ADDR_SIZE];<br> > uint16_t u16[RTE_IPV6_ADDR_SIZE / sizeof(uint16_t)];<br> > uint32_t u32[RTE_IPV6_ADDR_SIZE / sizeof(uint32_t)];<br> > uint64_t u64[RTE_IPV6_ADDR_SIZE / sizeof(uint64_t)];<br> > };<br> > } __rte_packed __rte_aligned(1);<br> > <br> > This would require some breakage of the APIs but I think it would<br> > benefit code readability and maintainability in the long term.<br> <br> 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.<br> <br> The long version:<br> <br> Please consider this proposal in a broader perspective.<br> <br> The IPv4 FIB lookup takes an uint32_t array, so the IPv4 address type here is 4 byte aligned: uint32_t *ips<br> 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.<br> <br> I don't think it is similarly simple for IPv6 addresses.<br> <br> The alignment of IPv6 addresses may depend on how/where they are used, e.g.:<br> 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.<br></blockquote><div><div><br></div><div>I think alignment should be 1 since in FIB6 users usually don't copy IPv6 address and just provide a pointer to the memory inside the packet. Current vector implementation loads IPv6 addresses using unaligned access (<span style="color:rgb(0,0,0);font-family:"Helvetica Neue";font-size:14px">_mm512_loadu_si512</span>) so it doesn't rely on alignment.</div><div></div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"> 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.<br></blockquote><div>Not necessary, if Ethernet frame in mbuf starts on 8b aligned address, then IPv6 is aligned only by 2 bytes.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"> <br> 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.<br> <br> 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.<br> For reference, Ethernet addresses are defined as 2 byte aligned [ETH].<br> <br> [XMM]: <a href="https://elixir.bootlin.com/dpdk/v24.03/source/lib/eal/x86/include/rte_vect.h#L42" rel="noreferrer" target="_blank">https://elixir.bootlin.com/dpdk/v24.03/source/lib/eal/x86/include/rte_vect.h#L42</a><br> [ETH]: <a href="https://elixir.bootlin.com/dpdk/v24.07-rc2/source/lib/net/rte_ether.h#L74" rel="noreferrer" target="_blank">https://elixir.bootlin.com/dpdk/v24.07-rc2/source/lib/net/rte_ether.h#L74</a><br> <br> > <br> > int rte_fib6_lookup_bulk(struct rte_fib6 *fib,<br> > const struct rte_ipv6_addr *ips,<br> > uint64_t *next_hops,<br> > int n);<br> > <br> > I already have a semi-working draft and am in the process of splitting<br> > the changes into small chunks to make them easier to review.<br> > <br> > <a href="https://github.com/DPDK/dpdk/compare/main...rjarry:dpdk:ipv6-address-" rel="noreferrer" target="_blank">https://github.com/DPDK/dpdk/compare/main...rjarry:dpdk:ipv6-address-</a><br> > rework<br> > <br> > Is that something that would be of interest? If yes, I would like to<br> > announce API breakage before the release of 24.07 so that the changes<br> > can be integrated into 24.11.<br> > <br> > Cheers!<br> > <br> > [1] <a href="https://github.com/rjarry/grout" rel="noreferrer" target="_blank">https://github.com/rjarry/grout</a><br> > [2]<br> > <a href="https://doc.dpdk.org/api/rte__fib6_8h.html#a924678410ccb9551cda3e75d742a" rel="noreferrer" target="_blank">https://doc.dpdk.org/api/rte__fib6_8h.html#a924678410ccb9551cda3e75d742a</a><br> > 11e3<br> > [3] <a href="https://git.dpdk.org/dpdk/tree/lib/fib/trie_avx512.c?h=v24.07-" rel="noreferrer" target="_blank">https://git.dpdk.org/dpdk/tree/lib/fib/trie_avx512.c?h=v24.07-</a><br> > rc2#n340<br> > [4] <a href="https://git.dpdk.org/dpdk/tree/lib/hash/rte_thash.h?h=v24.07-" rel="noreferrer" target="_blank">https://git.dpdk.org/dpdk/tree/lib/hash/rte_thash.h?h=v24.07-</a><br> > rc2#n156<br> > <br> > --<br> > Robin<br> <br> </blockquote></div><br clear="all"><div><br></div><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div>Vladimir<br></div></div></div>