Re: [GIT PULL] ftrace/x86: Add frames pointers to trampoline as necessary

[Linus Torvalds]

On Sat, Nov 22, 2014 at 5:05 AM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> Honestly, I did it this way because it was the simplest way to do it.

So the thing is, I already dislike the whole setup, and your patch
just makes it worse.

And part of the problem is that there is *already* too many stupid
layers of indirection, and "setup" macros.

And yes, they really look pretty stupid.

There's MCOUNT_SAVE_FRAME, which is supposed to create a frame, but,
as you found out, doesn't actually do a good job of creating a real
frame, despite its name.

It's also in a really stupid place, excuse my french. Why is it in
that header file?

Then there's "ftrace_caller_setup", which uses MCOUNT_SAVE_FRAME, and
sets up %rsi/rdi to have the parent/rip stuff.

And that one does a particularly bad job *too*, since it's used in
only two of the four places that want this, with the other two doing
the whole thing by hand (look for things like

        movq RIP(%rsp), %rsi
        subq $MCOUNT_INSN_SIZE, %rsi

being duplicated..)

And it's also being particularly stupid about this all, because of the
horrible MCOUNT_SAVE_FRAME placement somewhere else. That
MCOUNT_SAVE_FRAME macro actually loads that %rip value into %rdx, but
because of the extra indirection and odd extra macro in a header file
(even though that one *.S file is the only apparent user),
"ftrace_caller_setup" doesn't actually realize that, and instead just
reloads it from where the previous macro just saved it. So the code it
generates just looks bad too.

And now your patch adds a *third* macro, to make up for the fact that
the first macro did a bad job.

You already have too many of these macros, and they are already ugly.
Why not just *fix* the existing macro, instead of adding yet another
layer of confusion and horror?

> If you want, I can add more comments to explain all this.

I'd really prefer the underlying problem to be fixed, not more
explanations for odd code choices.

Why can't MCOUNT_SAVE_FRAME just save the frame right? Maybe even do
it unconditionally - the cost of setting up a frame is not that huge,
and it might be better to have more understandable code and avoid yet
another ifdef for a config option.

And while at it, why not move it to where it's used, and maybe make it
not scream?

And maybe "ftrace_caller_setup" could be fixed so that the two cases
that now do that magic MCOUNT_INSN_SIZE stuff could use it?

Looking at that file, I just get the heebie-jeebies. It's like a
Halloween house of horrors of macros and #ifdefs. I'd really prefer
for a fix to not just be band-aid that makes it even worse.

                   Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/