Messages in this thread Patch in this message | | | Date | Wed, 26 Jan 2011 18:53:22 +0100 | From | Oleg Nesterov <> | Subject | Re: Q: perf_install_in_context/perf_event_enable are racy? |
| |
On 01/24, Oleg Nesterov wrote: > > Frederic, All, can't we simplify this?
Well, to clarify, it looks simpler to me ;)
But if you don't like this approach, lets use task_events_schedulable flag.
> 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. > > ... > > 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.
Please see the untested patch below. It doesn't change perf_event_enable(), only perf_install_in_context(). Just for early review to know your opinion. To simplify the reading, here is the code:
void task_force_schedule(struct task_struct *p) { struct rq *rq;
preempt_disable(); rq = task_rq(p); if (rq->curr == p) wake_up_process(rq->stop); preempt_enable(); }
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); return; }
for (;;) { raw_spin_lock_irq(&ctx->lock); /* * The lock prevents that this context is * scheduled in, we can add the event safely. */ if (!ctx->is_active) add_event_to_ctx(event, ctx); raw_spin_unlock_irq(&ctx->lock);
if (event->attach_state & PERF_ATTACH_CONTEXT) { task_force_schedule(task); break; }
task_oncpu_function_call(task, __perf_install_in_context, event); if (event->attach_state & PERF_ATTACH_CONTEXT) break; } }
Oleg.
include/linux/sched.h | 1 + kernel/sched.c | 11 +++++++++++ kernel/perf_event.c | 49 +++++++++++++++++++++---------------------------- 3 files changed, 33 insertions(+), 28 deletions(-)
--- perf/include/linux/sched.h~1_force_resched 2011-01-14 18:21:04.000000000 +0100 +++ perf/include/linux/sched.h 2011-01-26 17:54:28.000000000 +0100 @@ -2584,6 +2584,7 @@ static inline void inc_syscw(struct task extern void task_oncpu_function_call(struct task_struct *p, void (*func) (void *info), void *info); +extern void task_force_schedule(struct task_struct *p); #ifdef CONFIG_MM_OWNER extern void mm_update_next_owner(struct mm_struct *mm); --- perf/kernel/sched.c~1_force_resched 2011-01-20 20:37:11.000000000 +0100 +++ perf/kernel/sched.c 2011-01-26 17:52:42.000000000 +0100 @@ -1968,6 +1968,17 @@ void sched_set_stop_task(int cpu, struct } } +void task_force_schedule(struct task_struct *p) +{ + struct rq *rq; + + preempt_disable(); + rq = task_rq(p); + if (rq->curr == p) + wake_up_process(rq->stop); + preempt_enable(); +} + /* * __normal_prio - return the priority that is based on the static prio */ --- perf/kernel/perf_event.c~2_install_ctx_via_resched 2011-01-21 18:41:02.000000000 +0100 +++ perf/kernel/perf_event.c 2011-01-26 18:32:30.000000000 +0100 @@ -943,16 +943,10 @@ static void __perf_install_in_context(vo /* * 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 events. + * the current task context of this cpu. */ - if (ctx->task && cpuctx->task_ctx != ctx) { - if (cpuctx->task_ctx || ctx->task != current) - return; - cpuctx->task_ctx = ctx; - } + if (ctx->task && cpuctx->task_ctx != ctx) + return; raw_spin_lock(&ctx->lock); ctx->is_active = 1; @@ -1030,27 +1024,26 @@ perf_install_in_context(struct perf_even return; } -retry: - task_oncpu_function_call(task, __perf_install_in_context, - event); - - raw_spin_lock_irq(&ctx->lock); - /* - * we need to retry the smp call. - */ - if (ctx->is_active && list_empty(&event->group_entry)) { + for (;;) { + raw_spin_lock_irq(&ctx->lock); + /* + * The lock prevents that this context is + * scheduled in, we can add the event safely. + */ + if (!ctx->is_active) + add_event_to_ctx(event, ctx); raw_spin_unlock_irq(&ctx->lock); - goto retry; - } - /* - * The lock prevents that this context is scheduled in so we - * can add the event safely, if it the call above did not - * succeed. - */ - if (list_empty(&event->group_entry)) - add_event_to_ctx(event, ctx); - raw_spin_unlock_irq(&ctx->lock); + if (event->attach_state & PERF_ATTACH_CONTEXT) { + task_force_schedule(task); + break; + } + + task_oncpu_function_call(task, __perf_install_in_context, + event); + if (event->attach_state & PERF_ATTACH_CONTEXT) + break; + } } /*
| |