"struct perf_sample_data" alignment

* "struct perf_sample_data" alignment
@ 2021-03-05  3:45 Linus Torvalds
  2021-03-05  8:36 ` Peter Zijlstra
  0 siblings, 1 reply; 6+ messages in thread
From: Linus Torvalds @ 2021-03-05  3:45 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim
  Cc: Linux Kernel Mailing List

Sp there's a note about new warnings in 5.12-rc1 that I looked at, and
many of the warnings made me go "Whaaa?". They were all of the type

   warning: 'perf_event_aux_event' uses dynamic stack allocation

and then when I go look, I see nothing that looks like a dynamic stack
allocation at all.

But you know what? The warning is kind of misleading, but at the same
time it's true in a sense. The problem is that the function (and a lot
of other functions) has a local variable like this:

        struct perf_sample_data sample;

and the definition of that "struct perf_sample_data" has
____cacheline_aligned at the end.

And guess what? That means that now the compiler has actually to play
games with manually aligning the frame of that function, since the
natural stack alignment is *not* a full cacheline aligned thing. So
it's kind of true: the frame allocation is mnot a simple static thing,
it's a nasty complex thing that wastes memory and time.

That ____cacheline_aligned goes back many years, this is not new, it
seems to come from back in 2014: commit 2565711fb7d7 ("perf: Improve
the perf_sample_data struct layout").

But it really seems entirely and utterly bogus. That cacheline
alignment makes things *worse*, when the variables are on the local
stack. The local stack is already going to be dirty and in the cache,
and aligning those things isn't going to - and I quote from the code
in that commend in that commit - "minimize the cachelines touched".

Quite the reverse. It's just going to make the stack frame use *more*
memory, and make any cacheline usage _worse_.

Maybe there are static (non-automatic) cases of that "struct
perf_sample_data" where the alignment makes sense, but it does not
seem to make sense on the _type_. It should be on those individual
variables, not on every perf_sample_data.

I didn't make any real effort to analyze this, but from a few quick
greps, it looks like every single case is an automatic variable on the
stack, and that the forced alignment is actually a bad thing every
single time. It never ever generates better cache use, but it _does_
generate worse code, and bigger stack frames.

Hmm? What am I missing?

                 Linus

^ permalink raw reply	[flat|nested] 6+ messages in thread