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

[Arnd Bergmann]

On Fri, Oct 9, 2026, at 23:28, Ian Rogers wrote:
> On Wed, Oct 7, 2026 at 4:00 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>> On Tue, Oct 6, 2026, at 14:31, Marc Kleine-Budde wrote:
>> > On 28.09.2026 17:39:15, Stefan Kerkmann wrote:
>> The previous upstream version was the result of endless discussions, and it
>> looks like changing it to the memcpy version was premature. At the time we
>> unified all architectures to use a common implentation, this was the only one
>> that resulted in correct and fast code on all architectures, so I don't
>> understand why this was just applied without including everyone who was
>> involved in coming up with the version that was replaced.
>>
>> My feeling is that we should just revert this. I'm not sure about the
>> motivation for the change. It sounds like this was meant to be
>> used in userland code, and that clashed with assumptions we make
>> in the kernel, but I don't think that is sufficient reason for
>> regressing kernel code.
>
> So the original motivation for the change was that the perf tool had
> an OpenSSL dependency for the sake of doing a hash when copying jitted
> code into a fake ELF file for the purpose of disassembly and
> symbolization. The kernel contained the same hash function, and using
> the kernel function allowed the perf tool to avoid an OpenSSL
> dependency. Linus asked the perf tool to minimize its dependencies, so
> we pursued this change. The kernel hash function used the
> get_unaligned code for unaligned memory accesses, meaning the change
> required the perf tool to add -fno-strict-aliasing to its build flags
> until we could have a strict aliasing safe get_unaligned. The strict
> aliasing safe code is what we're discussing here and when originally
> written it sat on the mailing list not really doing anything. To my
> surprise Thomas Gleixner picked it up 6 months later, and I believe
> something other than the perf tool motivated this.
>
> There was an issue with the original series on Power IIRC, they had a
> char global variable that they knew was an int through linker tricks.
> The change caused a correct compiler error of a 4 byte copy to a byte
> sized address, and I forget if we used pragmas or a local packed
> struct get_unaligned implementation to work around this. I didn't have
> a way to replicate the problem locally, I was glad others were helping
> with the series!

Ok, thanks for explaining the background.

> Am i going to be able to convince people builtin_memcpy is a better
> unaligned choice than a packed struct? Well memcpy is the expected C
> way to solve this problem, and C compilers like clang implicitly emit
> memcpy intrinsics when copying things like aggregate values. The
> packed struct requires a cast to a pointer type violating strict
> aliasing rules, but has been sound for many years because of
> -fno-strict-aliasing. We'd like the perf tool to be clean for things
> like undefined behavior sanitizers, but separate userland code could
> support this. I was trying to be as useful as possible and I don't
> think the patches were rushed at any point.

Aside from the performance regression, I see more issues with
using __builtin_memcpy() here:

- since the compiler is allowed to always turn __builtin_memcpy()
  into an extern memcpy() call, it looks invalid to use this
  in the vdso, which is not allowed to call any functions.

- the original version used memmove() instead of memcpy(). While
  this was removed in bf067edf5d2f ("openrisc: always use
  unaligned-struct header"), this was surely intentional at the time.

- the use of __unqual_scalar_typeof() turns a relatively simple
  expression into a much larger amount of preprocessed code, which
  tends to confuse the inlining choices and compile speed, especially
  when this is mixed with other macros that expand the arguments
  multiple times.

> What the bug report shows is that there is a lowering problem for
> memcpy with GCC on ARMv5, presumably as ARMv5 lacks unaligned memory
> operations. It seems the packed code shows how we can teach the faster
> unaligned lowering to GCC for ARMv5. Fixing the lowering issue in GCC
> will likely win performance improvements elsewhere on code compiled
> for ARMv5, as optimal memcpy codegen is an expectation for things like
> copying an aggregate value.

As far as I can tell, this only happens when building with -Os,
and I don't even think the decision to call the external memcpy()
is necessarily wrong here. The same thing happens on mips32, mips64,
openrisc, riscv32, riscv64, sh4, sparc32, and sparc64. Some of these
also use an out-of-line bswap32 in put_unaligned_be32() when building
with -Os.

> Fixing GCC would be best but a workaround is to use the packed
> unaligned functions:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/unaligned/packed_struct.h
> I guess this could be a pain if there are regressions elsewhere in the
> kernel that also need fixing.

I hadn't realized that we still have the extra copy for those,
my guess is that we had planned to remove that but somehow never
converted the last remaining user in tools/include/linux/jhash.h
after commit 50b4233a22b1 ("include/linux/jhash.h: replace
__get_unaligned_cpu32 in jhash function") did the second-to-last.

> I believe I spoke to Stefan at LPC on Tuesday and explained all of
> this, suggesting the packed unaligned functions as a workaround. It
> would be interesting to hear of other motivations for memcpy that
> Thomas may know. Perhaps some config value defaulted to memcpy and
> switching to packed structs works for everyone. Maybe separating the
> user and kernel code makes most sense.

Right, that seems easy enough to do: the include/vdso/ headers
are not meant for user consumption in the first place, so it would
make sense to use the struct variant there, and the
include/linux/unaligned/packed_struct.h version could provide the
memcpy() based code for tools/ and get deleted from the kernel
internal version.

It's just complicated a bit by the fact that the current code does
the exact oppposite ;-)

       Arnd