Re: [PATCH v11] lib: checksum: Use aligned accesses for ip_fast_csum and csum_ipv6_magic tests

Re: [PATCH v11] lib: checksum: Use aligned accesses for ip_fast_csum and csum_ipv6_magic tests

[Christophe Leroy]

+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.

Christophe

Re: [PATCH v11] lib: checksum: Use aligned accesses for ip_fast_csum and csum_ipv6_magic tests

[Charlie Jenkins]

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.

- Charlie

> Christophe

RE: [PATCH v11] lib: checksum: Use aligned accesses for ip_fast_csum and csum_ipv6_magic tests

[David Laight]

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)

Re: [PATCH v11] lib: checksum: Use aligned accesses for ip_fast_csum and csum_ipv6_magic tests

[Charlie Jenkins]

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