Re: [PATCH] LoongArch: Add unaligned access support

[Arnd Bergmann]

On Mon, Oct 17, 2022, at 9:31 AM, Huacai Chen wrote:
> Hi, Arnd,
>
> On Mon, Oct 17, 2022 at 3:12 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>>
>> On Sun, Oct 16, 2022, at 3:34 PM, Huacai Chen wrote:
>> > Loongson-2 series (Loongson-2K500, Loongson-2K1000) don't support
>> > unaligned access in hardware, while Loongson-3 series (Loongson-3A5000,
>> > Loongson-3C5000) are configurable whether support unaligned access in
>> > hardware. This patch add unaligned access emulation for those LoongArch
>> > processors without hardware support.
>> >
>> > Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx>
>>
>> What does the Loongarch ELF ABI say about this? On most architectures,
>> C compilers are not allowed to produce unaligned accesses for standard
>> compliant source code, the only way you'd get this is when casting
>> a an unaligned (e.g. char*) pointer to another type with higher alignment
>> requirement.
> Some unaligned accesses are observed from the kernel network stack, it
> seems related to whether the packet aligns to IP header or MAC header.

This is usually a bug in the device driver. It's a fairly common bug
since the network driver has to ensure the alignment is correct, but
it's usually fixable, and fixing it results in better performance on
machines that support unaligned access as well.

Which driver did you observe this with?

> And, gcc has a -mstrict-align parameter, if without this, there are
> unaligned instructions.

Does this default to strict or non-strict mode? Usually gcc does not
allow to turn this off on architectures that have no hardware support
for unaligned access.

>> > +/* sysctl hooks */
>> > +int unaligned_enabled __read_mostly = 1;     /* Enabled by default */
>> > +int no_unaligned_warning __read_mostly = 1;  /* Only 1 warning by default */
>>
>> The comment says 'sysctl', the implementation has a debugfs interface.
> Originally "enabled", "warning" and "counters" are all debugfs
> interfaces, then you told me to use sysctl. Now in this version
> "enabled" and "warning" are converted to sysctl, but there are no
> existing "counters" sysctl.

I don't see the sysctl interface in the patch, what am I missing?

      Arnd