On Wed, Jan 11, 2017 at 02:59:20PM +0000, Mark Rutland wrote: > Hi Peter, > > Sorry for the delay; this fell into my backlog over the holiday. > > On Fri, Dec 09, 2016 at 02:59:00PM +0100, Peter Zijlstra wrote: > > So while I went back and forth trying to make that less ugly, I figured > > there was another problem. > > > > Imagine the cpu_function_call() hitting the 'right' cpu, but not finding > > the task current. It will then continue to install the event in the > > context. However, that doesn't stop another CPU from pulling the task in > > question from our rq and scheduling it elsewhere. > > > > This all lead me to the below patch.. Now it has a rather large comment, > > and while it represents my current thinking on the matter, I'm not at > > all sure its entirely correct. I got my brain in a fair twist while > > writing it. > > > > Please as to carefully think about it. > > FWIW, I've given the below a spin on a few systems, and with it applied > my reproducer no longer triggers the issue. > > Unfortunately, most of the ordering concerns have gone over my head. :/ > > > @@ -2331,13 +2330,36 @@ perf_install_in_context(struct perf_event_context > > *ctx, > > /* > > * Installing events is tricky because we cannot rely on ctx->is_active > > * to be set in case this is the nr_events 0 -> 1 transition. > > + * > > + * Instead we use task_curr(), which tells us if the task is running. > > + * However, since we use task_curr() outside of rq::lock, we can race > > + * against the actual state. This means the result can be wrong. > > + * > > + * If we get a false positive, we retry, this is harmless. > > + * > > + * If we get a false negative, things are complicated. If we are after > > + * perf_event_context_sched_in() ctx::lock will serialize us, and the > > + * value must be correct. If we're before, it doesn't matter since > > + * perf_event_context_sched_in() will program the counter. > > + * > > + * However, this hinges on the remote context switch having observed > > + * our task->perf_event_ctxp[] store, such that it will in fact take > > + * ctx::lock in perf_event_context_sched_in(). > > Sorry if I'm being thick here, but which store are we describing above? > i.e. which function, how does that relate to perf_install_in_context()?
The only store to perf_event_ctxp[] of interest is the initial one in find_get_context(). > I haven't managed to wrap my head around why this matters. :/ See the scenario from: https://lkml.kernel.org/r/20161212124228.ge3...@twins.programming.kicks-ass.net Its installing the first event on 't', which concurrently with the install gets migrated to a third CPU. CPU0 CPU1 CPU2 (current == t) t->perf_event_ctxp[] = ctx; smp_mb(); cpu = task_cpu(t); switch(t, n); migrate(t, 2); switch(p, t); ctx = t->perf_event_ctxp[]; // must not be NULL smp_function_call(cpu, ..); generic_exec_single() func(); spin_lock(ctx->lock); if (task_curr(t)) // false add_event_to_ctx(); spin_unlock(ctx->lock); perf_event_context_sched_in(); spin_lock(ctx->lock); // sees event So its CPU0's store of t->perf_event_ctxp[] that must not go 'missing. Because if CPU2's load of that variable were to observe NULL, it would not try to schedule the ctx and we'd have a task running without its counter, which would be 'bad'. As long as we observe !NULL, we'll acquire ctx->lock. If we acquire it first and not see the event yet, then CPU0 must observe task_running() and retry. If the install happens first, then we must see the event on sched-in and all is well. In any case, I'll try and write a proper Changelog for this...
