Messages in this thread | | | Date | Thu, 27 Jan 2011 17:10:47 +0100 | From | Oleg Nesterov <> | Subject | Re: Q: perf_install_in_context/perf_event_enable are racy? |
| |
On 01/27, Peter Zijlstra wrote: > > +void task_function_trampoline(void *data) > +{ > + struct task_function_call *tfc = data; > + struct task_struct *p = tfc->p; > + struct rq *rq = this_rq(); > + > +#ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW > + if (rq->in_ctxsw) > + return; > +#endif > + > + if (rq->curr != p) > + return;
Yes, I think this should solve the problem.
> prepare_task_switch(struct rq *rq, struct task_struct *prev, > struct task_struct *next) > { > +#ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW > + rq->in_ctxsw = 1; > +#endif > + sched_info_switch(prev, next); > + perf_event_task_sched_out(prev, next); > fire_sched_out_preempt_notifiers(prev, next); > prepare_lock_switch(rq, next); > prepare_arch_switch(next); > + trace_sched_switch(prev, next); > }
Yes, I was wondering why schedule() calls perf_event_task_sched_out(). This way the code looks more symmetrical/understandable.
> /** > @@ -2823,6 +2860,7 @@ static void finish_task_switch(struct rq *rq, struct task_struct *prev) > perf_event_task_sched_in(current); > #ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW > local_irq_enable(); > + rq->in_ctxsw = 0;
If we think that context_switch finishes here, probably it would be more clean to clear ->in_ctxsw before local_irq_enable().
> #endif /* __ARCH_WANT_INTERRUPTS_ON_CTXSW */ > finish_lock_switch(rq, prev);
But, otoh, maybe finish_lock_switch() can clear in_ctxsw, it already checks __ARCH_WANT_INTERRUPTS_ON_CTXSW. Likewise, perhaps it can be set in prepare_lock_switch() which enables irqs.
But this is cosmetic and up to you.
Oleg.
| |