Re: Perf hotplug lockup in v4.9-rc8

Mark Rutland
2017-01-11T16:27:10Z

On Wed, Jan 11, 2017 at 05:03:58PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 11, 2017 at 02:59:20PM +0000, Mark Rutland wrote:
> > On Fri, Dec 09, 2016 at 02:59:00PM +0100, Peter Zijlstra wrote:

> > > + * 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().

Ah, I see. I'd missed the rcu_assign_pointer() when looking around for
an assignment.

> > I haven't managed to wrap my head around why this matters. :/
>
> See the scenario from:
>
> https://lkml.kernel.org/r/20161212124228.GE3124@twins.programming.kicks-ass.net
>
> Its installing the first event on 't', which concurrently with the
> install gets migrated to a third CPU.

I was completely failing to consider that this was the installation of
the first event; I should have read the existing comment. Things make a
lot more sense now.

> 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.

I think I follow now. Thanks for bearing with me!

> In any case, I'll try and write a proper Changelog for this...

If it's just the commit message and/or comments changing, feel free to
add:

Tested-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.