RE: [PATCH v4] net: fix checksum with unaligned buffer

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

> From: Emil Berg [mailto:emil.berg@ericsson.com]
> Sent: Friday, 1 July 2022 06.11
> 
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: den 30 juni 2022 19:46
> >
> > On Thu, 23 Jun 2022 14:39:00 +0200
> > Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > > +	/* if buffer is unaligned, keeping it byte order independent */
> > > +	if (unlikely(unaligned)) {
> > > +		uint16_t first = 0;
> > > +		if (unlikely(len == 0))
> > > +			return 0;
> >
> > Why is length == 0 unique to unaligned case?

Because the aligned case handles it gracefully. The unaligned case subtracts one from 'len', which (being unsigned) would become a very large number, causing 'end' to become way off.

> >
> > > +		((unsigned char *)&first)[1] = *(const unsigned
> > char *)buf;
> >
> > Use a proper union instead of casting to avoid aliasing warnings.

I copied what is done by 'left', so it resembles the existing code in the function, making it easier to review.

It is part of the endian neutral checksum handling. Tricky stuff! :-)

> >
> > > +		bsum += first;
> > > +		buf = RTE_PTR_ADD(buf, 1);
> > > +		len--;
> > > +	}
> >
> > Many CPU's (such as x86) won't care about alignment and therefore the
> > extra code to handle this is not worth doing.
> >
> 
> x86 does care about alignment. An example is the vmovdqa instruction,
> where 'a' stands for 'aligned'. The description in the link below says:
> "When the source or destination operand is a memory operand, the
> operand must be aligned on a 16-byte boundary or a general-protection
> exception (#GP) will be generated. "
> 
> https://www.felixcloutier.com/x86/movdqa:vmovdqa32:vmovdqa64
> 

Also, this misconception is exactly the bug [1] this patch fixes.

[1] https://bugs.dpdk.org/show_bug.cgi?id=1035

> > Perhaps DPDK needs a macro (like Linux kernel) for efficient
> unaligned
> > access.
> >
> > In Linux kernel it is CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS

I recently stumbled across RTE_ARCH_STRICT_ALIGN in /lib/eal/include/rte_common.h.

But I guess it is something else.

Anyway, this function has ugly alignment problems (also before the patch), and has gone through a couple of iterations to silence warnings from the compiler. These warnings should have been addressed instead of silenced. Mattias has suggested a far better solution [2] than mine, which also correctly addresses the compiler alignment warnings, so we will probably end up with his solution instead.

[2] http://inbox.dpdk.org/dev/AM8PR07MB7666AD7BF7B780CC5062C14598BD9@AM8PR07MB7666.eurprd07.prod.outlook.com/T/#m1a76490541fce4a85b12d9390f2f4fac5a9f4660