Received-SPF: pass (google.com: domain of linux-kernel+bounces-88834-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 1 Mar 2024 09:30:23 -0800
From: Charlie Jenkins <charlie@rivosinc.com>
To: David Laight <David.Laight@aculab.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>,
	Guenter Roeck <linux@roeck-us.net>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Helge Deller <deller@gmx.de>,
	"James E.J. Bottomley" <James.Bottomley@hansenpartnership.com>,
	Parisc List <linux-parisc@vger.kernel.org>,
	Arnd Bergmann <arnd@arndb.de>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Russell King <linux@armlinux.org.uk>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Palmer Dabbelt <palmer@rivosinc.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: [PATCH v11] lib: checksum: Use aligned accesses for ip_fast_csum
 and csum_ipv6_magic tests
Message-ID: <ZeIQr3JWoly4JraV@ghost>
References: <20240229-fix_sparse_errors_checksum_tests-v11-1-f608d9ec7574@rivosinc.com>
 <41a5d1e8-6f30-4907-ba63-8a7526e71e04@csgroup.eu>
 <ZeILu9x+/STqWVy+@ghost>
 <3e0e2b25ea2d4ab99e78aff04af94b69@AcuMS.aculab.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3e0e2b25ea2d4ab99e78aff04af94b69@AcuMS.aculab.com>

On Fri, Mar 01, 2024 at 05:24:39PM +0000, David Laight wrote:
> 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)
> 

Can you elaborate on how exactly you suggest the tests to be changed to
accomidate what you are saying here? I don't understand how what I have
proposed doesn't represent the use case of these functions.

- Charlie