Re: [BUG] TASK_DEAD task is able to be woken up in special condition

[Oleg Nesterov <oleg@redhat.com>]

On 01/25, Peter Zijlstra wrote:
>
> On Wed, 2012-01-25 at 16:45 +0100, Oleg Nesterov wrote:
> > > > >
> > > > >                 for (;;) {
> > > > >                         tsk->state = TASK_DEAD;
> > > > >                         schedule();
> > > > >                 }
> > > > >
> > > > > __schedule() can't race with ttwu() once it takes rq->lock. If the
> > > > > exiting task is deactivated, finish_task_switch() will see EXIT_DEAD.
> > > >
> > > > TASK_DEAD, right?
> >
> > Yes, but... I simply can't understand what I was thinking about.
> > And probably I missed something again, but I think this can't work.
>
> Oh man, total confusion. :-) Every time I look at this bug I see
> different shadows on the wall.

Same here ;)

And this time I do not understand your reply.

> > Afaics, this can only help to prevent the race with ttwu_remote()
> > doing ttwu_do_wakeup() under rq->lock.
>
> ttwu_do_wakeup() must always be called with rq->lock held.

Yes sure. I meant the code above can't race with p->on_rq == T case.

> > But we still can race with the !p->on_rq case which sets TASK_WAKING.
> > It can do this after finish_task_switch() observes TASK_DEAD and does
> > put_task_struct().
>
> <random scribbling that got erased>
>
> No, see below !p->on_rq isn't possible and thus pi_lock is indeed
> sufficient.

Which pi_lock? __schedule() doesn't take it. Hmm, see below...

> > > I think Yasunori-San's patch isn't
> > > sufficient, note how the p->state = TASK_RUNNING in ttwu_do_wakeup() can
> > > happen outside of p->pi_lock when the task gets queued on a remote cpu.
> >
> > Hmm, really? I am not sure, but I do not trust myself.
> >
> > To simplify, you mean that
> >
> > 	mb();
> > 	unlock_wait(pi_lock);
> >
> > 	tsk->state = TASK_DEAD;
> >
> > can change ->state from TASK_WAKING to TASK_DEAD, right? Is this really
> > possible? ttwu() ensures p->on_rq == F in this case.
>
> Ahhh.. hold on, p->on_rq must be true, since we disabled preemption
> before setting TASK_DEAD, so the thing cannot be scheduled out.

Why? __schedule() checks "preempt_count() & PREEMPT_ACTIVE". And it
should be scheduled out, in general this task struct will be freed soon.

> Does this mean that both Yasunori-San's solution and yours work again?

I think that Yasunori-San's solution should work.

But,

> /me goes in search of a fresh mind.. shees!

Yes! I need the fresh head too. Probably just to realize I was completely
wrong again.

Oleg.