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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Feb 26, 2024 at 10:35:18AM -0800, Charlie Jenkins wrote:
> On Mon, Feb 26, 2024 at 05:50:57PM +0000, Russell King (Oracle) wrote:
> > On Mon, Feb 26, 2024 at 08:44:29AM -0800, Guenter Roeck wrote:
> > > On 2/26/24 03:34, Christophe Leroy wrote:
> > > > 
> > > > 
> > > > Le 23/02/2024 à 23:11, 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.
> > > > 
> > > > I'm still wondering what we are really trying to fix here.
> > > > 
> > > > All other tests are explicitely testing that it works with any alignment.
> > > > 
> > > > Shouldn't ip_fast_csum() and csum_ipv6_magic() work for any alignment as
> > > > well ? I would expect it, I see no comment in arm code which explicits
> > > > that assumption around those functions.
> > > > 
> > > > Isn't the problem only the following line, because csum_offset is
> > > > unaligned ?
> > > > 
> > > > csum = *(__wsum *)(random_buf + i + csum_offset);
> > > > 
> > > > Otherwise, if there really is an alignment issue for the IPv6 source or
> > > > destination address, isn't it enough to perform a 32 bits alignment ?
> > > > 
> > > 
> > > It isn't just arm.
> > > 
> > > Question should be what alignments the functions are supposed to be able
> > > to handle, not what they are optimized for. If byte and/or half word alignments
> > > are expected to be supported, there is still architecture code which would
> > > have to be fixed. Unaligned accesses are known to fail on hppa64/parisc64
> > > and on sh4, for example. If unaligned accesses are expected to be handled,
> > > it would probably make sense to add a separate test case, though, to clarify
> > > that the test fails due to alignment issues, not due to input parameters.
> > 
> > It's network driver dependent. Most network drivers receive packets
> > to the offset defined by NET_IP_ALIGN (which is normally 2) which
> > has the effect of "mis-aligning" the ethernet header, but aligning
> > the IP header.
> > 
> > Whether drivers do that is up to drivers (and their capabilities).
> > Some network drivers can not do this kind of alignment, so there are
> > cases where the received packets aren't offset by two bytes, leading
> > to the IP header being aligned to an odd 16-bit word rather than an
> > even 16-bit word (and thus 32-bit aligned.)
> > 
> > Then you have the possibility of other headers between the ethernet
> > and IP header - not only things like VLANs, but also possibly DSA
> > headers (for switches) and how big those are.
> 
> Those additional combinations can be supported by future test cases,
> but the goal of this patch was simply to have basic testing for these
> functions. The NET_IP_ALIGN offset is what the kernel defines to be
> supported, so that is the test case I went for.

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!