RE: IPv6 APIs rework

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

> From: Robin Jarry [mailto:rjarry@redhat.com]
> Sent: Sunday, 21 July 2024 23.51
> 
> Hi Morten, Stephen,
> 
> Morten Brørup, Jul 21, 2024 at 18:12:
> > If the IPv6 address type you tested with was a struct containing
> > a union of different types (other than an array of 16 bytes), then
> > those sub-types made your IPv6 address type non-byte aligned, and
> > caused padding when used in other structures.
> >
> > Please try again with the simple array type:
> > struct rte_ipv6_addr { unsigned char addr_bytes[16]; };
> >
> > This should not cause any padding when used in other structures,
> > except if used with alignas().
> 
> Indeed removing the sub-types in the union removes the need for strict
> alignment and packing.
> 
> Too bad, I found these intermediate integers made the code a bit nicer
> but I can understand that it brings a lot of trouble down the line.

Maybe some magical macros (or inline functions) can be used for pretty casting to larger integer types, using alignof() and/or the GCC assume_aligned attribute.
Such macros/functions can be added in later patches.
Perhaps they might even be generic, so they could be used on other byte array types too.

> 
> NB: I tried uint8_t vs unsigned char, it makes no difference with
> implicit casting to (uint16_t *) or (uint32_t *). Explicit casting is
> required anyway.

Unfortunately, I still cannot recall why unsigned char is better for type casting than uint8_t, so I cannot support my statement with a trustworthy source of reference.

> 
> > If you are introducing an official IPv6 address type into DPDK, its
> > scope it not just the FIB6 API.
> >
> > Both Stephen and I can see that - in a broader perspective - the
> > packed and unaligned constraints are unacceptable for performance.
> >
> > It might not be a problem for the current FIB6 implementation, but it
> > *will* be a problem in many other places, if converted to using the
> > new IPv6 address type.
> >
> > PS:
> > I do consider adding a dedicated IPv6 address type to DPDK an
> > improvement over the current convention of using an uint8_t[16] array.
> > But we need to agree on the type, which must work optimally for
> > a broad spectrum of use cases. Otherwise, the new type is not an
> > improvement, but a deterioration of DPDK.
> 
> OK, I understand the stakes. I will comply and propose a simple struct
> without any packing nor explicit alignment.
> 
>     struct rte_ipv6_addr {
>         union {
>             unsigned char a[RTE_IPV6_ADDR_SIZE];
>         };
>     };
> 
> I have left the door open in order to ease adding sub-types in the
> future. Indeed, lpm6/fib6 tests rely on literal definitions of IPv6
> addresses and union types need an extra set of curly braces for literal
> definitions. If you think we will never need to add sub-types, I can get
> rid of this. It makes no difference at runtime.

I think it is safe to start without the union.
If the anonymous union only has one member, it makes no difference if the union is there or not.
So, if we add other sub-types in the future, the union can be added at that time.

NB: I used "addr_bytes" as the name of the array in the structure, as in the rte_ether_addr structure [1]; but I support using "a" instead, it is shorter and it seems obvious that it is the same.

[1]: https://elixir.bootlin.com/dpdk/v24.07-rc2/source/lib/net/rte_ether.h#L74

<brainstorming>
Perhaps we could add an anonymous union to rte_ether_addr, to shorten its access name similarly:

struct __rte_aligned(2) rte_ether_addr {
+   __extension__
+   union {
        uint8_t addr_bytes[RTE_ETHER_ADDR_LEN]; /**< Addr bytes in tx order */
+       unsigned char a[RTE_ETHER_ADDR_LEN]; /**< Same, but shorter name */
+   }
};

This is not related to your patch in any way. Just thinking out loud.
</brainstorming>

> 
> About the timing: when should I send a patch to announce IPv6 API
> breakage for 24.11?

ASAP, I guess.
I suggest you describe it as an introduction of an IPv6 address type, and list the APIs that will be updated to use this new type.
The intention of introducing the new IPv6 address type with a broader scope than just the FIB6 APIs is to inspire others to use the new IPv6 address type too.

> 
> Thanks for taking the time.
> Cheers.

Thank you for listening.