Re: Q: perf_install_in_context/perf_event_enable are racy?

From: Oleg Nesterov
Date: Mon Jan 24 2011 - 06:50:42 EST

On 01/21, Frederic Weisbecker wrote:
> +static DEFINE_PER_CPU(int, task_events_schedulable);

Yes, I think this can work. I thought about this too. The only problem,
this doesn't make the whole code more understandable ;)

> @@ -1587,6 +1594,8 @@ void __perf_event_task_sched_in(struct task_struct *task)
> struct perf_event_context *ctx;
> int ctxn;
> + __get_cpu_var(task_events_schedulable) = 1;
> +
> for_each_task_context_nr(ctxn) {
> ctx = task->perf_event_ctxp[ctxn];
> if (likely(!ctx))

This doesn't look right. We should set task_events_schedulable
_after_ perf_event_context_sched_in(), otherwise we have the similar
race with next.

rq->curr and current_task were already updated. __perf_install_in_context
should not set "cpuctx->task_ctx = next" before perf_event_context_sched_in(),
it does nothing if cpuctx->task_ctx == ctx.

OTOH, if we set task_events_schedulable after for_each_task_context_nr(),
then we have another race with next, but this race is minor. If
find_get_context() + perf_install_in_context() happen in this window,
the new event won't be scheduled until next reschedules itself.

> + /*
> + * Every pending sched switch must finish so that
> + * we ensure every pending calls to perf_event_task_sched_in/out are
> + * finished. We ensure the next ones will correctly handle the
> + * perf_task_events label and then the task_events_schedulable
> + * state. So perf_install_in_context() won't install events
> + * in the tiny race window between perf_event_task_sched_out()
> + * and perf_event_task_sched_in() in the __ARCH_WANT_INTERRUPTS_ON_CTXSW
> + * case.
> + */
> + synchronize_sched();

Yes, if perf_task_events was zero before perf_event_alloc(), then it
is possible that task_events_schedulable == 1 while schedule() is in
progress. perf_event_create_kernel_counter() needs this too.

Frederic, All, can't we simplify this?

First, we modify __perf_install_in_context() so that it never tries
to install the event into !is_active context. IOW, it never tries
to set cpuctx->task_ctx = ctx.

Then we add the new trivial helper stop_resched_task(task) which
simply wakeups the stop thread on task_cpu(task), and thus forces
this task to reschedule.


static void
perf_install_in_context(struct perf_event_context *ctx,
struct perf_event *event,
int cpu)
struct task_struct *task = ctx->task;

event->ctx = ctx;

if (!task) {
* Per cpu events are installed via an smp call and
* the install is always successful.
smp_call_function_single(cpu, __perf_install_in_context,
event, 1);

for (;;) {
bool done, need_resched = false;

done = !list_empty(&event->group_entry);
if (!done && !ctx->is_active) {
add_event_to_ctx(event, ctx);
need_resched = task_running(task);
done = true;

if (done) {
if (need_resched)

task_oncpu_function_call(task, __perf_install_in_context,

Yes, stop_resched_task() can't help if this task itself is the stop thread.
But the stop thread shouldn't run for a long time without rescheduling,
otherwise we already have the problems.

Do you all think this makes any sense?


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at