CFQ + 2.6.13-rc4-RT-V0.7.52-02 = BUG: scheduling with irqs disabled

CFQ + 2.6.13-rc4-RT-V0.7.52-02 = BUG: scheduling with irqs disabled

[Lee Revell]

Just found this in dmesg.

BUG: scheduling with irqs disabled: libc6.postinst/0x20000000/13229
caller is ___down_mutex+0xe9/0x1a0
 [<c029c1f9>] schedule+0x59/0xf0 (8)
 [<c029ced9>] ___down_mutex+0xe9/0x1a0 (28)
 [<c0221832>] cfq_exit_single_io_context+0x22/0xa0 (84)
 [<c02218ea>] cfq_exit_io_context+0x3a/0x50 (16)
 [<c021db84>] exit_io_context+0x64/0x70 (16)
 [<c011efda>] do_exit+0x5a/0x3e0 (20)
 [<c011f3ca>] do_group_exit+0x2a/0xb0 (24)
 [<c0103039>] syscall_call+0x7/0xb (20)

Lee

Re: CFQ + 2.6.13-rc4-RT-V0.7.52-02 = BUG: scheduling with irqs disabled

[Jens Axboe]

On Wed, Aug 24 2005, Lee Revell wrote:
> Just found this in dmesg.
> 
> BUG: scheduling with irqs disabled: libc6.postinst/0x20000000/13229
> caller is ___down_mutex+0xe9/0x1a0
>  [<c029c1f9>] schedule+0x59/0xf0 (8)
>  [<c029ced9>] ___down_mutex+0xe9/0x1a0 (28)
>  [<c0221832>] cfq_exit_single_io_context+0x22/0xa0 (84)
>  [<c02218ea>] cfq_exit_io_context+0x3a/0x50 (16)
>  [<c021db84>] exit_io_context+0x64/0x70 (16)
>  [<c011efda>] do_exit+0x5a/0x3e0 (20)
>  [<c011f3ca>] do_group_exit+0x2a/0xb0 (24)
>  [<c0103039>] syscall_call+0x7/0xb (20)

Hmm, Ingo I seem to remember you saying that the following construct:

        local_irq_save(flags);
        spin_lock(lock);

which is equivelant to spin_lock_irqsave() in mainline being illegal in
-RT, is that correct? This is what cfq uses right now for an exiting
task, as the above trace indicates.

-- 
Jens Axboe

Re: CFQ + 2.6.13-rc4-RT-V0.7.52-02 = BUG: scheduling with irqs disabled

[Esben Nielsen]

On Wed, 24 Aug 2005, Jens Axboe wrote:

> On Wed, Aug 24 2005, Lee Revell wrote:
> > Just found this in dmesg.
> > 
> > BUG: scheduling with irqs disabled: libc6.postinst/0x20000000/13229
> > caller is ___down_mutex+0xe9/0x1a0
> >  [<c029c1f9>] schedule+0x59/0xf0 (8)
> >  [<c029ced9>] ___down_mutex+0xe9/0x1a0 (28)
> >  [<c0221832>] cfq_exit_single_io_context+0x22/0xa0 (84)
> >  [<c02218ea>] cfq_exit_io_context+0x3a/0x50 (16)
> >  [<c021db84>] exit_io_context+0x64/0x70 (16)
> >  [<c011efda>] do_exit+0x5a/0x3e0 (20)
> >  [<c011f3ca>] do_group_exit+0x2a/0xb0 (24)
> >  [<c0103039>] syscall_call+0x7/0xb (20)
> 
> Hmm, Ingo I seem to remember you saying that the following construct:
> 
>         local_irq_save(flags);
>         spin_lock(lock);
> 
> which is equivelant to spin_lock_irqsave() in mainline being illegal in
> -RT, is that correct? 

I can easily answer this for Ingo.

