Re: [Regression] [PATCH v5 2/4] vdso: Switch get/put unaligned from packed struct to memcpy

[Stefan Kerkmann <s.kerkmann@pengutronix.de>]

Hi Ian,

On 10/16/25 22:51, Ian Rogers wrote:
> Type punning is necessary for get/put unaligned but the use of a
> packed struct violates strict aliasing rules, requiring
> -fno-strict-aliasing to be passed to the C compiler. Switch to using
> memcpy so that -fno-strict-aliasing isn't necessary.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  include/vdso/unaligned.h | 41 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/include/vdso/unaligned.h b/include/vdso/unaligned.h
> index ff0c06b6513e..9076483c9fbb 100644
> --- a/include/vdso/unaligned.h
> +++ b/include/vdso/unaligned.h
> @@ -2,14 +2,43 @@
>  #ifndef __VDSO_UNALIGNED_H
>  #define __VDSO_UNALIGNED_H
>  
> -#define __get_unaligned_t(type, ptr) ({							\
> -	const struct { type x; } __packed * __get_pptr = (typeof(__get_pptr))(ptr);	\
> -	__get_pptr->x;									\
> +#include <linux/compiler_types.h>
> +
> +/**
> + * __get_unaligned_t - read an unaligned value from memory.
> + * @type:	the type to load from the pointer.
> + * @ptr:	the pointer to load from.
> + *
> + * Use memcpy to affect an unaligned type sized load avoiding undefined behavior
> + * from approaches like type punning that require -fno-strict-aliasing in order
> + * to be correct. As type may be const, use __unqual_scalar_typeof to map to a
> + * non-const type - you can't memcpy into a const type. The
> + * __get_unaligned_ctrl_type gives __unqual_scalar_typeof its required
> + * expression rather than type, a pointer is used to avoid warnings about mixing
> + * the use of 0 and NULL. The void* cast silences ubsan warnings.
> + */
> +#define __get_unaligned_t(type, ptr) ({					\
> +	type *__get_unaligned_ctrl_type __always_unused = NULL;		\
> +	__unqual_scalar_typeof(*__get_unaligned_ctrl_type) __get_unaligned_val; \
> +	__builtin_memcpy(&__get_unaligned_val, (void *)(ptr),		\
> +			 sizeof(__get_unaligned_val));			\
> +	__get_unaligned_val;						\
>  })
>  
> -#define __put_unaligned_t(type, val, ptr) do {						\
> -	struct { type x; } __packed * __put_pptr = (typeof(__put_pptr))(ptr);		\
> -	__put_pptr->x = (val);								\
> +/**
> + * __put_unaligned_t - write an unaligned value to memory.
> + * @type:	the type of the value to store.
> + * @val:	the value to store.
> + * @ptr:	the pointer to store to.
> + *
> + * Use memcpy to affect an unaligned type sized store avoiding undefined
> + * behavior from approaches like type punning that require -fno-strict-aliasing
> + * in order to be correct. The void* cast silences ubsan warnings.
> + */
> +#define __put_unaligned_t(type, val, ptr) do {				\
> +	type __put_unaligned_val = (val);				\
> +	__builtin_memcpy((void *)(ptr), &__put_unaligned_val,		\
> +			 sizeof(__put_unaligned_val));			\
>  } while (0)
>  
>  #endif /* __VDSO_UNALIGNED_H */

