Re: IPv6 APIs rework

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, Jul 19, 2024 at 12:02:38PM +0200, Robin Jarry wrote:
> Morten Brørup, Jul 19, 2024 at 11:12:
> > > Vladimir Medvedkin, Jul 18, 2024 at 23:25:
> > > > 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.
> > 
> > How can they do that? The bulk lookup function takes an array of IPv6
> > addresses, not an array of pointers to IPv6 addresses.
> > 
> > What you are suggesting only works with single lookup, not bulk lookup.
> 
> Indeed for bulk lookup, you need to copy addresses on the stack.
> 
> > > Yes, my intention was exactly that, being able to map that structure >
> > directly in packets without copying them on the stack.
> > 
> > This would require changing the bulk lookup API to take an array of
> > pointers instead of an array of IPv6 addresses.
> > 
> > It would be acceptable to introduce a new single address lookup
> > function, taking a pointer to an unaligned (or 2 byte aligned) IPv6
> > address for the single lookup use cases mentioned above.
> 
> That would require two different IPv6 structures. I would prefer it we could
> avoid that. Or the unaligned lookup API needs to take a simple `const
> uint8_t *` parameter.
> 
> > I'm not worried about the IPv6 FIB functions. This proposal introduces a
> > generic IPv6 address type for *all of DPDK*, so you need to consider
> > *all* aspects, not just one library!
> > 
> > There may be current or future CPUs, where alignment makes a performance
> > difference. Do all architectures support unaligned 128 bit access at 100
> > % similar performance to aligned 128 bit access? I think not!
> > E.g. on X86 architecture, load/store across a cache boundary has a
> > performance impact. If the type is explicitly unaligned, an instance on
> > the stack (i.e. a local variable holding an IPv6 address) might cross a
> > cache boundary, whereas an 128 bit aligned instance on the stack is
> > guaranteed not to cross a cache boundary.
> > 
> > The generic IPv4 address type is natively aligned (i.e. 4 byte). When
> > accessing an IPv4 address in an IPv4 header following an Ethernet
> > header, it is not 4 byte aligned, so this is an *exception* from the
> > general case, and must be treated as such. You don't want to make the
> > general type unaligned (and thus inefficient) everywhere it is being
> > used, only because a few use cases require the unaligned form.
> 
> I think the main difference is that you almost never pass IPv4 addresses as
> reference but always as values. So alignment does not matter.
> 
> > The same principle must apply to the IPv6 address type. Let's make the
> > generic type natively aligned (16 byte). And you might also offer an
> > explicitly unaligned type for the exception use cases requiring
> > unaligned access.
> 
> The main issue with this is that you would not be able to use that type in
> the IPv6 header structure to map it to mbuf data. That leaves us with two
> options:
> 
> 1) Keep a single unaligned IPv6 type and hope for the best with
> performance. It will not be different from the current state of   things
> where every IPv6 is a uint8_t pointer.
> 
> 2) Have two IPv6 types, one 16 bytes aligned, and another one 1 byte
> aligned. The main issue with that second approach is that users may   get
> confused about which one to use and when.
> 
> I would prefer to keep it simple at first and go with option 1). We can
> always revisit that later and introduce an aligned IPv6 type for certain use
> cases.
> 
> What do you think?
>

+1 for option 1 - keep minimally aligned type. Having two types would be
confusing.

/Bruce