Messages in this thread | | | Subject | Re: [PATCH 1/2] trace_events_filter: use rcu_assign_pointer() when setting ftrace_event_call->filter | From | Steven Rostedt <> | Date | Wed, 23 Nov 2011 10:16:47 -0500 |
| |
[ Added Paul to Cc ]
On Tue, 2011-11-22 at 17:46 -0800, Tejun Heo wrote: > ftrace_event_call->filter is sched RCU protected but didn't use > rcu_assign_pointer(). Fix it.
Is it really needed? Maybe just for documentation but I'm not sure this use is required because all use cases have synchronize_sched() used, which is a big hammer compared to the rcu_assign_pointer().
> > TODO: Add proper __rcu annotation to call->filter and all its users. > > Signed-off-by: Tejun Heo <tj@kernel.org> > --- > kernel/trace/trace_events_filter.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > Index: work/kernel/trace/trace_events_filter.c > =================================================================== > --- work.orig/kernel/trace/trace_events_filter.c > +++ work/kernel/trace/trace_events_filter.c > @@ -1686,7 +1686,7 @@ static int replace_system_preds(struct e > * replace the filter for the call. > */ > filter = call->filter; > - call->filter = filter_item->filter; > + rcu_assign_pointer(call->filter, filter_item->filter);
We update filter here, and then call synchronize_sched() before we free the filter_item->filter.
> filter_item->filter = filter; > > fail = false; > @@ -1741,7 +1741,7 @@ int apply_event_filter(struct ftrace_eve > filter = call->filter; > if (!filter) > goto out_unlock; > - call->filter = NULL; > + rcu_assign_pointer(call->filter, NULL); > /* Make sure the filter is not being used */
Again you can see that synchronize_sched() is called here.
> synchronize_sched(); > __free_filter(filter); > @@ -1782,7 +1782,7 @@ out: > * string > */ > tmp = call->filter; > - call->filter = filter; > + rcu_assign_pointer(call->filter, filter);
We only call synchronize_sched if call->filter wasn't NULL, because we are going to free tmp. We need to make sure all users are done with tmp before we free it.
> if (tmp) { > /* Make sure the call is done with the filter */ > synchronize_sched();
Thus my question is, do we really need to add the rcu_assign_pointer(). I have no problem if we only do so to document that this is an rcu sched protected variable. But it should be commented as such.
-- Steve
| |