On Thu, 2 Nov 2017 17:53:13 +0900
Sergey Senozhatsky <[email protected]> wrote:
> On (10/31/17 15:32), Steven Rostedt wrote:
> [..]
> > (new globals)
> > static DEFINE_SPIN_LOCK(console_owner_lock);
> > static struct task_struct console_owner;
> > static bool waiter;
> >
> > console_unlock() {
> >
> > [ Assumes this part can not preempt ]
> >
> > spin_lock(console_owner_lock);
> > console_owner = current;
> > spin_unlock(console_owner_lock);
>
> + disables IRQs?
Yes, this was pseudo code, just to get an idea out. I'll have a patch
soon that will include all the nasty details.
>
> > for each message
> > write message out to console
> >
> > if (READ_ONCE(waiter))
> > break;
> >
> > spin_lock(console_owner_lock);
> > console_owner = NULL;
> > spin_unlock(console_owner_lock);
> >
> > [ preemption possible ]
>
> otherwise
>
> printk()
> if (console_trylock())
> console_unlock()
> preempt_disable()
> spin_lock(console_owner_lock);
> console_owner = current;
> spin_unlock(console_owner_lock);
> .......
> spin_lock(console_owner_lock);
> IRQ
> printk()
> console_trylock() // fails so we go to busy-loop part
> spin_lock(console_owner_lock); << deadlock
Yeah, I do disable interrupts. The pseudo code was just a way to
quickly convey the idea. I said "spin_lock" where I could have just
said "lock".
>
>
> even if we would replace spin_lock(console_owner_lock) with IRQ
> spin_lock, we still would need to protect against IRQs on the very
> same CPU. right? IOW, we need to store smp_processor_id() of a CPU
> currently doing console_unlock() and check it in vprintk_emit()?
> and we need to protect the entire console_unlock() function. not
> just the printing loop, otherwise the IRQ CPU will spin forever
> waiting for itself to up() the console_sem.
Yes and it will.
>
> this somehow reminds me of "static unsigned int logbuf_cpu", which
> we used to have in vprintk_emit() and were happy to remove it...
>
>
> the whole "console_unlock() is non-preemptible" can bite, I'm
> afraid. it's not always printk()->console_unlock(), sometimes
> it's console_lock()->console_unlock() that has to flush the
> logbuf.
>
> CPU0 CPU1 ~ CPU99
> console_lock();
> printk(); ... printk();
> console_unlock()
> preempt_disable();
> for (;;)
> call_console_drivers();
> <<lockup>>
>
>
> this pattern is not so unusual. _especially_ in the existing scheme
> of things.
>
> not to mention the problem of "the last printk()", which will take
> over and do the flush.
>
> CPU0 CPU1 ~ CPU99
> console_lock();
> printk(); ... printk();
> console_unlock();
> IRQ on CPU2
> printk()
> // take over console_sem
> console_unlock()
>
> and so on.
> seems that there will be lots of if-s.
Let's wait for the patch and talk more after I post it.
-- Steve
