Re: sched: BUG in load_balance

[Steven Rostedt]

On Mon, 2013-02-18 at 21:19 -0500, Sasha Levin wrote:

> > Only compiled tested:
> > 
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 0fcdbff..a31174c 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -5222,9 +5222,9 @@ void idle_balance(int this_cpu, struct rq *this_rq)
> >  	update_rq_runnable_avg(this_rq, 1);
> >  
> >  	/*
> > -	 * Drop the rq->lock, but keep preempt disabled.
> > +	 * Drop the rq->lock, but keep softirqs disabled.
> >  	 */
> > -	preempt_disable();
> > +	local_bh_disable();
> >  	raw_spin_unlock_irq(&this_rq->lock);
> >  
> >  	update_blocked_averages(this_cpu);
> > @@ -5253,7 +5253,7 @@ void idle_balance(int this_cpu, struct rq *this_rq)
> >  	rcu_read_unlock();
> >  
> >  	raw_spin_lock_irq(&this_rq->lock);
> > -	preempt_enable();
> > +	local_bh_enable();
> 
> I have to admit, I'm slightly confused with the patch: there's a raw_spin_lock_irq()
> followed by local_bh_enable(). afaik it's illegal to call local_bh_enable() with
> interrupts disabled.
> 

Bah, you're right. I was trying to enable interrupts without enabling
softirqs, but if an interrupt happens here and raises a softirq, it will
miss being executed by the local_bh_enabled().

We can keep the preempt disable and only disable local_bh, around the
idle_balance(). But still, I'm getting uncomfortable with these patches,
they may need more work, and Peter's not too happy with them either.

Ingo,

Can you revert this and the previous patch, and I'll work with Peter to
get something that we can agree on, where we can hopefully remove the
idle hooks from the scheduler.

Thanks,

-- Steve


-- Steve



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/