Re: Q: perf_install_in_context/perf_event_enable are racy?

From: Peter Zijlstra
Date: Fri Jan 21 2011 - 10:04:40 EST


On Fri, 2011-01-21 at 15:26 +0100, Oleg Nesterov wrote:
>
> > Ah, I think I see how that works:
>
> Hmm. I don't...
>
> >
> > __perf_event_task_sched_out()
> > perf_event_context_sched_out()
> > if (do_switch)
> > cpuctx->task_ctx = NULL;
>
> exactly, this clears ->task_ctx
>
> > vs
> >
> > __perf_install_in_context()
> > if (cpu_ctx->task_ctx != ctx)
>
> And then __perf_install_in_context() sets cpuctx->task_ctx = ctx,
> because ctx->task == current && cpuctx->task_ctx == NULL.

Hrm,. right, so the comment suggests it should do what it doesn't :-)

It looks like Paul's a63eaf34ae60bd (perf_counter: Dynamically allocate
tasks' perf_counter_context struct), relevant hunk below, wrecked it:

@@ -568,11 +582,17 @@ static void __perf_install_in_context(void *info)
* If this is a task context, we need to check whether it is
* the current task context of this cpu. If not it has been
* scheduled out before the smp call arrived.
+ * Or possibly this is the right context but it isn't
+ * on this cpu because it had no counters.
*/
- if (ctx->task && cpuctx->task_ctx != ctx)
- return;
+ if (ctx->task && cpuctx->task_ctx != ctx) {
+ if (cpuctx->task_ctx || ctx->task != current)
+ return;
+ cpuctx->task_ctx = ctx;
+ }

spin_lock_irqsave(&ctx->lock, flags);
+ ctx->is_active = 1;
update_context_time(ctx);

/*


I can't really seem to come up with a sane test that isn't racy with
something, my cold seems to have clogged not only my nose :/
--
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/