commit a339671db64b ("vdso: Switch get/put_unaligned() from packed struct to
memcpy()"), which landed in 7.0, causes a performance regression on an NXP
i.MX25 (ARMv5TE) SoC.

I found it while updating a client's board from 6.12 to 7.0. A fio 4k randwrite
benchmark on a NAND storage with UBI and UBIFS filesystem was the only workload
that showed a clear regression between those two versions, so I bisected with
it:

perf stat -e irq:irq_handler_entry --filter 'irq == 49' -a \
    -- \
    fio --name=rw \
    --filename=/var/stat/testfile \
    --size=8M \
    --rw=randwrite \
    --bs=4k \
    --direct=0 \
    --fsync=1 \
    --numjobs=4 \
    --group_reporting

| kernel | irq_handler_entry | fio bw   |
| ------ | ----------------- | -------- |
| 6.12   | 155783            | 401KiB/s |
| 6.13   | 162317            | 395KiB/s |
| 6.14   | 168741            | 392KiB/s |
| 6.15   | 168556            | 401KiB/s |
| 6.16   | 166090            | 403KiB/s |
| 6.17   | 162541            | 385KiB/s |
| 6.18   | 157527            | 386KiB/s |
| 6.19   | 183675            | 381KiB/s |
| 7.0    | 190372            | 297KiB/s |

The bisect targeted the large drop between 6.19 and 7.0; the smaller 6.17
regression predates this commit and is unrelated. Reverting a339671db64b
restores throughput to the 6.17 level (~385 KiB/s). The commit is still
present in 7.3-rc5, and the same codegen problem reproduces there.

Digging deeper, I built 7.3-rc5 with my config and GCC 16.2, with and without
the commit, and compared the object files: 114 of them differ. As
<vdso/unaligned.h> is included by <linux/unaligned.h>, every
get/put_unaligned() call site depends on it transitively. GCC did not inline
__builtin_memcpy() and turned it into a function call, e.g. in crypto/crc32c.c
(__chksum_finup(), inlined into chksum_digest()):

Without the commit:

<chksum_digest>:
                   str    lr, [sp, #-0x4]!
                   sub    sp, sp, #12
                   str    lr, [sp, #-0x4]!
                   bl    0xc0 <chksum_digest+0xc> @ imm = #-0x8
        R_ARM_CALL    __gnu_mcount_nc
                   ldr    r0, [r0]
                   str    r3, [sp, #0x4]
                   ldr    r0, [r0, #0x20]
                   bl    0xd0 <chksum_digest+0x1c> @ imm = #-0x8
        R_ARM_CALL    crc32c
                   mvn    r2, r0
                   mov    r0, #0
                   ldr    r3, [sp, #0x4]
                   lsr    r12, r2, #8
                   lsr    r1, r2, #16
                   strb    r2, [r3]
                   lsr    r2, r2, #24
                   strb    r12, [r3, #0x1]
                   strb    r1, [r3, #0x2]
                   strb    r2, [r3, #0x3]
                   add    sp, sp, #12
                   ldr    pc, [sp], #4

With the commit:

<chksum_digest>:
                   push    {r4, lr}
                   sub    sp, sp, #8
                   str    lr, [sp, #-0x4]!
                   bl    0x124 <chksum_digest+0xc> @ imm = #-0x8
        R_ARM_CALL    __gnu_mcount_nc
                   ldr    r0, [r0]
                   ldr    r12, [pc, #0x54]        @ 0x188 <chksum_digest+0x70>
                   ldr    r0, [r0, #0x20]
                   mov    r4, r3
                   ldr    r12, [r12]
                   str    r12, [sp, #0x4]
                   mov    r12, #0
                   bl    0x144 <chksum_digest+0x2c> @ imm = #-0x8
        R_ARM_CALL    crc32c
                   mvn    r3, r0
                   mov    r2, #4
                   mov    r0, r4
                   mov    r1, sp
                   str    r3, [sp]
                   bl    0x15c <chksum_digest+0x44> @ imm = #-0x8
        R_ARM_CALL    memcpy
                   ldr    r3, [pc, #0x20]         @ 0x188 <chksum_digest+0x70>
                   ldr    r2, [r3]
                   ldr    r3, [sp, #0x4]
                   eors    r2, r3, r2
                   mov    r3, #0
                   bne    0x184 <chksum_digest+0x6c> @ imm = #0x8
                   mov    r0, #0
                   add    sp, sp, #8
                   pop    {r4, pc}
                   bl    0x184 <chksum_digest+0x6c> @ imm = #-0x8
        R_ARM_CALL    __stack_chk_fail
     188: 00 00 00 00      .word    0x00000000
        R_ARM_ABS32    __stack_chk_guard

Is this an accepted trade-off? My understanding is that the kernel is always
built with -fno-strict-aliasing, so the packed-struct type punning was well
defined there, and the __packed annotation is what lets GCC generate valid
code for the unaligned access.

Best regards,
Stefan

-- 
Pengutronix e.K.                       | Stefan Kerkmann             |
Steuerwalder Str. 21                   | https://www.pengutronix.de/ |
31137 Hildesheim, Germany              | Phone: +49-5121-206917-128  |
Amtsgericht Hildesheim, HRA 2686       | Fax:   +49-5121-206917-9    |