Re: [PATCH,RFC] perf: panic due to inclied cpu context task_ctx value

[Peter Zijlstra <peterz@infradead.org>]

On Tue, 2011-03-29 at 18:28 +0200, Oleg Nesterov wrote:
> On 03/29, Peter Zijlstra wrote:
> >
> > On Mon, 2011-03-28 at 18:56 +0200, Oleg Nesterov wrote:
> > >
> > > jump_label_dec:
> > >
> > > 	if (atomic_dec_and_test(key))
> > > 		jump_label_disable(key);
> > >
> > > Another thread can create the PERF_ATTACH_TASK event in between
> > > and call jump_label_update(JUMP_LABEL_ENABLE) first. Looks like,
> > > jump_label_update() should ensure that "type" matches the state
> > > of the "*key" under jump_label_lock().
> >
> > No I think you're right, and I think we fixed that but it looks like
> > Ingo still didn't merge the new jump-label patches :/
> 
> OK. To remind, we have another problem, perf_install can race with exit.
> But lets ignore this for now...

Yay! ;-)

> 
> You know, I honestly tried to convince myself I can understand your
> patch. All I can say, I'll try to read it again ;) But the main idea
> is clear, we give more respect to ->nr_events and once it is zero
> task_ctx must not be active.

Right, except I'm leaking an ->is_active and was seriously considering
your clever idea of splitting the sched_in and sched_out jump-labels.

> > @@ -2114,8 +2107,19 @@ static void perf_event_context_sched_in(
> >  	struct perf_cpu_context *cpuctx;
> >
> >  	cpuctx = __get_cpu_context(ctx);
> > -	if (cpuctx->task_ctx == ctx)
> > +	raw_spin_lock(&ctx->lock);
> > +	/*
> > +	 * Serialize against perf_install_in_context(), the interesting case
> > +	 * is where perf_install_in_context() finds the context inactive and
> > +	 * another cpu is just about to schedule the task in. In that case
> > +	 * we need to avoid observing a stale ctx->nr_events.
> 
> I don't understand the comment... We can't race __perf_install_in_context,
> it can only run on the same CPU but we are called with irqs disabled.

Ah, I meant a race with perf_install_in_context() where task_function()
fails because !task_curr(), in that case we'll attempt
add_event_to_ctx() from the remote cpu. If we race wrong with sched_in
nobody might schedule the event.

> > +	ctx->is_active = 1;
> > +	if (cpuctx->task_ctx == ctx || !ctx->nr_events) {
> > +		raw_spin_lock(&ctx->lock);
> >  		return;
> 
> I guess you meant _unlock.

Yeah, utter fail day today :/

> But now I don't understand what ->is_active means. We make it true,
> but doesn't set cpuctx->task_ctx = ctx. Why __perf_event_release()
> clears ->is_active then?

Yeha, that's the big problem I have with this.

> It seems to me, instead we should change ctx_sched_in() to check
> nr_events and do nothing if it is zero.

you mean, not set ->is_active?

> > +	cpuctx->task_ctx = ctx;
> > +
> >  	ctx_sched_in(ctx, cpuctx, EVENT_PINNED, task);
> 
> But we already dropped ctx->lock, __perf_event_release() can remove
> the last event before ctx_sched_in() takes it again.
> 
> This should be moved into ctx_sched_in() afaics, but this is not simple.
> 
> So, perhaps we can take ctx->lock and check nr_events after the 2nd
> ctx_sched_in(). If it is zero, we should clear task_ctx/is_active.
> 
> Perhaps. Right now I got lost.

Yeah, you and me both.. I went to look at something else because I
simply confused myself more. Hopefully tomorrow will bring some sanity.