Re: v2.6.26-rc7: BUG task_struct: Poison overwritten

["Vegard Nossum" <vegard.nossum@gmail.com>]

On Sat, Jun 21, 2008 at 11:21 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> But it looks like there might be some cpu hotplug race with group
> scheduling - heiko (s390) and avi (x86_64) reported some cpu hotplug
> crashes. We're still looking into those.

Thanks.

I was poking around in kernel/sched.c and noticed something odd: In
migrate_dead(), we have this code:

        /*
         * Drop lock around migration; if someone else moves it,
         * that's OK. No task can be added to this CPU, so iteration is
         * fine.
         */
        spin_unlock_irq(&rq->lock);
        move_task_off_dead_cpu(dead_cpu, p);
        spin_lock_irq(&rq->lock);

which is fine in itself, I guess. But spin_unlock_irq() will enable
interrupts. And move_task_off_dead_cpu() has this comment:

/*
 * Figure out where task on dead CPU should go, use force if necessary.
 * NOTE: interrupts should be disabled by the caller
 */
static void move_task_off_dead_cpu(int dead_cpu, struct task_struct *p)
{

...but here, interrupts will not be disabled. On the other hand
__migrate_task_irq() (called by move_task_off_dead_cpu()) calls
local_irq_disable() itself... What do you think of this? Is the
comment wrong? Or is there a difference between "interrupts" and
"local_irq"?


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036