Messages in this thread | | | Date | Sun, 6 Mar 2016 18:35:30 +0900 | From | Sergey Senozhatsky <> | Subject | Re: [RFC][PATCH v2 1/2] printk: Make printk() completely async |
| |
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/1454505240-23446-2-git-send-email-mhocko@kernel.org ). > > 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
| |