Re: [PATCHv2 02/10] ftrace: Change mcount call replacement logic

From: Steven Rostedt
Date: Tue Dec 20 2011 - 11:33:26 EST


On Tue, 2011-12-20 at 14:10 +0100, Jiri Olsa wrote:

> let me try with an example..
>
> say we have only 2 traceable functions - A and B ;)
>
> 1) set global filter for function A with 'echo A > ./set_ftrace_filter'
> a - A is put to the global_ops filter
>
> 2) enable function trace with 'echo function > current_tracer'
> a - register_ftrace_function is called with function trace ftrace_ops (GLOBAL flag)
> b - update_ftrace_function is called, setting ftrace_ops callback function
> to be called directly from the assembly entry_* code
> c - ftrace_hash_rec_enable is called, and dyn_ftrace record
> for function A is updated:
> A::flags|FL_MASK = 1
> d - ftrace_replace_code(1) is called, and function A is
> brought in to life
>
> 3) enable function trace via perf ftrace_ops
> a - register_ftrace_function is called with perf event ftrace_ops (!GLOBAL flag)
> b - update_ftrace_function is called, setting ftrace_ops_list_func
> function to be called from the assembly entry_* code and
> handle the ftrace_ops' dispatch
> c - ftrace_hash_rec_enable is called, and A and B dyn_ftrace records
> are updated:
> A::flags|FL_MASK = 2
> B::flags|FL_MASK = 1
> d - ftrace_replace_code(1) is called, and function B is
> brought in to life
>
> 4) disable function trace via perf ftrace_ops
> a - unregister_ftrace_function is called with perf event ftrace_ops (same as in step 3)
> b - update_ftrace_function is called, setting global ftrace_ops (from step 2)
> callback function to be called directly from the assembly entry_* code
> c - ftrace_hash_rec_disable is called, and A and B dyn_ftrace
> records are updated:
> A::flags|FL_MASK = 1
> B::flags|FL_MASK = 0
> d - ??? see below..
>
> Now, only the global function trace ftrace_ops is enabled (from step 2),
> but both A and B are alive and feeding the tracer despite its filter (function A),
> because its ftrace_ops is directly linked to the assembly entry_* code (step 4b).
>
> The reason is that even though we updated the B's dyn_ftrace record (step 4c)
> to be 'B::flags|FL_MASK == 0', we did not update the B call itself and disable it.
>
> If we'd call ftrace_replace_code(1) at 4d), the B function would be
> disabled, and we would get expected behaviour.
>
> So thats the reason I think we should update the calls (mcount call code)
> each time we unregister the ftrace_ops, because some records could be
> enabled only for the specific ftrace_ops and we need to put them down
> when we disable this ftrace_ops.
>
>
> hopefully it make any sense.. :)

Ah, OK, I am able to trigger this. I see what you mean. Yeah, now that
we have a counter, it should enable/disable based on the counter. And
the DISABLE is still needed for just the "disable regardless"
(/proc/sys/kernel/ftrace_enabled = 0).

I'll apply your patch and see how it works. Thanks!

-- Steve


--
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/