Message-ID: <c0449c0a-33bc-49c4-97e3-56a79a6ce93e@csgroup.eu>
Date: Wed, 28 Feb 2024 07:25:58 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Charlie Jenkins <charlie@...osinc.com>, "Russell King (Oracle)"
	<linux@...linux.org.uk>
CC: Guenter Roeck <linux@...ck-us.net>, 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>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Palmer Dabbelt
	<palmer@...osinc.com>, Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v10] lib: checksum: Use aligned accesses for ip_fast_csum
 and csum_ipv6_magic tests



Le 28/02/2024 à 01:21, Charlie Jenkins a écrit :
> On Tue, Feb 27, 2024 at 10:28:45AM +0000, Russell King (Oracle) wrote:
>> On Tue, Feb 27, 2024 at 06:47:38AM +0000, Christophe Leroy wrote:
>>>
>>>
>>> Le 27/02/2024 à 00:48, Guenter Roeck a écrit :
>>>> On 2/26/24 15:17, Charlie Jenkins wrote:
>>>>> On Mon, Feb 26, 2024 at 10:33:56PM +0000, David Laight wrote:
>>>>>> ...
>>>>>>> I think you misunderstand. "NET_IP_ALIGN offset is what the kernel
>>>>>>> defines to be supported" is a gross misinterpretation. It is not
>>>>>>> "defined to be supported" at all. It is the _preferred_ alignment
>>>>>>> nothing more, nothing less.
>>>>>
>>>>> This distinction is arbitrary in practice, but I am open to being proven
>>>>> wrong if you have data to back up this statement. If the driver chooses
>>>>> to not follow this, then the driver might not work. ARM defines the
>>>>> NET_IP_ALIGN to be 2 to pad out the header to be on the supported
>>>>> alignment. If the driver chooses to pad with one byte instead of 2
>>>>> bytes, the driver may fail to work as the CPU may stall after the
>>>>> misaligned access.
>>>>>
>>>>>>
>>>>>> I'm sure I've seen code that would realign IP headers to a 4 byte
>>>>>> boundary before processing them - but that might not have been in
>>>>>> Linux.
>>>>>>
>>>>>> I'm also sure there are cpu which will fault double length misaligned
>>>>>> memory transfers - which might be used to marginally speed up code.
>>>>>> Assuming more than 4 byte alignment for the IP header is likely
>>>>>> 'wishful thinking'.
>>>>>>
>>>>>> There is plenty of ethernet hardware that can only write frames
>>>>>> to even boundaries and plenty of cpu that fault misaligned accesses.
>>>>>> There are even cases of both on the same silicon die.
>>>>>>
>>>>>> You also pretty much never want a fault handler to fixup misaligned
>>>>>> ethernet frames (or really anything else for that matter).
>>>>>> It is always going to be better to check in the code itself.
>>>>>>
>>>>>> x86 has just made people 'sloppy' :-)
>>>>>>
>>>>>>      David
>>>>>>
>>>>>> -
>>>>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
>>>>>> MK1 1PT, UK
>>>>>> Registration No: 1397386 (Wales)
>>>>>>
>>>>>
>>>>> If somebody has a solution they deem to be better, I am happy to change
>>>>> this test case. Otherwise, I would appreciate a maintainer resolving
>>>>> this discussion and apply this fix.
>>>>>
>>>> Agreed.
>>>>
>>>> I do have a couple of patches which add explicit unaligned tests as well as
>>>> corner case tests (which are intended to trigger as many carry overflows
>>>> as possible). Once I get those working reliably, I'll be happy to submit
>>>> them as additional tests.
>>>>
>>>
>>> The functions definitely have to work at least with and without VLAN,
>>> which means the alignment cannot be greater than 4 bytes. That's also
>>> the outcome of the discussion.
>>
>> Thanks for completely ignoring what I've said. No. The alignment ends up
>> being commonly 2 bytes.
>>
>> As I've said several times, network drivers do _not_ have to respect
>> NET_IP_ALIGN. There are 32-bit ARM drivers which have a DMA engine in
>> them which can only DMA to a 32-bit aligned address. This means that
>> the start of the ethernet header is placed at a 32-bit aligned address
>> making the IP header misaligned to 32-bit.
>>
>> I don't see what is so difficult to understand about this... but it
>> seems that my comments on this are being ignored time and time again,
>> and I can only think that those who are ignoring my comments have
>> some alterior motive here.
>>
>> -- 
>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> 
> I don't understand how the capabilities of some ARM drivers factor in
> here. It appears that a common case for calling this function is to pass
> in an IP header that is aligned along an ethernet header + NET_IP_ALIGN.
> It is perfectly acceptable that some drivers don't align along
> NET_IP_ALIGN, but that does not seem relevant here.
> 
> This test case is supposed to be as true to the "general case" as
> possible, so I have aligned the data along 14 + NET_IP_ALIGN. On ARM
> this will be a 16-byte boundary since NET_IP_ALIGN is 2. A driver that
> does not follow this may not be appropriately tested by this test case,
> but anyone is welcome to submit additional test cases that address this
> additional alignment concern.

But then this test case is becoming less and less true to the "general 
case" with this patch, whereas your initial implementation was almost 
perfect as it was covering most cases, a lot more than what we get with 
that patch applied.

Powered by blists - more mailing lists