Messages in this thread | | | Date | Wed, 20 May 2009 02:24:53 +0200 | From | Frederic Weisbecker <> | Subject | Re: [PATCH] tracing: add trace_event_read_lock() |
| |
On Tue, May 19, 2009 at 10:05:21AM +0800, Lai Jiangshan wrote: > Frederic Weisbecker wrote: > > On Mon, May 18, 2009 at 07:35:34PM +0800, Lai Jiangshan wrote: > >> I found that there is nothing to protect event_hash in > >> ftrace_find_event(). > > > > > > > > Actually, rcu protects it, but not enough. We have neither > > synchronize_rcu() nor rcu_read_lock. > > We have no rcu_read_lock(), RCU can not protects it. > > > > > So we protect against concurrent hlist accesses. > > But the event can be removed when a module is unloaded, > > and that can happen between the time we get the event output > > callback and the time we actually use it. > > > > [...] > > > It could be more fine grained. > > I think it's fine-grained enough, write-side(modules loading/unloading) > is happened rarely. trace_event_read_lock() will not sleep very likely. > > Thoughts?
Yeah, the write lock is a rare event, that's why I think it's enough fine grained.
> > We could have a per event rwsem, and also place the > > protected read section only in trace_print_entry() which is the only racy window. > > > > print_trace_line() is the only racy window. > So I just protect print_trace_line()(except __ftrace_dump()) > > I protect loops which call print_trace_line(), it > reduces invoke-times: > > trace_event_read_lock(); > while (...) { > ... > print_trace_line(); > ... > } > trace_event_read_unlock();
Yeah, I meant it could have been:
trace_event_read_lock(); print_trace_line(); trace_event_read_unlock();
It's more fine grained, but:
- the write lock path is rarely taken - it would add more extra calls then more overhead
IMO this is fine as an rwsem design point of view.
But I have mixed feelings when I consider it could be done using rcu. I will explain that in my next answer to Paul and will wait for your comments.
Thanks!
Frederic.
> Thanks! > Lai > > > But I'm not sure it's that worthy since event removal is a rare thing. > > > > So I guess this patch is fine. > > > > > > > > > >
| |