Question about a patch for stable

Question about a patch for stable

[Steven Rostedt]

Hi Greg,

I was bringing Documentation/trace/ftrace.txt up to date, and obviously
I found some minor bugs in the code while doing so :-)  One of the
things I've discovered, is that the new -mfentry option for gcc used for
function tracing (for only x86 and gcc >= 4.6.0), the stack tracer gives
some bogus results:

     # cat stack_trace
            Depth    Size   Location    (48 entries)
            -----    ----   --------
      0)     4824     216   ftrace_call+0x5/0x2f
      1)     4608     112   ____cache_alloc+0xb7/0x22d
      2)     4496      80   kmem_cache_alloc+0x63/0x12f
[...]

One thing is that it shows the ftrace_call label of the function tracer
instead of the top function and its stack. This is due to fentry being
called as the first operation of a function instead of the way mcount is
called, which is after the stack frame is set up. Because the function
is traced before the stack frame was set up, we lose what function
called the current function. Not only that, the stack frame size (216)
is a combination of that function we missed as well as the ftrace_call
stack size used to save registers.

I have a couple of small fixes that make this more correct:

     # cat stack_trace
            Depth    Size   Location    (14 entries)
            -----    ----   --------
      0)     2640      48   update_group_power+0x26/0x187
      1)     2592     224   update_sd_lb_stats+0x2a5/0x4ac
      2)     2368     160   find_busiest_group+0x31/0x1f1
      3)     2208     256   load_balance+0xd9/0x662

The bug only affects that first entry. Do you think its worth adding to
stable. The changes are solely contained in kernel/trace/trace_stack.c
and do not affect anything else.

I'm not sure I'm going to even submit this for 3.9, and just let it go
into 3.9.1.

I also noticed that the contents of the file stack_max_size doesn't
match the depth (2640 from above), because it also includes the stack
size of the overhead of the stack tracer itself. I have a fix for that
too, but I believe this has always been broken (with and without
-mfentry) and I'm not sure that deserves to be back ported. I'm going to
queue the fix for 3.10. I'm not sure people care about this one or not.

What's your thoughts?

Thanks,

-- Steve

Re: Question about a patch for stable

[Greg Kroah-Hartman]

On Wed, Mar 13, 2013 at 10:09:05PM -0400, Steven Rostedt wrote:
> Hi Greg,
> 
> I was bringing Documentation/trace/ftrace.txt up to date, and obviously
> I found some minor bugs in the code while doing so :-)  One of the
> things I've discovered, is that the new -mfentry option for gcc used for
> function tracing (for only x86 and gcc >= 4.6.0), the stack tracer gives
> some bogus results:
> 
>      # cat stack_trace
>             Depth    Size   Location    (48 entries)
>             -----    ----   --------
>       0)     4824     216   ftrace_call+0x5/0x2f
>       1)     4608     112   ____cache_alloc+0xb7/0x22d
>       2)     4496      80   kmem_cache_alloc+0x63/0x12f
> [...]
> 
> One thing is that it shows the ftrace_call label of the function tracer
> instead of the top function and its stack. This is due to fentry being
> called as the first operation of a function instead of the way mcount is
> called, which is after the stack frame is set up. Because the function
> is traced before the stack frame was set up, we lose what function
> called the current function. Not only that, the stack frame size (216)
> is a combination of that function we missed as well as the ftrace_call
> stack size used to save registers.
> 
> I have a couple of small fixes that make this more correct:
> 
>      # cat stack_trace
>             Depth    Size   Location    (14 entries)
>             -----    ----   --------
>       0)     2640      48   update_group_power+0x26/0x187
>       1)     2592     224   update_sd_lb_stats+0x2a5/0x4ac
>       2)     2368     160   find_busiest_group+0x31/0x1f1
>       3)     2208     256   load_balance+0xd9/0x662
> 
> The bug only affects that first entry. Do you think its worth adding to
> stable. The changes are solely contained in kernel/trace/trace_stack.c
> and do not affect anything else.
> 
> I'm not sure I'm going to even submit this for 3.9, and just let it go
> into 3.9.1.
> 
> I also noticed that the contents of the file stack_max_size doesn't
> match the depth (2640 from above), because it also includes the stack
> size of the overhead of the stack tracer itself. I have a fix for that
> too, but I believe this has always been broken (with and without
> -mfentry) and I'm not sure that deserves to be back ported. I'm going to
> queue the fix for 3.10. I'm not sure people care about this one or not.
> 
> What's your thoughts?

I always defer to the maintainer of the subsystem, so it's up to you.

greg k-h

Re: Question about a patch for stable

[Steven Rostedt]

On Wed, 2013-03-13 at 19:48 -0700, Greg Kroah-Hartman wrote:

> I always defer to the maintainer of the subsystem, so it's up to you.

Great! I'll add them all :-)   It just irritates me when I do a stack
trace and see some strange results. Even when I know they are minor and
understand why they happen. But if they can be fixed, they should be!

-- Steve