Messages in this thread | | | From | Namhyung Kim <> | Subject | Re: probe_event_disable()->synchronize_sched() | Date | Thu, 03 Jul 2014 09:54:57 +0900 |
| |
Hi Oleg,
On Tue, 1 Jul 2014 21:31:47 +0200, Oleg Nesterov wrote: > Namhyung, Masami, > > Please look at the question below. Perhaps we discussed this before, > but I can recall nothing.
I'm not sure I grok the code enough to answer your question, but...
> > > 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.
Okay, I'd like to see it. :)
> > ------------------------------------------------------------------------- > 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.
It looks like the code was copied from trace_kprobe.c file. But IIUC, unlike kprobes, uprobe events are always called in a process context.
Also u{,ret}probe_trace_func() call handlers under rcu_read_lock() not rcu_read_lock_sched() so I guess the synchronize_sched() can go.
Thanks, Namhyung
> > 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.
| |