Message-ID: <41d77dbb-468d-4bc3-8f03-a9d4271abd75@csgroup.eu>
Date: Mon, 11 Mar 2024 15:34:15 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Charlie Jenkins <charlie@...osinc.com>, Guenter Roeck <linux@...ck-us.net>
CC: David Laight <David.Laight@...lab.com>, 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>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Palmer Dabbelt <palmer@...osinc.com>
Subject: Re: [PATCH v11] lib: checksum: Use aligned accesses for ip_fast_csum
 and csum_ipv6_magic tests



Le 01/03/2024 à 19:58, Charlie Jenkins a écrit :
> On Fri, Mar 01, 2024 at 10:32:36AM -0800, Guenter Roeck wrote:
>> On 2/29/24 14:46, Charlie Jenkins wrote:
>>> 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.
>>>
>>> Furthermore, all architectures except the m68k pad "struct
>>> csum_ipv6_magic_data" to 44 bytes. To make compatible with the m68k,
>>> manually pad this structure to 44 bytes.
>>>
>>> Fixes: 6f4c45cbcb00 ("kunit: Add tests for csum_ipv6_magic and ip_fast_csum")
>>> Signed-off-by: Charlie Jenkins <charlie@...osinc.com>
>>> Reviewed-by: Guenter Roeck <linux@...ck-us.net>
>>> Acked-by: Palmer Dabbelt <palmer@...osinc.com>
>>> ---
>>> The ip_fast_csum and csum_ipv6_magic tests did not work on all
>>> architectures due to differences in misaligned access support.
>>> Fix those issues by changing endianness of data and aligning the data.
>>>
>>> This patch relies upon a patch from Christophe:
>>>
>>> [PATCH net] kunit: Fix again checksum tests on big endian CPUs
>>>
>>
>> Various test results:
>>
>> On v6.8-rc6-120-g87adedeba51a (current mainline), without this patch
>>
>> - mps2-an385:mps2_defconfig crashes in IPv6 checksum tests
>> - ipv6 checksum tests fail on parisc, parisc64, sh, and sheb.
>>
>> The previously seen problems on big endian systems are still seen with
>> v6.8-rc6, but are gone after commit 3d6423ef8d51 ("kunit: Fix again
>> checksum tests on big endian CPUs") has been applied upstream. This includes
>> the test failures seen with m68k.
>>
>> The parisc/parisc64 test failures are independent of this patch. Fixes are
>> available in linux-next and pending in qemu. The sh/sheb failures are due
>> to upstream commit cadc4e1a2b4 and are no longer seen after reverting that
>> patch.
>>
>> This leaves the mps2-an385:mps2_defconfig crash, which is avoided by this patch.
>> My understanding, which may be wrong, is that arm images with thumb instructions
>> do not support unaligned accesses (maybe I should say do not support unaligned
>> accesses with the mps2-an385 qemu emulation; I did not test with real hardware,
>> after all).
>>
>> Given all that, the continued discussion around the subject, and the lack
>> of agreement if unaligned accesses should be tested or not, I don't really
>> see a path forward for this patch. The remaining known problem is arm with
>> thumb instructions. I don't think that is going to be fixed. I suspect that
>> no one but me even tries to run that code (or any arm:nommu images, for that
>> matter). I'd suggest to drop this patch, and I'll stop testing IP checksum
>> generation for mps2-an385:mps2_defconfig.
>>
>> Sorry for all the noise this has generated.
>>
>> Thanks,
>> Guenter
> 
> If that's what people want. I still don't understand why there is any
> problem with relying on NET_IP_ALIGN as that seems like that macro was
> defined to create an expected alignment.
> 
> It would be nice to use the struct csum_ipv6_magic_data instead of doing
> manual alignment and restricting len to 16 bits. I can send a patch that
> only covers that if people are interested.
> 
> This was my first foray into writing generic test cases and it drew a
> significant amount of criticism. I appreciate Guenter's efforts to make
> the tests have more expected behavior across all supported platforms,
> but the community obviously doesn't agree that is a reasonable goal.
> Makes my life easier though, because then I can just not upstream test
> cases and focus on feature work...

Hi Charlie,

Do not get discouraged.

Your additional kunit checksum tests are very valuable and usefull, and 
as far as I understand they have allowed to identify and fix problems 
that had gone unnoticed until now. I myself would have been very happy 
to have such tests available when I inlined ip_fast_csum() and 
implemented csum_ipv6_magic() for powerpc.

The thing which brought you criticism (from myself at least) is that 
your rencent tentative fixes distort your original work and makes it 
much less useful than it originally was. I fear that if your test had 
been like that at the begining, they wouldn't have allowed to discover 
and fix the same problems.

So yes your tests are very welcome for upstreaming and I encourage you 
to continue upstreaming good tests as the one you initialy upstreamed 
for checksum tests.

If you have not already done, I recommend you to read the guidance about 
patch submission:

https://docs.kernel.org/process/6.Followthrough.html


https://docs.kernel.org/process/submitting-patches.html

And especially

https://docs.kernel.org/process/submitting-patches.html#respond-to-review-comments

and

https://docs.kernel.org/process/submitting-patches.html#don-t-get-discouraged-or-impatient

Christophe

Powered by blists - more mailing lists