Messages in this thread | | | Subject | Re: [PATCH] perf: Cure task_oncpu_function_call() races | From | Peter Zijlstra <> | Date | Tue, 01 Feb 2011 19:18:13 +0100 |
| |
On Tue, 2011-02-01 at 19:08 +0100, Peter Zijlstra wrote: > > > +static void perf_remove_from_context(struct perf_event *event) > > > { > > > ... > > > raw_spin_lock_irq(&ctx->lock); > > > /* > > > + * If we failed to find a running task, but find it running now that > > > + * we've acquired the ctx->lock, retry. > > > */ > > > + if (task_curr(task)) { > > > raw_spin_unlock_irq(&ctx->lock); > > > goto retry; > > > } > > > > > > /* > > > + * Since the task isn't running, its safe to remove the event, us > > > + * holding the ctx->lock ensures the task won't get scheduled in. > > > */ > > > + list_del_event(event, ctx); > > > > this looks suspicious (the same for perf_install_in_context). > > > > Unlike the IPI handler, this can see schedule-in-progress in any state. > > In particular, we can see rq->curr == next (so that task_curr() == F), > > but before "prev" has already called perf_event_task_sched_out(). > > > > So we have to check ctx->is_active, or schedule() should change rq->curr > > after perf_event_task_sched_out(). > > I only considered current == next in that case, not current == prev, let > me undo some of those sched.c bits and put a comment.
On second thought, your proposed ->is_active check seems to result in much nicer code in sched.c. Let me think through that.
| |