On Fri, Dec 11, 2015 at 05:17:25PM +0200, Alexander Shishkin wrote: > Peter Zijlstra <[email protected]> writes: > > > On Fri, Dec 11, 2015 at 03:36:36PM +0200, Alexander Shishkin wrote: > >> +static int __perf_event_itrace_filters_setup(void *info) > >> +{ > >> + struct perf_event *event = info; > >> + int ret; > >> + > >> + if (READ_ONCE(event->state) != PERF_EVENT_STATE_ACTIVE) > >> + return -EAGAIN; > >> + > >> + /* matches smp_wmb() in event_sched_in() */ > >> + smp_rmb(); > >> + > >> + /* > >> + * There is a window with interrupts enabled before we get here, > >> + * so we need to check again lest we try to stop another cpu's event. > >> + */ > >> + if (READ_ONCE(event->oncpu) != smp_processor_id()) > >> + return -EAGAIN; > >> + > >> + event->pmu->stop(event, PERF_EF_UPDATE); > >> + rcu_read_lock(); > >> + ret = event->pmu->itrace_filter_setup(event); > >> + rcu_read_unlock(); > >> + event->pmu->start(event, PERF_EF_RELOAD); > > > > Would it not be more sensible to let the ::itrace_filter_setup() method > > do the stop/start-ing if and when needed? > > I don't have a strong opinion on this, the only question is, are we > comfortable with pmu driver callback doing the > rcu_read_lock/unlock, because it still needs to iterate the filter list. > Other than that it's probably a good idea.
See another email; I'm not sure RCU works for this. You can observe more events than you set out for, and if you do multiple iterations of the list they need not match. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
