Message-ID: <3e0e2b25ea2d4ab99e78aff04af94b69@AcuMS.aculab.com>
Date: Fri, 1 Mar 2024 17:24:39 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Charlie Jenkins' <charlie@...osinc.com>, Christophe Leroy
	<christophe.leroy@...roup.eu>
CC: Guenter Roeck <linux@...ck-us.net>, Palmer Dabbelt <palmer@...belt.com>,
	Andrew Morton <akpm@...ux-foundation.org>, Helge Deller <deller@....de>,
	"James E.J. Bottomley" <James.Bottomley@...senpartnership.com>, Parisc List
	<linux-parisc@...r.kernel.org>, Arnd Bergmann <arnd@...db.de>, "Geert
 Uytterhoeven" <geert@...ux-m68k.org>, Russell King <linux@...linux.org.uk>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Palmer Dabbelt
	<palmer@...osinc.com>, Linux ARM <linux-arm-kernel@...ts.infradead.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH v11] lib: checksum: Use aligned accesses for ip_fast_csum
 and csum_ipv6_magic tests

From: Charlie Jenkins
> Sent: 01 March 2024 17:09
> 
> On Fri, Mar 01, 2024 at 07:17:38AM +0000, Christophe Leroy wrote:
> > +CC netdev ARM Russell
> >
> > Le 29/02/2024 à 23:46, Charlie Jenkins a écrit :
> > > The test cases for ip_fast_csum and csum_ipv6_magic were not properly
> > > aligning the IP header, which were causing failures on architectures
> > > that do not support misaligned accesses like some ARM platforms. To
> > > solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the
> > > standard alignment of an IP header and must be supported by the
> > > architecture.
> >
> > In your description, please provide more details on platforms that have
> > a problem, what the problem is exactly (Failed calculation, slowliness,
> > kernel Oops, panic, ....) on each platform.
> >
> > And please copy maintainers and lists of platforms your are specifically
> > addressing with this change. And as this is network related, netdev list
> > should have been copied as well.
> >
> > I still think that your patch is not the good approach, it looks like
> > you are ignoring all the discussion. Below is a quote of what Geert said
> > and I fully agree with that:
> >
> > 	IMHO the tests should validate the expected functionality.  If a test
> > 	fails, either functionality is missing or behaves wrong, or the test
> > 	is wrong.
> >
> > 	What is the point of writing tests for a core functionality like network
> > 	checksumming that do not match the expected functionality?
> >
> >
> > So we all agree that there is something to fix, because today's test
> > does odd-address accesses which is unexpected for those functions, but
> > 2-byte alignments should be supported hence tested by the test. Limiting
> > the test to a 16-bytes alignment deeply reduces the usefullness of the test.
> >
> 
> Maybe I am lost in the conversations. This isn't limited to 16-bytes
> alignment? It aligns along 14 + NET_IP_ALIGN. That is 16 on some
> platforms and 14 on platforms where unaligned accesses are desired.
> These functions are expected to be called with this offset. Testing with
> any other alignment is not the expected behavior. These tests are
> testing the expected functionality.

Aligned received frames can have a 4 byte VLAN header (or two) removed.
So the alignment of the IP header is either 4n or 4n+2.
If the cpu fault misaligned accesses you really want the alignment
to be 4n.

You pretty much never want to trap and fixup a misaligned access.
Especially in the network stack.
I suspect it is better to do a realignment copy of the entire frame.
At some point the data will be copied again, although you may want
a CBU (crystal ball unit) to decide whether to align on an 8n
or 8n+4 boundary to optimise a later copy.

CPU that support misaligned transfers just make coders sloppy :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Powered by blists - more mailing lists