Messages in this thread | | | Date | Thu, 2 Nov 2017 17:53:13 +0900 | From | Sergey Senozhatsky <> | Subject | Re: [PATCH] mm: don't warn about allocations which stall for too long |
| |
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?
> 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
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.
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.
-ss
| |