lkml.org 
[lkml]   [2011]   [Mar]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH,RFC] perf: panic due to inclied cpu context task_ctx value
From
Date
On Tue, 2011-03-29 at 18:28 +0200, Oleg Nesterov wrote:
> On 03/29, Peter Zijlstra wrote:
> >
> > On Mon, 2011-03-28 at 18:56 +0200, Oleg Nesterov wrote:
> > >
> > > jump_label_dec:
> > >
> > > if (atomic_dec_and_test(key))
> > > jump_label_disable(key);
> > >
> > > Another thread can create the PERF_ATTACH_TASK event in between
> > > and call jump_label_update(JUMP_LABEL_ENABLE) first. Looks like,
> > > jump_label_update() should ensure that "type" matches the state
> > > of the "*key" under jump_label_lock().
> >
> > No I think you're right, and I think we fixed that but it looks like
> > Ingo still didn't merge the new jump-label patches :/
>
> OK. To remind, we have another problem, perf_install can race with exit.
> But lets ignore this for now...

Yay! ;-)

>
> You know, I honestly tried to convince myself I can understand your
> patch. All I can say, I'll try to read it again ;) But the main idea
> is clear, we give more respect to ->nr_events and once it is zero
> task_ctx must not be active.

Right, except I'm leaking an ->is_active and was seriously considering
your clever idea of splitting the sched_in and sched_out jump-labels.

> > @@ -2114,8 +2107,19 @@ static void perf_event_context_sched_in(
> > struct perf_cpu_context *cpuctx;
> >
> > cpuctx = __get_cpu_context(ctx);
> > - if (cpuctx->task_ctx == ctx)
> > + raw_spin_lock(&ctx->lock);
> > + /*
> > + * Serialize against perf_install_in_context(), the interesting case
> > + * is where perf_install_in_context() finds the context inactive and
> > + * another cpu is just about to schedule the task in. In that case
> > + * we need to avoid observing a stale ctx->nr_events.
>
> I don't understand the comment... We can't race __perf_install_in_context,
> it can only run on the same CPU but we are called with irqs disabled.

Ah, I meant a race with perf_install_in_context() where task_function()
fails because !task_curr(), in that case we'll attempt
add_event_to_ctx() from the remote cpu. If we race wrong with sched_in
nobody might schedule the event.

> > + ctx->is_active = 1;
> > + if (cpuctx->task_ctx == ctx || !ctx->nr_events) {
> > + raw_spin_lock(&ctx->lock);
> > return;
>
> I guess you meant _unlock.

Yeah, utter fail day today :/

> But now I don't understand what ->is_active means. We make it true,
> but doesn't set cpuctx->task_ctx = ctx. Why __perf_event_release()
> clears ->is_active then?

Yeha, that's the big problem I have with this.

> It seems to me, instead we should change ctx_sched_in() to check
> nr_events and do nothing if it is zero.

you mean, not set ->is_active?

> > + cpuctx->task_ctx = ctx;
> > +
> > ctx_sched_in(ctx, cpuctx, EVENT_PINNED, task);
>
> But we already dropped ctx->lock, __perf_event_release() can remove
> the last event before ctx_sched_in() takes it again.
>
> This should be moved into ctx_sched_in() afaics, but this is not simple.
>
> So, perhaps we can take ctx->lock and check nr_events after the 2nd
> ctx_sched_in(). If it is zero, we should clear task_ctx/is_active.
>
> Perhaps. Right now I got lost.

Yeah, you and me both.. I went to look at something else because I
simply confused myself more. Hopefully tomorrow will bring some sanity.




\
 
 \ /
  Last update: 2011-03-29 21:01    [W:0.124 / U:0.908 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site