Messages in this thread | | | Date | Tue, 1 Jul 2014 21:31:47 +0200 | From | Oleg Nesterov <> | Subject | probe_event_disable()->synchronize_sched() (Was: tracing/uprobes: Revert "Support mix of ftrace and perf") |
| |
Namhyung, Masami,
Please look at the question below. Perhaps we discussed this before, but I can recall nothing.
On 06/30, Oleg Nesterov wrote: > > Actually, I'll probably try to make the patch tomorrow. It looks simple > enough, the main complication is CONFIG_PERF. And, to keep this patch > simple, I won't try to optimize (say) the TP_FLAG_TRACE-comes-first > case which could avoid uprobe_apply().
I regret very much I said this ;) OK, I'll probably try anyway, but...
> Yes, I still think it would be better to change the register/unregister > API first, but I do not know when I do this ;)
OK, we can do this later.
But it turns out that trace_uprobe.c needs other cleanups, and I simply can't uglify this code more without these cleanups... Starting from set/clear TP_FLAG_*. I'll recheck, but it seems that there is no reason to carefully set (say) TP_FLAG_TRACE before uprobe_register() and then clear it if _register() fails. And uprobe_dispatcher() is very ugly if is_ret_probe(). And more. So it needs a series.
------------------------------------------------------------------------- And. I am puzzled by probe_event_disable()->synchronize_sched(). Why do we need it? I mean, why we can't use call_rcu() ? The comment says "synchronize with u{,ret}probe_trace_func" but it doesn't explain _why_ do we need to sync.
I thought that perhaps the caller needs to synch with the callbacks. Say, __trace_remove_event_call() can destroy the data which can be used by the callbacks. But no, this is only possible if we are going to call uprobe_unregister(), and this adds the necessary serialization.
So why? Looks like, the only reason is instance_rmdir() which can do TRACE_REG_UNREGISTER and after that destroy this ftrace_event_file? But event_trace_del_tracer() also has synchronize_sched() right after __ftrace_set_clr_event_nolock() ?
So please tell me why do we need this synchronize_sched ;) And imo this should be documented.
Oleg.
| |