Yes, spin_lock(lock) is blocking since lock is mutex, not a spinlock under
preempt-rt. But isn't it easy to fix? Replace the two lines by
spin_lock_irqsave(flags). That would work for both preempt-rt
and !preempt-rt.

You supposed to ask if the macro name spin_lock() isn't confusing. It very
much is, but one of Ingo's aims is not to change existing code too much.
The purist would probably change all instances of spin_lock() to lock() or
down() to stop refering to a specific lock type when it can be changed
with config-options. That would, however, require a large patch,
which does the preempt-rt branch harder to merge with the main-line.

Esben


> This is what cfq uses right now for an exiting
> task, as the above trace indicates.
> 
> -- 
> Jens Axboe

Re: CFQ + 2.6.13-rc4-RT-V0.7.52-02 = BUG: scheduling with irqs disabled

[Jens Axboe]

On Wed, Aug 24 2005, Esben Nielsen wrote:
> On Wed, 24 Aug 2005, Jens Axboe wrote:
> 
> > On Wed, Aug 24 2005, Lee Revell wrote:
> > > Just found this in dmesg.
> > > 
> > > BUG: scheduling with irqs disabled: libc6.postinst/0x20000000/13229
> > > caller is ___down_mutex+0xe9/0x1a0
> > >  [<c029c1f9>] schedule+0x59/0xf0 (8)
> > >  [<c029ced9>] ___down_mutex+0xe9/0x1a0 (28)
> > >  [<c0221832>] cfq_exit_single_io_context+0x22/0xa0 (84)
> > >  [<c02218ea>] cfq_exit_io_context+0x3a/0x50 (16)
> > >  [<c021db84>] exit_io_context+0x64/0x70 (16)
> > >  [<c011efda>] do_exit+0x5a/0x3e0 (20)
> > >  [<c011f3ca>] do_group_exit+0x2a/0xb0 (24)
> > >  [<c0103039>] syscall_call+0x7/0xb (20)
> > 
> > Hmm, Ingo I seem to remember you saying that the following construct:
> > 
> >         local_irq_save(flags);
> >         spin_lock(lock);
> > 
> > which is equivelant to spin_lock_irqsave() in mainline being illegal in
> > -RT, is that correct? 
> 
> I can easily answer this for Ingo.
> 
> Yes, spin_lock(lock) is blocking since lock is mutex, not a spinlock under
> preempt-rt. But isn't it easy to fix? Replace the two lines by
> spin_lock_irqsave(flags). That would work for both preempt-rt
> and !preempt-rt.

Well, it might and it might not be. There's a correctness and
optimization side to it. For this case it is probably doable, but I have
to say that the new semantics defy normal logic.

> You supposed to ask if the macro name spin_lock() isn't confusing. It very
> much is, but one of Ingo's aims is not to change existing code too much.
> The purist would probably change all instances of spin_lock() to lock() or
> down() to stop refering to a specific lock type when it can be changed
> with config-options. That would, however, require a large patch,
> which does the preempt-rt branch harder to merge with the main-line.

I can certainly understand Ingo's point of view, as long as he is
maintaining the patch outside of the kernel. Where it ever to go in,
this would have to change.

-- 
Jens Axboe

Re: CFQ + 2.6.13-rc4-RT-V0.7.52-02 = BUG: scheduling with irqs disabled

[Ingo Molnar]

* Esben Nielsen <simlo@phys.au.dk> wrote:

> Yes, spin_lock(lock) is blocking since lock is mutex, not a spinlock 
> under preempt-rt. But isn't it easy to fix? Replace the two lines by 
> spin_lock_irqsave(flags). That would work for both preempt-rt and 
> !preempt-rt.

at this moment we do not pester upstream developers with PREEMPT_RT 
details. It is not at all clear at this moment whether and if how any 
API changes will look like. So there's nothing "to fix" at all!!

for the cases where there's a clear cleanup potential from merging flags 
and locks management we submit separate patches, which stand on their 
own. It happened already, and it will happen in the future. The rest 
Jens does not need to care about.

