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()? I haven't managed to wrap my head around why this matters. :/ Thanks, Mark.
