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

[Oleg Nesterov]

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

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.

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

> +	ctx->is_active = 1;
> +	if (cpuctx->task_ctx == ctx || !ctx->nr_events) {
> +		raw_spin_lock(&ctx->lock);
>  		return;

I guess you meant _unlock.

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?

This looks wrong at first glance. Suppose we have the same problem,
this task misses perf_event_context_sched_out() after that. OK,
->task_ctx == NULL.

But, suppose that after that this task sleeps "forever" and we create
another counter and call perf_install_in_context() again. Now we hang
in "retry" loop.

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

> +	}
> +	raw_spin_lock(&ctx->lock);

Again, s/lock/unlock/

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

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/