_often_, trouble on the PREEMPT_RT side highlights some potential 
trouble on the upstream side. Unclean locking rules, unecessary/unsafe 
disabling of interrupts, etc. But no way is there a 1:1 relationship.  
E.g. in this particular case i already fixed the warning in the current 
-RT tree.

	Ingo

Re: CFQ + 2.6.13-rc4-RT-V0.7.52-02 = BUG: scheduling with irqs disabled

[Ingo Molnar]

* Jens Axboe <axboe@suse.de> wrote:

> > BUG: scheduling with irqs disabled: libc6.postinst/0x20000000/13229
> > caller is ___down_mutex+0xe9/0x1a0
> >  [<c029c1f9>] schedule+0x59/0xf0 (8)
> >  [<c029ced9>] ___down_mutex+0xe9/0x1a0 (28)
> >  [<c0221832>] cfq_exit_single_io_context+0x22/0xa0 (84)
> >  [<c02218ea>] cfq_exit_io_context+0x3a/0x50 (16)
> >  [<c021db84>] exit_io_context+0x64/0x70 (16)
> >  [<c011efda>] do_exit+0x5a/0x3e0 (20)
> >  [<c011f3ca>] do_group_exit+0x2a/0xb0 (24)
> >  [<c0103039>] syscall_call+0x7/0xb (20)
> 
> Hmm, Ingo I seem to remember you saying that the following construct:
> 
>         local_irq_save(flags);
>         spin_lock(lock);
> 
> which is equivelant to spin_lock_irqsave() in mainline being illegal 
> in -RT, is that correct? This is what cfq uses right now for an 
> exiting task, as the above trace indicates.

yes, that's correct. Mainline's exit_io_contexts() uses the above 
construct because there's no task_lock_irqsave(current, flags) API.

note that recent -RT kernels are a lot less drastic about these cases 
and print a once-per-bootup warning, not a scary message like above.  
This message should not happen in recent -RT kernels.

The problem was this piece of code in exit_io_contexts():

        local_irq_save(flags);
        task_lock(current);
        ioc = current->io_context;
        current->io_context = NULL;
        ioc->task = NULL;
        task_unlock(current);
        local_irq_restore(flags);

i understand the detached use of flags, but i'm also wondering why irqs 
have to be disabled here in the first place? At this point in do_exit() 
we should normally not have any pending IO attached to our io_context.  
What am i missing?

	Ingo

Re: CFQ + 2.6.13-rc4-RT-V0.7.52-02 = BUG: scheduling with irqs disabled

[Jens Axboe]

On Thu, Aug 25 2005, Ingo Molnar wrote:
> 
> * Jens Axboe <axboe@suse.de> wrote:
> 
> > > BUG: scheduling with irqs disabled: libc6.postinst/0x20000000/13229
> > > caller is ___down_mutex+0xe9/0x1a0
> > >  [<c029c1f9>] schedule+0x59/0xf0 (8)
> > >  [<c029ced9>] ___down_mutex+0xe9/0x1a0 (28)
> > >  [<c0221832>] cfq_exit_single_io_context+0x22/0xa0 (84)
> > >  [<c02218ea>] cfq_exit_io_context+0x3a/0x50 (16)
> > >  [<c021db84>] exit_io_context+0x64/0x70 (16)
> > >  [<c011efda>] do_exit+0x5a/0x3e0 (20)
> > >  [<c011f3ca>] do_group_exit+0x2a/0xb0 (24)
> > >  [<c0103039>] syscall_call+0x7/0xb (20)
> > 
> > Hmm, Ingo I seem to remember you saying that the following construct:
> > 
> >         local_irq_save(flags);
> >         spin_lock(lock);
> > 
> > which is equivelant to spin_lock_irqsave() in mainline being illegal 
> > in -RT, is that correct? This is what cfq uses right now for an 
> > exiting task, as the above trace indicates.
> 
> yes, that's correct. Mainline's exit_io_contexts() uses the above 
> construct because there's no task_lock_irqsave(current, flags) API.
> 
> note that recent -RT kernels are a lot less drastic about these cases 
> and print a once-per-bootup warning, not a scary message like above.  
> This message should not happen in recent -RT kernels.
> 
> The problem was this piece of code in exit_io_contexts():
> 
>         local_irq_save(flags);
>         task_lock(current);
>         ioc = current->io_context;
>         current->io_context = NULL;
>         ioc->task = NULL;
>         task_unlock(current);
>         local_irq_restore(flags);
> 
> i understand the detached use of flags, but i'm also wondering why irqs 
> have to be disabled here in the first place? At this point in do_exit() 
> we should normally not have any pending IO attached to our io_context.  
> What am i missing?

