On (03/06/16 16:18), Tetsuo Handa wrote: > Sergey Senozhatsky wrote: > > printk() is expected to work under different conditions and in different > > scenarios, including corner cases of OOM when all of the workers are busy > > (e.g. allocating memory). Thus by default printk() uses its own dedicated > > workqueue with WQ_MEM_RECLAIM bit set. It falls back to system_long_wq > > (console_unlock() is time unbound) only if it has failed to allocate > > printk_wq. Another thing to mention, is that deferred printk() messages > > may appear before printk_wq is allocated, so in the very beginning we > > have to printk deferred messages the old way -- in IRQ context. > > I think we should not depend on system_long_wq which does not have > WQ_MEM_RECLAIM bit. If workqueue allocation upon boot fails (due to ENOMEM), > such systems won't be able to start userspace programs.
well, yes. system_long_wq is a fallback, but probably just BUG_ON(!printk_wq) would be a better thing to do. > Moreover, I don't like use of a workqueue even if printk_wq was allocated > with WQ_MEM_RECLAIM bit. As you can see in the discussion of the OOM reaper, > the OOM reaper chose a dedicated kernel thread rather than a workqueue > ( > http://lkml.kernel.org/r/[email protected] > ). > > Blocking actual printing until ongoing workqueue item calls > schedule_timeout_*() > is not nice (see commit 373ccbe59270 and 564e81a57f97). printk and console_drivers are expected to be 'callable' from any context, including IRQs, or under spin_locks, etc. so neither part of printk->console_unlock()->console_driver->write() can sleep. console_driver->write() is quite often something like this foo_write() { spin_lock_irqsave(&port->lock); uart_console_write( ... foo_put_char); spin_unlock_iqrestore(&port->lock); } and foo_put_char(...), perhaps, waits for device to become ready transaction while (!(UART_GET_STATUS(port) & TXEMPTY)) cpu_relax(); and then sends out the character to the device UART_SET_DATA(port, (unsigned char)ch) so an already executing printk_wq item shouldn't block because of OOM happening on another CPU. do you mean a new worker allocation delay and a MAYDAY timer delay? > Use of WQ_MEM_RECLAIM means we add a task_struct for that workqueue. Thus, > using > a kernel thread does not change total number of task_struct compared to > WQ_MEM_RECLAIM > approach. I think actual printing should occur as soon as possible rather > than randomly > deferred until workqueue item sleeps. hm, need to take a look. > > +static void printing_work_func(struct work_struct *work) > > +{ > > + console_lock(); > > + console_unlock(); > > +} > > Is this safe? If somebody invokes the OOM killer between console_lock() > and console_unlock(), won't this cause OOM killer messages not printed? between console_lock() and console_unlock() nothing can steal that lock from worker; so once a worker grabbed the console_lock() it will print (and console_lock() does not mean "lock logbuf", printk() from other CPUs can add messages to the logbuf while worker loops in console_unlock()). if you mean something like console_lock(); for (...) { do_foo() { ... pr_err(" ... foo message ...\n"); ... } } console_unlock(); then yes, nothing will be printed until that process executes console_unlock(), because it's console_unlock() that pushes the messages to console drivers. (console_lock()/console_unlock() have other problems and issues, which are not addressed in this patch set). there is only one function that ignores the state of console semaphore: panic()->console_flush_on_panic() but that's a separate thing, not related to this patch set. if you mean something else here, then please more details. -ss