There can quite easily be lots of pending IO for the io_context (and, in
CFQ's case, below cfq_io_contexts), task exiting is completely decoupled
from any pending io.

Then there's the cfq_exit_io_context() locking. I have to ponder this a
bit, I cannot even convince myself that it is currently safe right now.

-- 
Jens Axboe

Re: CFQ + 2.6.13-rc4-RT-V0.7.52-02 = BUG: scheduling with irqs disabled

[Ingo Molnar]

* Jens Axboe <axboe@suse.de> wrote:

> There can quite easily be lots of pending IO for the io_context (and, 
> in CFQ's case, below cfq_io_contexts), task exiting is completely 
> decoupled from any pending io.

yes, but that only affects the io_context reference count. Actual new 
use of tsk->io_context should only be possible on the IO-submission 
side, which should all have stopped by the time we execute do_exit().  
(and it's synchronous anyway, so the fact that we are executing in the 
kernel prevents the same thread from submitting new IO, in this case.)

i.e. the removal of tsk->io_context can be done without locking out 
interrupts. No interrupt or io_context is supposed to access 
current->io_context at that point.

> Then there's the cfq_exit_io_context() locking. I have to ponder this 
> a bit, I cannot even convince myself that it is currently safe right 
> now.

i think it should be mostly safe already - it seems to be overlocking a 
bit. E.g. the read_lock_irq(&tasklist_lock) could be a simple 
read_lock() i think.

	Ingo

Re: CFQ + 2.6.13-rc4-RT-V0.7.52-02 = BUG: scheduling with irqs disabled

[Jens Axboe]

On Thu, Aug 25 2005, Ingo Molnar wrote:
> 
> * Jens Axboe <axboe@suse.de> wrote:
> 
> > There can quite easily be lots of pending IO for the io_context (and, 
> > in CFQ's case, below cfq_io_contexts), task exiting is completely 
> > decoupled from any pending io.
> 
> yes, but that only affects the io_context reference count. Actual new 
> use of tsk->io_context should only be possible on the IO-submission 
> side, which should all have stopped by the time we execute do_exit().  
> (and it's synchronous anyway, so the fact that we are executing in the 
> kernel prevents the same thread from submitting new IO, in this case.)
> 
> i.e. the removal of tsk->io_context can be done without locking out 
> interrupts. No interrupt or io_context is supposed to access 
> current->io_context at that point.

Well that's not quite correct, changes were made that allow lookup of
the cic from another task. What happens is that process A will be
dirtying lots of data and process B will be quitely reading other data.
Process B can then get unlucky and have to do page reclaim on behalf of
process A, simply because it tries to allocate a page under memory
pressure.

This is probably fairly rare, because async writeout goes to a dedicated
queue typically bound to one (or more) of the pdflush threads which
never exit. None the less, it needs fixing.

This used to be safe (it used rcu and the cfq_cic_lock to protect read
and write side respectively), I _think_ this was dropped at some point
because I dropped the cross-lookup. I have to ponder how best to fix
this...

-- 
Jens Axboe